From: Jakub Kicinski <kuba@kernel.org>
To: Paolo Abeni <pabeni@redhat.com>
Cc: Cong Wang <xiyou.wangcong@gmail.com>,
davem@davemloft.net, netdev@vger.kernel.org, edumazet@google.com,
Linux Kernel Functional Testing <lkft@linaro.org>,
jhs@mojatatu.com, jiri@resnulli.us, john.hurley@netronome.com
Subject: Re: [PATCH net] net: sched: gred: prevent races when adding offloads to stats
Date: Tue, 17 Jan 2023 11:10:19 -0800 [thread overview]
Message-ID: <20230117111019.50c47ea1@kernel.org> (raw)
In-Reply-To: <7e0d5d6891697d24f9f9509fb8626ea9129b5eb2.camel@redhat.com>
On Tue, 17 Jan 2023 10:00:56 +0100 Paolo Abeni wrote:
> On Sat, 2023-01-14 at 18:20 -0800, Cong Wang wrote:
> > On Thu, Jan 12, 2023 at 08:41:37PM -0800, Jakub Kicinski wrote:
> > > Naresh reports seeing a warning that gred is calling
> > > u64_stats_update_begin() with preemption enabled.
> > > Arnd points out it's coming from _bstats_update().
> >
> > The stack trace looks confusing to me without further decoding.
> >
> > Are you sure we use sch->qstats/bstats in __dev_queue_xmit() there
> > not any netdev stats? It may be a false positive one as they may end up
> > with the same lockdep class.
I didn't repro this myself, TBH, but there is u64_stats_update_begin()
inside _bstats_update(). Pretty sure it will trigger the warning that
preemption is not disabled on non-SMP systems.
> I'm unsure I read you comment correctly. Please note that the
> referenced message includes several splats. The first one - arguably
> the most relevant - points to the lack of locking in the gred control
> path.
Yup, I'm not really sure if we're fixing the right splat for the bug.
But I am fairly confident we should be holding a lock while writing
bstats from the dump path, enqueue/dequeue may run concurrently.
> The posted patch LGTM, could you please re-phrase your doubts?
next prev parent reply other threads:[~2023-01-17 20:17 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-13 4:41 [PATCH net] net: sched: gred: prevent races when adding offloads to stats Jakub Kicinski
2023-01-15 2:20 ` Cong Wang
2023-01-17 9:00 ` Paolo Abeni
2023-01-17 19:10 ` Jakub Kicinski [this message]
2023-01-19 20:19 ` Cong Wang
2023-01-19 21:16 ` Jakub Kicinski
2023-01-19 20:17 ` Cong Wang
2023-01-19 5:10 ` patchwork-bot+netdevbpf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230117111019.50c47ea1@kernel.org \
--to=kuba@kernel.org \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=jhs@mojatatu.com \
--cc=jiri@resnulli.us \
--cc=john.hurley@netronome.com \
--cc=lkft@linaro.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=xiyou.wangcong@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.