Re: [PATCH net v2] r8152: avoid the driver drops a lot of packets

[Jakub Kicinski <kuba@kernel.org>]

On Thu, 7 Sep 2023 07:16:50 +0000 Hayes Wang wrote:
> > Before we tweak the heuristics let's make sure rx_bottom() behaves
> > correctly. Could you make sure that
> >  - we don't perform _any_ rx processing when budget is 0
> >    (see the NAPI documentation under Documentation/networking)  
> 
> The work_done would be 0, and napi_complete_done() wouldn't be called.
> However, skb_queue_len(&tp->rx_queue) may be increased. I think it is
> not acceptable, right?

If budget is 0 we got called by netconsole, meaning we may be holding
arbitrary locks. And we can't use napi_alloc_skb() which is for
softirq/bh context only. We should only try to complete Tx in that
case, since r8152_poll() doesn't handle any Tx the right thing seems
to be to add if (!budget) return 0;

> >  - finish the current aggregate even if budget run out, return
> >    work_done = budget in that case.
> >    With this change the rx_queue thing should be gone completely.  
> 
> Excuse me. I don't understand this part. I know that when the packets are
> more than budget, the maximum packets which could be handled is budget.
> That is, return work_done = budget. However, the extra packets would be queued
> to rx_queue. I don't understand what you mean about " the rx_queue thing
> should be gone completely". I think the current driver would return
> work_done = budget, and queue the other packets. I don't sure what you
> want me to change.

Nothing will explode if we process a few more packets than budget
(assuming budget > 0). If we already do allocations and prepare
those skbs - there's no point holding onto them in the driver.
Just sent them up the stack (and then we won't need the local rx_queue).

> >  - instead of copying the head use napi_get_frags() + napi_gro_frags()
> >    it gives you an skb, you just attach the page to it as a frag and
> >    hand it back to GRO. This makes sure you never pull data into head
> >    rather than just headers.  
> 
> I would study about them. Thanks.
> 
> Should I include above changes for this patch?
> I think I have to submit another patches for above.
> 
> > Please share the performance results with those changes.  
> 
> I couldn't reproduce the problem, so I couldn't provide the result
> with the differences.

Hm, if you can't repro my intuition would be to only take the patch for
budget=0 handling into net, and the rest as improvements into net-next.