Re: [PATCH net-next v1] net: gro_cells: move backlog drop handling outside the local lock

[Xuanqiang Luo <xuanqiang.luo@linux.dev>]

在 2026/9/15 20:26, Eric Dumazet 写道:
> On Tue, Sep 15, 2026 at 5:16 AM Xuanqiang Luo <xuanqiang.luo@linux.dev> wrote:
>>
>> From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
>>
>> When the GRO cell queue length exceeds max_backlog, gro_cells_receive()
>> holds bh_lock while updating the RX drop counter and freeing the rejected
>> skb. The skb has not been queued, and the counter is updated with
>> this_cpu_inc(), so neither operation requires the queue lock.
>>
>> Release bh_lock before accounting for and freeing the dropped skb to
>> shorten the critical section.
> 
> local_lock_nested_bh() is only a real lock on PREEMPT_RT. On other
> kernels it is a lockdep assertion, so this patch generates the same
> code. And this is the backlog overflow path: by definition we are
> already dropping packets there. Shortening this "critical section"
> buys nothing measurable, and there is no benchmark in the changelog.
> 
> Also, the result is harder to read than what it replaces: a label
> named "unlock" that actually drops the skb, falling through into
> "drop:", and a backward goto past the return. Three labels for a
> 20-line function.
> 
> If you want to improve gro_cells_receive(), please add drop reasons
> instead: SKB_DROP_REASON_DEV_READY for the !IFF_UP case and
> SKB_DROP_REASON_CPU_BACKLOG for the overflow one. That actually helps
> people tracking down drops.
> 

Thank you for the quick reply!

I see your point. I'll take another look.

Thanks,
Xuanqiang

> 
>>
>> Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
>> ---
>>   net/core/gro_cells.c | 24 ++++++++++++------------
>>   1 file changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/net/core/gro_cells.c b/net/core/gro_cells.c
>> index d8c0a28671201..f46cecd246aab 100644
>> --- a/net/core/gro_cells.c
>> +++ b/net/core/gro_cells.c
>> @@ -14,7 +14,6 @@ struct gro_cell {
>>   int gro_cells_receive(struct gro_cells *gcells, struct sk_buff *skb)
>>   {
>>          struct net_device *dev = skb->dev;
>> -       bool have_bh_lock = false;
>>          struct gro_cell *cell;
>>          int res;
>>
>> @@ -26,32 +25,33 @@ int gro_cells_receive(struct gro_cells *gcells, struct sk_buff *skb)
>>
>>          if (!gcells->cells || skb_cloned(skb) || netif_elide_gro(dev)) {
>>                  res = netif_rx(skb);
>> -               goto unlock;
>> +               goto out_rcu;
>>          }
>>
>>          local_lock_nested_bh(&gcells->cells->bh_lock);
>> -       have_bh_lock = true;
>>          cell = this_cpu_ptr(gcells->cells);
>>
>> -       if (skb_queue_len(&cell->napi_skbs) > READ_ONCE(net_hotdata.max_backlog)) {
>> -drop:
>> -               dev_core_stats_rx_dropped_inc(dev);
>> -               kfree_skb(skb);
>> -               res = NET_RX_DROP;
>> +       if (skb_queue_len(&cell->napi_skbs) > READ_ONCE(net_hotdata.max_backlog))
>>                  goto unlock;
>> -       }
>>
>>          __skb_queue_tail(&cell->napi_skbs, skb);
>>          if (skb_queue_len(&cell->napi_skbs) == 1)
>>                  napi_schedule(&cell->napi);
>>
>> +       local_unlock_nested_bh(&gcells->cells->bh_lock);
>>          res = NET_RX_SUCCESS;
>>
>> -unlock:
>> -       if (have_bh_lock)
>> -               local_unlock_nested_bh(&gcells->cells->bh_lock);
>> +out_rcu:
>>          rcu_read_unlock();
>>          return res;
>> +
>> +unlock:
>> +       local_unlock_nested_bh(&gcells->cells->bh_lock);
>> +drop:
>> +       dev_core_stats_rx_dropped_inc(dev);
>> +       kfree_skb(skb);
>> +       res = NET_RX_DROP;
>> +       goto out_rcu;
>>   }
>>   EXPORT_SYMBOL(gro_cells_receive);
>>
>>
>> base-commit: 1142eb185b05db61a78130890fc4ed268f4cb4e6
>> --
>> 2.43.0
>>