Hoi Willem, thanks a lot for reviewing this patch, much appreciated !!
On Fri, Jan 29, 2021 at 5:11 PM Willem de Bruijn
<willemdebruijn.ker...@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.
