Re: [PATCH net-next 1/4] net: dev: Remove the preempt_disable() in netif_rx_internal().

["Toke Høiland-Jørgensen" <toke@toke.dk>]

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