Re: [PATCH v5 net 6/6] net/sched: qdisc_destroy() old ingress and clsact Qdiscs before grafting

On Thu, Jun 08, 2023 at 10:48:12AM +0300, Vlad Buslov wrote:

quoted

quoted

It looks like even though 3f05e6886a59 ("net_sched: unset
TCQ_F_CAN_BYPASS when adding filters") was introduced after cls api
unlock by now we have these in exactly the same list of supported
kernels (5.4 LTS and newer). Considering this, the conversion to the
atomic bitops can be done as a standalone fix for cited commit and after
it will have been accepted and backported the qdisc fix can just assume
that qdisc->flags is an atomic bitops field in all target kernels and
use it as-is. WDYT?

Sounds great, how about:

  1. I'll post the non-replay version of the fix (after updating the commit
     message), and we apply that first, as suggested by Jamal

From my side there are no objections to any of the proposed approaches
since we have never had any users with legitimate use-case where they
need to replace/delete a qdisc concurrently with a filter update, so
returning -EBUSY (or -EAGAIN) to the user in such case would work as
either temporary or the final fix.

I see, yeah.

However, Jakub had reservations with such approach so don't know where we
stand now regarding this.

Either way I'd say applying that non-replay version first is better than
leaving the bug unfixed.  It's been many days since the root cause of the
issue has been figured out.  I'll post it, and start making qdisc->flags
atomic.

quoted

  2. Make qdisc->flags atomic

  3. Make the fix better by replaying and using the (now atomic)
     IS-DESTROYING flag with test_and_set_bit() and friends

?

Again, no objections from my side. Ping me if you need help with any of
these.

Sure, thanks, Vlad!
Peilin Ye