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@xxxxxxxxx> wrote:
>
> From: Xuanqiang Luo <luoxuanqiang@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
>  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
>