[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CANn89i+Cc1aOHVFnYvZ93EDee81RaGNrv47ZBVdQXmxMuuMmww@mail.gmail.com>
Date: Thu, 7 Nov 2019 08:59:49 -0800
From: Eric Dumazet <edumazet@...gle.com>
To: paulmck@...nel.org
Cc: LKML <linux-kernel@...r.kernel.org>,
linux-tip-commits@...r.kernel.org,
syzbot <syzkaller@...glegroups.com>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...nel.org>, Borislav Petkov <bp@...en8.de>
Subject: Re: [tip: timers/core] hrtimer: Annotate lockless access to timer->state
On Thu, Nov 7, 2019 at 8:54 AM Paul E. McKenney <paulmck@...nel.org> wrote:
>
> On Thu, Nov 07, 2019 at 08:39:42AM -0800, Eric Dumazet wrote:
> > On Thu, Nov 7, 2019 at 8:35 AM Eric Dumazet <edumazet@...gle.com> wrote:
> > >
> > > On Thu, Nov 7, 2019 at 8:11 AM Paul E. McKenney <paulmck@...nel.org> 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
>
> Ouch!
>
> > /**
> > * 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 ?
>
> That sounds like a reasonable approach to me. There aren't all that
> many uses of hlist_unhashed(), so a name change should not be a problem.
Maybe I was not clear : I did not rename skb_queue_empty()
I chose to add another helper.
Contexts that can safely use skb_queue_empty() still continue to use
it, since it might help
the compiler to generate better code.
So If I add hlist_unhashed_lockless(), I would only use it from
timer_pending() at first.
Then an audit of the code might reveal other potential users.
Powered by blists - more mailing lists