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

Thank you for the quick reply!

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

Thanks,
Xuanqiang

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