Re: [PATCH net-next v1 1/3] net: stmmac: Switch to zero-copy in non-XDP RX path

[Alexander Lobakin <aleksander.lobakin@intel.com>]

From: Yanteng Si <si.yanteng@linux.dev>
Date: Mon, 13 Jan 2025 17:41:41 +0800

> 在 2025/1/10 17:53, Furong Xu 写道:
>> Avoid memcpy in non-XDP RX path by marking all allocated SKBs to
>> be recycled in the upper network stack.
>>
>> This patch brings ~11.5% driver performance improvement in a TCP RX
>> throughput test with iPerf tool on a single isolated Cortex-A65 CPU
>> core, from 2.18 Gbits/sec increased to 2.43 Gbits/sec.
>>
>> Signed-off-by: Furong Xu <0x1207@gmail.com>
>> ---
>>   drivers/net/ethernet/stmicro/stmmac/stmmac.h  |  1 +
>>   .../net/ethernet/stmicro/stmmac/stmmac_main.c | 23 +++++++++++--------
>>   2 files changed, 15 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/
>> net/ethernet/stmicro/stmmac/stmmac.h
>> index 548b28fed9b6..5c39292313de 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
>> @@ -126,6 +126,7 @@ struct stmmac_rx_queue {
>>       unsigned int cur_rx;
>>       unsigned int dirty_rx;
>>       unsigned int buf_alloc_num;
>> +    unsigned int napi_skb_frag_size;
>>       dma_addr_t dma_rx_phy;
>>       u32 rx_tail_addr;
>>       unsigned int state_saved;
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/
>> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> index 038df1b2bb58..43125a6f8f6b 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> @@ -1320,7 +1320,7 @@ static unsigned int stmmac_rx_offset(struct
>> stmmac_priv *priv)
>>       if (stmmac_xdp_is_enabled(priv))
>>           return XDP_PACKET_HEADROOM;
>>   -    return 0;
>> +    return NET_SKB_PAD;
>>   }
>>     static int stmmac_set_bfsize(int mtu, int bufsize)
>> @@ -2019,17 +2019,21 @@ static int
>> __alloc_dma_rx_desc_resources(struct stmmac_priv *priv,
>>       struct stmmac_channel *ch = &priv->channel[queue];
>>       bool xdp_prog = stmmac_xdp_is_enabled(priv);
>>       struct page_pool_params pp_params = { 0 };
>> -    unsigned int num_pages;
>> +    unsigned int dma_buf_sz_pad, num_pages;
>>       unsigned int napi_id;
>>       int ret;
>>   +    dma_buf_sz_pad = stmmac_rx_offset(priv) + dma_conf->dma_buf_sz +
>> +             SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
>> +    num_pages = DIV_ROUND_UP(dma_buf_sz_pad, PAGE_SIZE);
>> +
>>       rx_q->queue_index = queue;
>>       rx_q->priv_data = priv;
>> +    rx_q->napi_skb_frag_size = num_pages * PAGE_SIZE;
>>         pp_params.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV;
>>       pp_params.pool_size = dma_conf->dma_rx_size;
>> -    num_pages = DIV_ROUND_UP(dma_conf->dma_buf_sz, PAGE_SIZE);
>> -    pp_params.order = ilog2(num_pages);
>> +    pp_params.order = order_base_2(num_pages);
>>       pp_params.nid = dev_to_node(priv->device);
>>       pp_params.dev = priv->device;
>>       pp_params.dma_dir = xdp_prog ? DMA_BIDIRECTIONAL : DMA_FROM_DEVICE;
>> @@ -5574,19 +5578,20 @@ static int stmmac_rx(struct stmmac_priv *priv,
>> int limit, u32 queue)
>>               /* XDP program may expand or reduce tail */
>>               buf1_len = ctx.xdp.data_end - ctx.xdp.data;
>>   -            skb = napi_alloc_skb(&ch->rx_napi, buf1_len);
>> +            skb = napi_build_skb(page_address(buf->page),
>> +                         rx_q->napi_skb_frag_size);
>>               if (!skb) {
>> +                page_pool_recycle_direct(rx_q->page_pool,
>> +                             buf->page);
>>                   rx_dropped++;
>>                   count++;
>>                   goto drain_data;
>>               }
>>                 /* XDP program may adjust header */
>> -            skb_copy_to_linear_data(skb, ctx.xdp.data, buf1_len);
>> +            skb_reserve(skb, ctx.xdp.data - ctx.xdp.data_hard_start);
> The network subsystem still requires that the length
> of each line of code should not exceed 80 characters.
> So let's silence the warning:
> 
> WARNING: line length of 81 exceeds 80 columns
> #87: FILE: drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:5592:
> +            skb_reserve(skb, ctx.xdp.data - ctx.xdp.data_hard_start);

I agree that &ctx.xdp could be made an onstack pointer to shorten these
lines, but please don't spam with the checkpatch output.

1. It's author's responsibility to read netdev CI output on Patchwork,
   reviewers shouldn't copy its logs.
2. The only alternative without making a shortcut for &ctx.xdp is

			skb_reserve(skb,
				    ctx.xdp.data - ctx.xdp.data_hard_start);

This looks really ugly and does more harm than good.

If you really want to help, pls come with good propositions how to avoid
such warnings in an elegant way.

> 
> Thanks,
> Yanteng

Thanks,
Olek