Re: [PATCH net-next v1 2/6] lan743x: support rx multi-buffer packets

[Sven Van Asbroeck <thesven73@gmail.com>]

Hoi Willem, thanks a lot for reviewing this patch, much appreciated !!

On Fri, Jan 29, 2021 at 5:11 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> > +static struct sk_buff *
> > +lan743x_rx_trim_skb(struct sk_buff *skb, int frame_length)
> > +{
> > +       if (skb_linearize(skb)) {
>
> Is this needed? That will be quite expensive

The skb will only be non-linear when it's created from a multi-buffer frame.
Multi-buffer frames are only generated right after a mtu change - fewer than
32 frames will be non-linear after an mtu increase. So as long as people don't
change the mtu in a tight loop, skb_linearize is just a single comparison,
99.999999+% of the time.

>
> Is it possible to avoid the large indentation change, or else do that
> in a separate patch? It makes it harder to follow the functional
> change.

It's not immediately obvious, but I have replaced the whole function
with slightly different logic, and the replacement content has a much
flatter indentation structure, and should be easier to follow.

Or perhaps I am misinterpreting your question?

> > +
> > +       /* add buffers to skb via skb->frag_list */
> > +       if (is_first) {
> > +               skb_reserve(skb, RX_HEAD_PADDING);
> > +               skb_put(skb, buffer_length - RX_HEAD_PADDING);
> > +               if (rx->skb_head)
> > +                       dev_kfree_skb_irq(rx->skb_head);
> > +               rx->skb_head = skb;
> > +       } else if (rx->skb_head) {
> > +               skb_put(skb, buffer_length);
> > +               if (skb_shinfo(rx->skb_head)->frag_list)
> > +                       rx->skb_tail->next = skb;
> > +               else
> > +                       skb_shinfo(rx->skb_head)->frag_list = skb;
>
> Instead of chaining skbs into frag_list, you could perhaps delay skb
> alloc until after reception, allocate buffers stand-alone, and link
> them into the skb as skb_frags? That might avoid a few skb alloc +
> frees. Though a bit change, not sure how feasible.

The problem here is this (copypasta from somewhere else in this patch):

/* Only the last buffer in a multi-buffer frame contains the total frame
* length. All other buffers have a zero frame length. The chip
* occasionally sends more buffers than strictly required to reach the
* total frame length.
* Handle this by adding all buffers to the skb in their entirety.
* Once the real frame length is known, trim the skb.
*/

In other words, the chip sometimes sends more buffers than strictly needed to
fit the frame. linearize + trim deals with this thorny issue perfectly.

If the skb weren't linearized, we would run into trouble when trying to trim
(remove from the end) a chunk bigger than the last skb fragment.

> > +process_extension:
> > +       if (extension_index >= 0) {
> > +               u32 ts_sec;
> > +               u32 ts_nsec;
> > +
> > +               ts_sec = le32_to_cpu(desc_ext->data1);
> > +               ts_nsec = (le32_to_cpu(desc_ext->data2) &
> > +                         RX_DESC_DATA2_TS_NS_MASK_);
> > +               if (rx->skb_head) {
> > +                       hwtstamps = skb_hwtstamps(rx->skb_head);
> > +                       if (hwtstamps)
>
> This is always true.
>
> You can just call skb_hwtstamps(skb)->hwtstamp = ktime_set(ts_sec, ts_nsec);

Thank you, will do !

>
> Though I see that this is existing code just moved due to
> aforementioned indentation change.

True, but I can make the change anyway.