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

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

On Fri, Jan 29, 2021 at 5:11 PM Willem de Bruijn
[off-list ref] wrote:

quoted

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

quoted

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

quoted

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