On Thu, Nov 7, 2019 at 8:35 AM Eric Dumazet <edumazet@xxxxxxxxxx> wrote:
>
> On Thu, Nov 7, 2019 at 8:11 AM Paul E. McKenney <paulmck@xxxxxxxxxx> wrote:
> >
> > OK, so this is due to timer_pending() lockless access to ->entry.pprev
> > to determine whether or not the timer is on the list. New one on me!
> >
> > Given that use case, I don't have an objection to your patch to list.h.
> >
> > Except...
> >
> > Would it make sense to add a READ_ONCE() to hlist_unhashed()
> > and to then make timer_pending() invoke hlist_unhashed()? That
> > would better confine the needed uses of READ_ONCE().
>
> Sounds good to me, I had the same idea but was too lazy to look at the
> history of timer_pending()
> to check if the pprev pointer check was really the same underlying idea.
Note that forcing READ_ONCE() in hlist_unhashed() might force the compiler
to read the pprev pointer twice in some cases.
This was one of the reason for me to add skb_queue_empty_lockless()
variant in include/linux/skbuff.h
/**
* skb_queue_empty_lockless - check if a queue is empty
* @list: queue head
*
* Returns true if the queue is empty, false otherwise.
* This variant can be used in lockless contexts.
*/
static inline bool skb_queue_empty_lockless(const struct sk_buff_head *list)
{
return READ_ONCE(list->next) == (const struct sk_buff *) list;
}
So maybe add a hlist_unhashed_lockless() to clearly document why
callers are using the lockless variant ?
|