Re: [PATCH RESEND bpf-next 05/15] ice: Introduce ice_xdp_buff

[Alexander Lobakin]

From: Larysa Zaremba <larysa.zaremba@xxxxxxxxx>
Date: Tue, 23 May 2023 10:02:42 +0200

> On Mon, May 22, 2023 at 06:46:40PM +0200, Alexander Lobakin wrote:
>> From: Larysa Zaremba <larysa.zaremba@xxxxxxxxx>
>> Date: Fri, 12 May 2023 17:25:57 +0200
>>
>>> In order to use XDP hints via kfuncs we need to put
>>> RX descriptor and ring pointers just next to xdp_buff.
>>> Same as in hints implementations in other drivers, we archieve

                                                          ^^^^^^^^
                                                          achieve

I missed this one initially :D

>>> this through putting xdp_buff into a child structure.
>>>
>>> Currently, xdp_buff is stored in the ring structure,
>>> so replace it with union that includes child structure.
>>> This way enough memory is available while existing XDP code
>>> remains isolated from hints.

[...]

>>> +	/* End of the 1st cache line */
>>> +	struct ice_rx_ring *rx_ring;
>>
>> Can't we get rid of ring dependency? Maybe there's only a couple fields
>> that could be copied here instead of referencing the ring? I just find
>> it weird that our drivers often look for something in the ring structure
>> to parse a descriptor ._.
>> If not, can't it be const?
> 
> You're right, I could put just rx_ring->cached_phctime into this structure.
> But I recall you saying that if we access ring for timestamps only this is not a 
> problem :)

Sure, it's not a problem, I just thought it's an overkill to put pointer
to the ring here, since it's not needed to parse descriptors.
...checked right now, the function which processes timestamp from a
descriptor really needs only ::cached_phctime from the ring, nothing
more. Sorta overkill I think :s This phctime would be enough to put here.

> 
>>
>>> +};
>>> +
>>> +static_assert(offsetof(struct ice_xdp_buff, xdp_buff) == 0);
>>> +
>>>  /* indices into GLINT_ITR registers */
>>>  #define ICE_RX_ITR	ICE_IDX_ITR0
>>>  #define ICE_TX_ITR	ICE_IDX_ITR1

[...]

>>> +		struct ice_xdp_buff xdp_ext;
>>> +		struct xdp_buff xdp;
>>> +	};
>>
>> ...or you can leave just one xdp_ext (naming it just "xdp") -- for now,
>> this union does literally nothing, as xdp_ext contains xdp at its very
>> beginning.
> 
> I would like to leave non-meta-related-code rather unaware of existance of 
> ice_xdp_buff. Why access '&ring->xdp.xdp_buff' or '(struct xdp_buff *)xdp', when 
> we can do just 'ring->xdp'?

Hmm, got it. On point :D

> 
>>
>>>  	/* CL3 - 3rd cacheline starts here */
>>>  	struct bpf_prog *xdp_prog;
>>>  	u16 rx_offset;

[...]

>>> +static inline void
>>> +ice_xdp_set_meta_srcs(struct xdp_buff *xdp,
>>
>> Not sure about the naming... But can't propose anything :clownface:
>> ice_xdp_init_buff()? Like xdp_init_buff(), but ice_xdp_buff :D
> 
> ice_xdp_init_buff() sound exactly like a custom wrapper for xdp_init_buff(), but 
> usage of those functions would be quite different. I've contemplated the naming 
> of this one for some time and think it's good enough as it is, at least it 
> communicates that function has sth to do with 'xdp' and 'meta' and doesn't sound 
> like it fills in metadata.

ice_xdp_prepare_buff() :D Just kiddin, "set_meta_srcs" is fine, too.

>>
>>> +		      union ice_32b_rx_flex_desc *eop_desc,
>>> +		      struct ice_rx_ring *rx_ring)
>>> +{
>>> +	struct ice_xdp_buff *xdp_ext = (struct ice_xdp_buff *)xdp;
>>
>> I'd use container_of(), even though it will do the same thing here.
>> BTW, is having &xdp_buff at offset 0 still a requirement?
> 
> I've actually forgot about why it is a requirement, but have found my older 
> github answer to you.
> 
> "AF_XDP implementation also assumes xdp_buff is at the start".
> 
> What I meant by that is xdp_buffs from xsk_pool have only tailroom.

> 
> Maybe I should add a comment about this next to static assert.
> Will change to container_of, I guess it's more future-proof.

Ah, AF_XDP programs, right. Comment near the assertion + container_of()
sounds perfect.

[...]

Thanks,
Olek