Re: [PATCH v2 net-next 14/14] mlx4: remove duplicate code in mlx4_en_process_rx_cq()

On Thu, Feb 9, 2017 at 9:15 AM, Saeed Mahameed
[off-list ref] wrote:

On Thu, Feb 9, 2017 at 3:58 PM, Eric Dumazet [off-list ref] wrote:

quoted

We should keep one way to build skbs, regardless of GRO being on or off.

Note that I made sure to defer as much as possible the point we need to
pull data from the frame, so that future prefetch() we might add
are more effective.

These skb attributes derive from the CQE or ring :
 ip_summed, csum
 hash
 vlan offload
 hwtstamps
 queue_mapping

As a bonus, this patch removes mlx4 dependency on eth_get_headlen()

So no copy at all in driver RX ?  as it is handled in
napi_gro_frags(&cq->napi); ?

Absolutely.

General question, why not just use build_skb() ? which approach is better ?

napi_gro_frags() is better.

Especially if you want MTU = 1800 or so.

But also because the page is meant to be read only, when using
page-recycling technique.

quoted

which is very often broken enough to give us headaches.

                 }

Why just not move this whole csum logic to a dedicated function ?
all it needs to do is update some csum statistics and determine
"ip_summed" value;

I guess we could.

Not required anymore ?  I know napi_frags_skb does the job,
but what about skb->pkt_type field which is handled in eth_type_trans ?

Look at napi_frags_skb()

quoted

+               nr = mlx4_en_complete_rx_desc(priv, frags, skb, length);
+               if (nr) {

likely() ?

quoted

+                       skb_shinfo(skb)->nr_frags = nr;
+                       skb->len = length;
+                       skb->data_len = length;
+                       napi_gro_frags(&cq->napi);
                }

no need to kfree_skb/reset some SKB fields in case nr == 0 ?
since this SKB will be returned via napi_get_frags if you didn't
actually consume it ..

for example 1st packet is a valn packet and for some reason
mlx4_en_complete_rx_desc returned 0 on it.
next packet is a non vlan packet that will get the same skb of the 1st
packet with non clean SKB fields such as (skb->vlan_tci) !

Good points. For one reason I mixed with napi_reuse_skb().

Note that I do not believe this path can ever be taken anyway.

By definition, we allow the NIC to use one RX ring slot if and only if
all the page fragmenst were properly allocated.