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

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

[Xuanqiang Luo]

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.

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

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

[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.

Thanks.

pw-bot: cr

>
> 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
>

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

[Xuanqiang Luo]

在 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
>>