| Message ID | 20220202122848.647635-2-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dev: PREEMPT_RT fixups. | expand |
On Wed, Feb 2, 2022 at 4:28 AM Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote: > > The preempt_disable() and rcu_disable() section was introduced in commit > bbbe211c295ff ("net: rcu lock and preempt disable missing around generic xdp") > > The backtrace shows that bottom halves were disabled and so the usage of > smp_processor_id() would not trigger a warning. > The "suspicious RCU usage" warning was triggered because > rcu_dereference() was not used in rcu_read_lock() section (only > rcu_read_lock_bh()). A rcu_read_lock() is sufficient. > > Remove the preempt_disable() statement which is not needed. I am confused by this changelog/analysis of yours. According to git blame, you are reverting this patch. commit cece1945bffcf1a823cdfa36669beae118419351 Author: Changli Gao <xiaosuo@gmail.com> Date: Sat Aug 7 20:35:43 2010 -0700 net: disable preemption before call smp_processor_id() Although netif_rx() isn't expected to be called in process context with preemption enabled, it'd better handle this case. And this is why get_cpu() is used in the non-RPS #ifdef branch. If tree RCU is selected, rcu_read_lock() won't disable preemption, so preempt_disable() should be called explictly. Signed-off-by: Changli Gao <xiaosuo@gmail.com> Signed-off-by: David S. Miller <davem@davemloft.net> But I am not sure we can. Here is the code in larger context: #ifdef CONFIG_RPS if (static_branch_unlikely(&rps_needed)) { struct rps_dev_flow voidflow, *rflow = &voidflow; int cpu; preempt_disable(); rcu_read_lock(); cpu = get_rps_cpu(skb->dev, skb, &rflow); if (cpu < 0) cpu = smp_processor_id(); ret = enqueue_to_backlog(skb, cpu, &rflow->last_qtail); rcu_read_unlock(); preempt_enable(); } else #endif This code needs the preempt_disable(). > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > --- > net/core/dev.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/net/core/dev.c b/net/core/dev.c > index 1baab07820f65..325b70074f4ae 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -4796,7 +4796,6 @@ static int netif_rx_internal(struct sk_buff *skb) > struct rps_dev_flow voidflow, *rflow = &voidflow; > int cpu; > > - preempt_disable(); > rcu_read_lock(); > > cpu = get_rps_cpu(skb->dev, skb, &rflow); > @@ -4806,7 +4805,6 @@ static int netif_rx_internal(struct sk_buff *skb) > ret = enqueue_to_backlog(skb, cpu, &rflow->last_qtail); > > rcu_read_unlock(); > - preempt_enable(); > } else > #endif > { > -- > 2.34.1 >
Eric Dumazet <edumazet@google.com> writes: > On Wed, Feb 2, 2022 at 4:28 AM Sebastian Andrzej Siewior > <bigeasy@linutronix.de> wrote: >> >> The preempt_disable() and rcu_disable() section was introduced in commit >> bbbe211c295ff ("net: rcu lock and preempt disable missing around generic xdp") >> >> The backtrace shows that bottom halves were disabled and so the usage of >> smp_processor_id() would not trigger a warning. >> The "suspicious RCU usage" warning was triggered because >> rcu_dereference() was not used in rcu_read_lock() section (only >> rcu_read_lock_bh()). A rcu_read_lock() is sufficient. >> >> Remove the preempt_disable() statement which is not needed. > > I am confused by this changelog/analysis of yours. > > According to git blame, you are reverting this patch. > > commit cece1945bffcf1a823cdfa36669beae118419351 > Author: Changli Gao <xiaosuo@gmail.com> > Date: Sat Aug 7 20:35:43 2010 -0700 > > net: disable preemption before call smp_processor_id() > > Although netif_rx() isn't expected to be called in process context with > preemption enabled, it'd better handle this case. And this is why get_cpu() > is used in the non-RPS #ifdef branch. If tree RCU is selected, > rcu_read_lock() won't disable preemption, so preempt_disable() should be > called explictly. > > Signed-off-by: Changli Gao <xiaosuo@gmail.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > > > But I am not sure we can. > > Here is the code in larger context: > > #ifdef CONFIG_RPS > if (static_branch_unlikely(&rps_needed)) { > struct rps_dev_flow voidflow, *rflow = &voidflow; > int cpu; > > preempt_disable(); > rcu_read_lock(); > > cpu = get_rps_cpu(skb->dev, skb, &rflow); > if (cpu < 0) > cpu = smp_processor_id(); > > ret = enqueue_to_backlog(skb, cpu, &rflow->last_qtail); > > rcu_read_unlock(); > preempt_enable(); > } else > #endif > > This code needs the preempt_disable(). This is mostly so that the CPU ID stays the same throughout that section of code, though, right? So wouldn't it work to replace the preempt_disable() with a migrate_disable()? That should keep _RT happy, no? -Toke
On 2022-02-02 09:10:10 [-0800], Eric Dumazet wrote: > On Wed, Feb 2, 2022 at 4:28 AM Sebastian Andrzej Siewior > <bigeasy@linutronix.de> wrote: > > > > The preempt_disable() and rcu_disable() section was introduced in commit > > bbbe211c295ff ("net: rcu lock and preempt disable missing around generic xdp") > > > > The backtrace shows that bottom halves were disabled and so the usage of > > smp_processor_id() would not trigger a warning. > > The "suspicious RCU usage" warning was triggered because > > rcu_dereference() was not used in rcu_read_lock() section (only > > rcu_read_lock_bh()). A rcu_read_lock() is sufficient. > > > > Remove the preempt_disable() statement which is not needed. > > I am confused by this changelog/analysis of yours. > > According to git blame, you are reverting this patch. > > commit cece1945bffcf1a823cdfa36669beae118419351 > Author: Changli Gao <xiaosuo@gmail.com> > Date: Sat Aug 7 20:35:43 2010 -0700 > > net: disable preemption before call smp_processor_id() > > Although netif_rx() isn't expected to be called in process context with > preemption enabled, it'd better handle this case. And this is why get_cpu() > is used in the non-RPS #ifdef branch. If tree RCU is selected, > rcu_read_lock() won't disable preemption, so preempt_disable() should be > called explictly. > > Signed-off-by: Changli Gao <xiaosuo@gmail.com> > Signed-off-by: David S. Miller <davem@davemloft.net> Nut sure if I ignored it or made a wrong turn somewhere. But I remember reading it. But here, preempt_disable() was added because | Although netif_rx() isn't expected to be called in process context with | preemption enabled, it'd better handle this case. and this isn't much of a good reason. Simply because netif_rx() shouldn't not be called from preemptible context. > But I am not sure we can. > > Here is the code in larger context: > > #ifdef CONFIG_RPS > if (static_branch_unlikely(&rps_needed)) { > struct rps_dev_flow voidflow, *rflow = &voidflow; > int cpu; > > preempt_disable(); > rcu_read_lock(); > > cpu = get_rps_cpu(skb->dev, skb, &rflow); > if (cpu < 0) > cpu = smp_processor_id(); > > ret = enqueue_to_backlog(skb, cpu, &rflow->last_qtail); > > rcu_read_unlock(); > preempt_enable(); > } else > #endif > > This code needs the preempt_disable(). But why? netif_rx_internal() should be invoked with disabled BH so I don't see a reason why preemption needs additionally be disabled in this section. On PREEMPT_RT we can get preempted but the task remains on the CPU and other network activity will be block on the BH-lock. Sebastian
On 2022-02-03 13:00:06 [+0100], Toke Høiland-Jørgensen wrote: > > Here is the code in larger context: > > > > #ifdef CONFIG_RPS > > if (static_branch_unlikely(&rps_needed)) { > > struct rps_dev_flow voidflow, *rflow = &voidflow; > > int cpu; > > > > preempt_disable(); > > rcu_read_lock(); > > > > cpu = get_rps_cpu(skb->dev, skb, &rflow); > > if (cpu < 0) > > cpu = smp_processor_id(); > > > > ret = enqueue_to_backlog(skb, cpu, &rflow->last_qtail); > > > > rcu_read_unlock(); > > preempt_enable(); > > } else > > #endif > > > > This code needs the preempt_disable(). > > This is mostly so that the CPU ID stays the same throughout that section > of code, though, right? So wouldn't it work to replace the > preempt_disable() with a migrate_disable()? That should keep _RT happy, > no? It would but as mentioned previously: BH is disabled and smp_processor_id() is stable. > -Toke Sebastian
Sebastian Andrzej Siewior <bigeasy@linutronix.de> writes: > On 2022-02-03 13:00:06 [+0100], Toke Høiland-Jørgensen wrote: >> > Here is the code in larger context: >> > >> > #ifdef CONFIG_RPS >> > if (static_branch_unlikely(&rps_needed)) { >> > struct rps_dev_flow voidflow, *rflow = &voidflow; >> > int cpu; >> > >> > preempt_disable(); >> > rcu_read_lock(); >> > >> > cpu = get_rps_cpu(skb->dev, skb, &rflow); >> > if (cpu < 0) >> > cpu = smp_processor_id(); >> > >> > ret = enqueue_to_backlog(skb, cpu, &rflow->last_qtail); >> > >> > rcu_read_unlock(); >> > preempt_enable(); >> > } else >> > #endif >> > >> > This code needs the preempt_disable(). >> >> This is mostly so that the CPU ID stays the same throughout that section >> of code, though, right? So wouldn't it work to replace the >> preempt_disable() with a migrate_disable()? That should keep _RT happy, >> no? > > It would but as mentioned previously: BH is disabled and > smp_processor_id() is stable. Ah, right, because of the change in loopback to use netif_rx_ni()? But that bit of the analysis only comes later in your series, so at the very least you should be explaining this in the commit message here. Or you could potentially squash patches 1 and 2 and do both changes at once, since it's changing two bits of the same function and both need the same analysis... However, if we're going with Eric's suggestion of an internal __netif_rx() for loopback that *doesn't* do local_bh_disable() then this code would end up being called without BH disable, so we'd need the migrate_disable() anyway, no? -Toke
On 2022-02-03 13:41:13 [+0100], Toke Høiland-Jørgensen wrote: > Sebastian Andrzej Siewior <bigeasy@linutronix.de> writes: > > > It would but as mentioned previously: BH is disabled and > > smp_processor_id() is stable. > > Ah, right, because of the change in loopback to use netif_rx_ni()? But > that bit of the analysis only comes later in your series, so at the very > least you should be explaining this in the commit message here. Or you > could potentially squash patches 1 and 2 and do both changes at once, > since it's changing two bits of the same function and both need the same > analysis... > > However, if we're going with Eric's suggestion of an internal > __netif_rx() for loopback that *doesn't* do local_bh_disable() then this > code would end up being called without BH disable, so we'd need the > migrate_disable() anyway, no? Eric suggested to the __netif_rx() for loopback which is already in BH-disabled section. So if that is the only "allowed" caller, we wouldn't have to worry. If __netif_rx() becomes more users and one calls it from preemptible context then we have a problem (like netif_rx() vs netif_rx_ni()). migrate_disable() will shut up smp_processor_id(), yes, but we need something to process pending softirqs. Otherwise they are delayed until the next IRQ, spin_unlock_bh(), etc. > -Toke Sebastian
diff --git a/net/core/dev.c b/net/core/dev.c index 1baab07820f65..325b70074f4ae 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -4796,7 +4796,6 @@ static int netif_rx_internal(struct sk_buff *skb) struct rps_dev_flow voidflow, *rflow = &voidflow; int cpu; - preempt_disable(); rcu_read_lock(); cpu = get_rps_cpu(skb->dev, skb, &rflow); @@ -4806,7 +4805,6 @@ static int netif_rx_internal(struct sk_buff *skb) ret = enqueue_to_backlog(skb, cpu, &rflow->last_qtail); rcu_read_unlock(); - preempt_enable(); } else #endif {
The preempt_disable() and rcu_disable() section was introduced in commit bbbe211c295ff ("net: rcu lock and preempt disable missing around generic xdp") The backtrace shows that bottom halves were disabled and so the usage of smp_processor_id() would not trigger a warning. The "suspicious RCU usage" warning was triggered because rcu_dereference() was not used in rcu_read_lock() section (only rcu_read_lock_bh()). A rcu_read_lock() is sufficient. Remove the preempt_disable() statement which is not needed. Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- net/core/dev.c | 2 -- 1 file changed, 2 deletions(-)