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

[Willem de Bruijn]

On Fri, Jan 29, 2021 at 6:03 PM Sven Van Asbroeck <thesven73@xxxxxxxxx> wrote:
>
> 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@xxxxxxxxx> 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.

Ah. I had missed the temporary state of this until the buffers are
reinitialized. Yes, then there is no reason to worry. Same for the
frag_list vs frags comment I made.

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

Okay. I found it a bit hard to parse how much true code change was
mixed in with just reindenting existing code. If a lot, then no need
to split of the code refactor.

>
> > > +
> > > +       /* 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.