[xdp-hints] Re: [PATCH bpf-next V1 2/5] igc: add igc_xdp_buff wrapper for xdp_buff in driver

[Jesper Dangaard Brouer <jbrouer@redhat.com>]

On 18/04/2023 06.34, Song, Yoong Siang wrote:
> On Monday, April 17, 2023 10:57 PM, Jesper Dangaard Brouer <brouer@redhat.com> wrote:
>> Driver specific metadata data for XDP-hints kfuncs are propagated via tail
>> extending the struct xdp_buff with a locally scoped driver struct.
>>
>> Zero-Copy AF_XDP/XSK does similar tricks via struct xdp_buff_xsk. This
>> xdp_buff_xsk struct contains a CB area (24 bytes) that can be used for extending
>> the locally scoped driver into. The XSK_CHECK_PRIV_TYPE define catch size
>> violations build time.
>>
> 
> Since the main purpose of this patch is to introduce igc_xdp_buff, and
> you have another two patches for timestamp and hash,
> thus, suggest to move timestamp and hash related code into respective patches.
> 
>> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
>> ---
>> drivers/net/ethernet/intel/igc/igc.h      |    6 ++++++
>> drivers/net/ethernet/intel/igc/igc_main.c |   30 ++++++++++++++++++++++-------
>> 2 files changed, 29 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igc/igc.h
>> b/drivers/net/ethernet/intel/igc/igc.h
>> index f7f9e217e7b4..c609a2e648f8 100644
>> --- a/drivers/net/ethernet/intel/igc/igc.h
>> +++ b/drivers/net/ethernet/intel/igc/igc.h
>> @@ -499,6 +499,12 @@ struct igc_rx_buffer {
>> 	};
>> };
>>
>> +/* context wrapper around xdp_buff to provide access to descriptor
>> +metadata */ struct igc_xdp_buff {
>> +	struct xdp_buff xdp;
>> +	union igc_adv_rx_desc *rx_desc;
> 
> Move rx_desc to 4th patch (Rx hash patch)
> 

Hmm, rx_desc is also needed by 3rd patch (Rx timestamp), so that would 
break...

I can reorder patches, and have "Rx hash patch" come before "Rx 
timestamp" patch.


>> +};
>> +
>> struct igc_q_vector {
>> 	struct igc_adapter *adapter;    /* backlink */
>> 	void __iomem *itr_register;
>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c
>> b/drivers/net/ethernet/intel/igc/igc_main.c
>> index bfa9768d447f..3a844cf5be3f 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_main.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
>> @@ -2236,6 +2236,8 @@ static bool igc_alloc_rx_buffers_zc(struct igc_ring
>> *ring, u16 count)
>> 	if (!count)
>> 		return ok;
>>
>> +	XSK_CHECK_PRIV_TYPE(struct igc_xdp_buff);
>> +
>> 	desc = IGC_RX_DESC(ring, i);
>> 	bi = &ring->rx_buffer_info[i];
>> 	i -= ring->count;
>> @@ -2520,8 +2522,8 @@ static int igc_clean_rx_irq(struct igc_q_vector
>> *q_vector, const int budget)
>> 		union igc_adv_rx_desc *rx_desc;
>> 		struct igc_rx_buffer *rx_buffer;
>> 		unsigned int size, truesize;
>> +		struct igc_xdp_buff ctx;
>> 		ktime_t timestamp = 0;
>> -		struct xdp_buff xdp;
>> 		int pkt_offset = 0;
>> 		void *pktbuf;
>>
>> @@ -2555,13 +2557,14 @@ static int igc_clean_rx_irq(struct igc_q_vector
>> *q_vector, const int budget)
>> 		}
>>
>> 		if (!skb) {
>> -			xdp_init_buff(&xdp, truesize, &rx_ring->xdp_rxq);
>> -			xdp_prepare_buff(&xdp, pktbuf - igc_rx_offset(rx_ring),
>> +			xdp_init_buff(&ctx.xdp, truesize, &rx_ring->xdp_rxq);
>> +			xdp_prepare_buff(&ctx.xdp, pktbuf - igc_rx_offset(rx_ring),
>> 					 igc_rx_offset(rx_ring) + pkt_offset,
>> 					 size, true);
>> -			xdp_buff_clear_frags_flag(&xdp);
>> +			xdp_buff_clear_frags_flag(&ctx.xdp);
>> +			ctx.rx_desc = rx_desc;
> 
> Move rx_desc to 4th patch (Rx hash patch)

Again would break 3rd patch.

> 
>>
>> -			skb = igc_xdp_run_prog(adapter, &xdp);
>> +			skb = igc_xdp_run_prog(adapter, &ctx.xdp);
>> 		}
>>
>> 		if (IS_ERR(skb)) {
>> @@ -2583,9 +2586,9 @@ static int igc_clean_rx_irq(struct igc_q_vector
>> *q_vector, const int budget)
>> 		} else if (skb)
>> 			igc_add_rx_frag(rx_ring, rx_buffer, skb, size);
>> 		else if (ring_uses_build_skb(rx_ring))
>> -			skb = igc_build_skb(rx_ring, rx_buffer, &xdp);
>> +			skb = igc_build_skb(rx_ring, rx_buffer, &ctx.xdp);
>> 		else
>> -			skb = igc_construct_skb(rx_ring, rx_buffer, &xdp,
>> +			skb = igc_construct_skb(rx_ring, rx_buffer, &ctx.xdp,
>> 						timestamp);
>>
>> 		/* exit if we failed to retrieve a buffer */ @@ -2686,6 +2689,15
>> @@ static void igc_dispatch_skb_zc(struct igc_q_vector *q_vector,
>> 	napi_gro_receive(&q_vector->napi, skb);  }
>>
>> +static struct igc_xdp_buff *xsk_buff_to_igc_ctx(struct xdp_buff *xdp) {
>> +	/* xdp_buff pointer used by ZC code path is alloc as xdp_buff_xsk. The
>> +	 * igc_xdp_buff shares its layout with xdp_buff_xsk and private
>> +	 * igc_xdp_buff fields fall into xdp_buff_xsk->cb
>> +	 */
>> +       return (struct igc_xdp_buff *)xdp; }
>> +
> 
> Move xsk_buff_to_igc_ctx to 3th patch (timestamp patch), which is first patch
> adding xdp_metadata_ops support to igc.
> 

Hmm, maybe, but that make the "wrapper" patch incomplete and then it
gets "completed" in the first patch that adds a xdp_metadata_ops.

>> static int igc_clean_rx_irq_zc(struct igc_q_vector *q_vector, const int budget)  {
>> 	struct igc_adapter *adapter = q_vector->adapter; @@ -2704,6 +2716,7
>> @@ static int igc_clean_rx_irq_zc(struct igc_q_vector *q_vector, const int
>> budget)
>> 	while (likely(total_packets < budget)) {
>> 		union igc_adv_rx_desc *desc;
>> 		struct igc_rx_buffer *bi;
>> +		struct igc_xdp_buff *ctx;
>> 		ktime_t timestamp = 0;
>> 		unsigned int size;
>> 		int res;
>> @@ -2721,6 +2734,9 @@ static int igc_clean_rx_irq_zc(struct igc_q_vector
>> *q_vector, const int budget)
>>
>> 		bi = &ring->rx_buffer_info[ntc];
>>
>> +		ctx = xsk_buff_to_igc_ctx(bi->xdp);
> 
> Move xsk_buff_to_igc_ctx to 3th patch (timestamp patch), which is first patch
> adding xdp_metadata_ops support to igc.
>
Sure, but it feels wrong to no "complete" the wrapper work in the
wrapper patch.

>> +		ctx->rx_desc = desc;
> 
> Move rx_desc to 4th patch (Rx hash patch)
> 

I'll reorder patch 3 and 4, else it doesn't make any sense to gradually
introduce the members in wrapper struct igc_xdp_buff.

--Jesper