| Message ID | 20240418073248.2952954-1-edumazet@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | net_sched: first series for RTNL-less qdisc dumps | expand |
On Thu, Apr 18, 2024 at 3:32 AM Eric Dumazet <edumazet@google.com> wrote: > > Medium term goal is to implement "tc qdisc show" without needing > to acquire RTNL. > > This first series makes the requested changes in 14 qdisc. > > Notes : > > - RTNL is still held in "tc qdisc show", more changes are needed. > > - Qdisc returning many attributes might want/need to provide > a consistent set of attributes. If that is the case, their > dump() method could acquire the qdisc spinlock, to pair the > spinlock acquision in their change() method. > For the series: Reviewed-by: Jamal Hadi Salim<jhs@mojatatu.com> Not a show-stopper, we'll run the tdc tests after (and use this as an opportunity to add more tests if needed). For your next series we'll try to do that after you post. cheers, jamal > V2: Addressed Simon feedback (Thanks a lot Simon) > > Eric Dumazet (14): > net_sched: sch_fq: implement lockless fq_dump() > net_sched: cake: implement lockless cake_dump() > net_sched: sch_cbs: implement lockless cbs_dump() > net_sched: sch_choke: implement lockless choke_dump() > net_sched: sch_codel: implement lockless codel_dump() > net_sched: sch_tfs: implement lockless etf_dump() > net_sched: sch_ets: implement lockless ets_dump() > net_sched: sch_fifo: implement lockless __fifo_dump() > net_sched: sch_fq_codel: implement lockless fq_codel_dump() > net_sched: sch_fq_pie: implement lockless fq_pie_dump() > net_sched: sch_hfsc: implement lockless accesses to q->defcls > net_sched: sch_hhf: implement lockless hhf_dump() > net_sched: sch_pie: implement lockless pie_dump() > net_sched: sch_skbprio: implement lockless skbprio_dump() > > include/net/red.h | 12 ++--- > net/sched/sch_cake.c | 110 ++++++++++++++++++++++----------------- > net/sched/sch_cbs.c | 20 +++---- > net/sched/sch_choke.c | 21 ++++---- > net/sched/sch_codel.c | 29 +++++++---- > net/sched/sch_etf.c | 10 ++-- > net/sched/sch_ets.c | 25 +++++---- > net/sched/sch_fifo.c | 13 ++--- > net/sched/sch_fq.c | 108 ++++++++++++++++++++++++-------------- > net/sched/sch_fq_codel.c | 57 ++++++++++++-------- > net/sched/sch_fq_pie.c | 61 ++++++++++++---------- > net/sched/sch_hfsc.c | 9 ++-- > net/sched/sch_hhf.c | 35 ++++++++----- > net/sched/sch_pie.c | 39 +++++++------- > net/sched/sch_skbprio.c | 8 +-- > 15 files changed, 323 insertions(+), 234 deletions(-) > > -- > 2.44.0.683.g7961c838ac-goog >
On Thu, Apr 18, 2024 at 06:23:27AM -0400, Jamal Hadi Salim wrote: > On Thu, Apr 18, 2024 at 3:32 AM Eric Dumazet <edumazet@google.com> wrote: > > > > Medium term goal is to implement "tc qdisc show" without needing > > to acquire RTNL. > > > > This first series makes the requested changes in 14 qdisc. > > > > Notes : > > > > - RTNL is still held in "tc qdisc show", more changes are needed. > > > > - Qdisc returning many attributes might want/need to provide > > a consistent set of attributes. If that is the case, their > > dump() method could acquire the qdisc spinlock, to pair the > > spinlock acquision in their change() method. > > > > For the series: > Reviewed-by: Jamal Hadi Salim<jhs@mojatatu.com> > > Not a show-stopper, we'll run the tdc tests after (and use this as an > opportunity to add more tests if needed). > For your next series we'll try to do that after you post. Hi Jamal, On the topic of tdc, I noticed the following both with and without this series applied. Is this something you are aware of? not ok 990 ce7d - Add mq Qdisc to multi-queue device (4 queues) I'm not sure if it is valid, but I tried running tdc like this: $ ng --build --config tools/testing/selftests/tc-testing/config $ vng -v --run . --user root --cpus 4 -- \ "cd ./tools/testing/selftests/tc-testing; ./tdc.py;"
On Thu, Apr 18, 2024 at 11:08 AM Simon Horman <horms@kernel.org> wrote: > > On Thu, Apr 18, 2024 at 06:23:27AM -0400, Jamal Hadi Salim wrote: > > On Thu, Apr 18, 2024 at 3:32 AM Eric Dumazet <edumazet@google.com> wrote: > > > > > > Medium term goal is to implement "tc qdisc show" without needing > > > to acquire RTNL. > > > > > > This first series makes the requested changes in 14 qdisc. > > > > > > Notes : > > > > > > - RTNL is still held in "tc qdisc show", more changes are needed. > > > > > > - Qdisc returning many attributes might want/need to provide > > > a consistent set of attributes. If that is the case, their > > > dump() method could acquire the qdisc spinlock, to pair the > > > spinlock acquision in their change() method. > > > > > > > For the series: > > Reviewed-by: Jamal Hadi Salim<jhs@mojatatu.com> > > > > Not a show-stopper, we'll run the tdc tests after (and use this as an > > opportunity to add more tests if needed). > > For your next series we'll try to do that after you post. > > Hi Jamal, > > On the topic of tdc, I noticed the following both > with and without this series applied. Is this something > you are aware of? > > not ok 990 ce7d - Add mq Qdisc to multi-queue device (4 queues) > Since you said it also happens before Eric's patch, I took a look in the test and nothing seems to stand out. Which iproute2 version are you using? We are running tdc in tandem with net-next (and iproute2-next) via nipa for a while now and didn't see this problem pop up. So I am guessing something in your setup? > I'm not sure if it is valid, but I tried running tdc like this: > > $ ng --build --config tools/testing/selftests/tc-testing/config > $ vng -v --run . --user root --cpus 4 -- \ > "cd ./tools/testing/selftests/tc-testing; ./tdc.py;" This looks reasonable... cheers, jamal
On Thu, Apr 18, 2024 at 07:05:08PM -0400, Jamal Hadi Salim wrote: > On Thu, Apr 18, 2024 at 11:08 AM Simon Horman <horms@kernel.org> wrote: > > > > On Thu, Apr 18, 2024 at 06:23:27AM -0400, Jamal Hadi Salim wrote: > > > On Thu, Apr 18, 2024 at 3:32 AM Eric Dumazet <edumazet@google.com> wrote: > > > > > > > > Medium term goal is to implement "tc qdisc show" without needing > > > > to acquire RTNL. > > > > > > > > This first series makes the requested changes in 14 qdisc. > > > > > > > > Notes : > > > > > > > > - RTNL is still held in "tc qdisc show", more changes are needed. > > > > > > > > - Qdisc returning many attributes might want/need to provide > > > > a consistent set of attributes. If that is the case, their > > > > dump() method could acquire the qdisc spinlock, to pair the > > > > spinlock acquision in their change() method. > > > > > > > > > > For the series: > > > Reviewed-by: Jamal Hadi Salim<jhs@mojatatu.com> > > > > > > Not a show-stopper, we'll run the tdc tests after (and use this as an > > > opportunity to add more tests if needed). > > > For your next series we'll try to do that after you post. > > > > Hi Jamal, > > > > On the topic of tdc, I noticed the following both > > with and without this series applied. Is this something > > you are aware of? > > > > not ok 990 ce7d - Add mq Qdisc to multi-queue device (4 queues) > > > > Since you said it also happens before Eric's patch, I took a look in > the test and nothing seems to stand out. Which iproute2 version are > you using? > We are running tdc in tandem with net-next (and iproute2-next) via > nipa for a while now and didn't see this problem pop up. So I am > guessing something in your setup? Thanks Jamal, I appreciate you checking this. I agree it seems likely that it relates to my environment. And I'll try out iproute2-next. For the record I'm using the Fedora 39 packaged iproute2, iproute-6.4.0-2.fc39.x86_64. For the kernel, I was using net-next from within the past few days. > > I'm not sure if it is valid, but I tried running tdc like this: > > > > $ ng --build --config tools/testing/selftests/tc-testing/config > > $ vng -v --run . --user root --cpus 4 -- \ > > "cd ./tools/testing/selftests/tc-testing; ./tdc.py;" > > This looks reasonable... Thanks, that was my main question.
Hello: This series was applied to netdev/net-next.git (main) by David S. Miller <davem@davemloft.net>: On Thu, 18 Apr 2024 07:32:34 +0000 you wrote: > Medium term goal is to implement "tc qdisc show" without needing > to acquire RTNL. > > This first series makes the requested changes in 14 qdisc. > > Notes : > > [...] Here is the summary with links: - [v2,net-next,01/14] net_sched: sch_fq: implement lockless fq_dump() https://git.kernel.org/netdev/net-next/c/24bcc3076790 - [v2,net-next,02/14] net_sched: cake: implement lockless cake_dump() https://git.kernel.org/netdev/net-next/c/9263650102bb - [v2,net-next,03/14] net_sched: sch_cbs: implement lockless cbs_dump() https://git.kernel.org/netdev/net-next/c/8eb54a421a62 - [v2,net-next,04/14] net_sched: sch_choke: implement lockless choke_dump() https://git.kernel.org/netdev/net-next/c/7253c1d1e7a5 - [v2,net-next,05/14] net_sched: sch_codel: implement lockless codel_dump() https://git.kernel.org/netdev/net-next/c/c45bd26c829e - [v2,net-next,06/14] net_sched: sch_tfs: implement lockless etf_dump() https://git.kernel.org/netdev/net-next/c/a1ac3a7c3d1e - [v2,net-next,07/14] net_sched: sch_ets: implement lockless ets_dump() https://git.kernel.org/netdev/net-next/c/c5f1dde7f731 - [v2,net-next,08/14] net_sched: sch_fifo: implement lockless __fifo_dump() https://git.kernel.org/netdev/net-next/c/01daf66b791e - [v2,net-next,09/14] net_sched: sch_fq_codel: implement lockless fq_codel_dump() https://git.kernel.org/netdev/net-next/c/396a0038508a - [v2,net-next,10/14] net_sched: sch_fq_pie: implement lockless fq_pie_dump() https://git.kernel.org/netdev/net-next/c/13a9965de324 - [v2,net-next,11/14] net_sched: sch_hfsc: implement lockless accesses to q->defcls https://git.kernel.org/netdev/net-next/c/49e8ae537002 - [v2,net-next,12/14] net_sched: sch_hhf: implement lockless hhf_dump() https://git.kernel.org/netdev/net-next/c/293c7e2b3e2f - [v2,net-next,13/14] net_sched: sch_pie: implement lockless pie_dump() https://git.kernel.org/netdev/net-next/c/6c00dc4fdb40 - [v2,net-next,14/14] net_sched: sch_skbprio: implement lockless skbprio_dump() https://git.kernel.org/netdev/net-next/c/c85cedb38f41 You are awesome, thank you!
On Fri, Apr 19, 2024 at 3:18 AM Simon Horman <horms@kernel.org> wrote: > > On Thu, Apr 18, 2024 at 07:05:08PM -0400, Jamal Hadi Salim wrote: > > On Thu, Apr 18, 2024 at 11:08 AM Simon Horman <horms@kernel.org> wrote: > > > > > > On Thu, Apr 18, 2024 at 06:23:27AM -0400, Jamal Hadi Salim wrote: > > > > On Thu, Apr 18, 2024 at 3:32 AM Eric Dumazet <edumazet@google.com> wrote: > > > > > > > > > > Medium term goal is to implement "tc qdisc show" without needing > > > > > to acquire RTNL. > > > > > > > > > > This first series makes the requested changes in 14 qdisc. > > > > > > > > > > Notes : > > > > > > > > > > - RTNL is still held in "tc qdisc show", more changes are needed. > > > > > > > > > > - Qdisc returning many attributes might want/need to provide > > > > > a consistent set of attributes. If that is the case, their > > > > > dump() method could acquire the qdisc spinlock, to pair the > > > > > spinlock acquision in their change() method. > > > > > > > > > > > > > For the series: > > > > Reviewed-by: Jamal Hadi Salim<jhs@mojatatu.com> > > > > > > > > Not a show-stopper, we'll run the tdc tests after (and use this as an > > > > opportunity to add more tests if needed). > > > > For your next series we'll try to do that after you post. > > > > > > Hi Jamal, > > > > > > On the topic of tdc, I noticed the following both > > > with and without this series applied. Is this something > > > you are aware of? > > > > > > not ok 990 ce7d - Add mq Qdisc to multi-queue device (4 queues) > > > > > > > Since you said it also happens before Eric's patch, I took a look in > > the test and nothing seems to stand out. Which iproute2 version are > > you using? > > We are running tdc in tandem with net-next (and iproute2-next) via > > nipa for a while now and didn't see this problem pop up. So I am > > guessing something in your setup? > > Thanks Jamal, > > I appreciate you checking this. > I agree it seems likely that it relates to my environment. > And I'll try out iproute2-next. > Yeah, that would work although i think what you showed earlier should have worked with just iproute2. Actually one thing comes to mind noticing you are using tdc.py - that test uses netdevsim. You may have to modprobe netdevsim. If you run it via tdc.sh it would probe and load it for you > For the record I'm using the Fedora 39 packaged iproute2, > iproute-6.4.0-2.fc39.x86_64. > We use debian and ubuntu mostly. > For the kernel, I was using net-next from within the past few days. > Havent tested the latest/greatest net-next but say 3-4 days old net-next. > > > I'm not sure if it is valid, but I tried running tdc like this: > > > > > > $ ng --build --config tools/testing/selftests/tc-testing/config > > > $ vng -v --run . --user root --cpus 4 -- \ > > > "cd ./tools/testing/selftests/tc-testing; ./tdc.py;" > > > > This looks reasonable... > > Thanks, that was my main question. Just what i said above. cheers, jamal
On Fri, Apr 19, 2024 at 10:24:52AM -0400, Jamal Hadi Salim wrote: > On Fri, Apr 19, 2024 at 3:18 AM Simon Horman <horms@kernel.org> wrote: > > > > On Thu, Apr 18, 2024 at 07:05:08PM -0400, Jamal Hadi Salim wrote: > > > On Thu, Apr 18, 2024 at 11:08 AM Simon Horman <horms@kernel.org> wrote: > > > > > > > > On Thu, Apr 18, 2024 at 06:23:27AM -0400, Jamal Hadi Salim wrote: > > > > > On Thu, Apr 18, 2024 at 3:32 AM Eric Dumazet <edumazet@google.com> wrote: > > > > > > > > > > > > Medium term goal is to implement "tc qdisc show" without needing > > > > > > to acquire RTNL. > > > > > > > > > > > > This first series makes the requested changes in 14 qdisc. > > > > > > > > > > > > Notes : > > > > > > > > > > > > - RTNL is still held in "tc qdisc show", more changes are needed. > > > > > > > > > > > > - Qdisc returning many attributes might want/need to provide > > > > > > a consistent set of attributes. If that is the case, their > > > > > > dump() method could acquire the qdisc spinlock, to pair the > > > > > > spinlock acquision in their change() method. > > > > > > > > > > > > > > > > For the series: > > > > > Reviewed-by: Jamal Hadi Salim<jhs@mojatatu.com> > > > > > > > > > > Not a show-stopper, we'll run the tdc tests after (and use this as an > > > > > opportunity to add more tests if needed). > > > > > For your next series we'll try to do that after you post. > > > > > > > > Hi Jamal, > > > > > > > > On the topic of tdc, I noticed the following both > > > > with and without this series applied. Is this something > > > > you are aware of? > > > > > > > > not ok 990 ce7d - Add mq Qdisc to multi-queue device (4 queues) > > > > > > > > > > Since you said it also happens before Eric's patch, I took a look in > > > the test and nothing seems to stand out. Which iproute2 version are > > > you using? > > > We are running tdc in tandem with net-next (and iproute2-next) via > > > nipa for a while now and didn't see this problem pop up. So I am > > > guessing something in your setup? > > > > Thanks Jamal, > > > > I appreciate you checking this. > > I agree it seems likely that it relates to my environment. > > And I'll try out iproute2-next. > > > > Yeah, that would work although i think what you showed earlier should > have worked with just iproute2. Actually one thing comes to mind > noticing you are using tdc.py - that test uses netdevsim. You may have > to modprobe netdevsim. If you run it via tdc.sh it would probe and > load it for you Thanks, tdc.sh seems to work better, as does invoking TDC via make (which calls tdc.sh). ...