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
