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

[Larysa Zaremba <larysa.zaremba@intel.com>]

On Mon, May 22, 2023 at 06:46:40PM +0200, Alexander Lobakin wrote:
> From: Larysa Zaremba <larysa.zaremba@intel.com>
> 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
> > 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.
> > 
> > Size of the new child structure (ice_xdp_buff) is 72 bytes,
> > therefore it does not fit into a single cache line.
> > To at least place union at the start of cache line, move 'next'
> > field from CL3 to CL1, as it isn't used often.
> > 
> > Placing union at the start of cache line makes at least xdp_buff
> > and descriptor fit into a single CL,
> > ring pointer is used less often, so it can spill into the next CL.
> 
> Spill or span?

I guess 'span' is the better word.

> 
> > 
> > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com>
> > ---
> >  drivers/net/ethernet/intel/ice/ice_txrx.c     |  7 ++++--
> >  drivers/net/ethernet/intel/ice/ice_txrx.h     | 23 ++++++++++++++++---
> >  drivers/net/ethernet/intel/ice/ice_txrx_lib.h | 11 +++++++++
> >  3 files changed, 36 insertions(+), 5 deletions(-)
> 
> [...]
> 
> > --- a/drivers/net/ethernet/intel/ice/ice_txrx.h
> > +++ b/drivers/net/ethernet/intel/ice/ice_txrx.h
> > @@ -260,6 +260,15 @@ enum ice_rx_dtype {
> >  	ICE_RX_DTYPE_SPLIT_ALWAYS	= 2,
> >  };
> >  
> > +struct ice_xdp_buff {
> > +	struct xdp_buff xdp_buff;
> > +	union ice_32b_rx_flex_desc *eop_desc;	/* Required for all metadata */
> 
> Probably can be const here as well after changing all the places
> appropriately -- I don't think you write to it anywhere.

Correct.

> 
> > +	/* 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 :)

> 
> > +};
> > +
> > +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
> > @@ -301,7 +310,6 @@ enum ice_dynamic_itr {
> >  /* descriptor ring, associated with a VSI */
> >  struct ice_rx_ring {
> >  	/* CL1 - 1st cacheline starts here */
> > -	struct ice_rx_ring *next;	/* pointer to next ring in q_vector */
> >  	void *desc;			/* Descriptor ring memory */
> >  	struct device *dev;		/* Used for DMA mapping */
> >  	struct net_device *netdev;	/* netdev ring maps to */
> > @@ -313,12 +321,19 @@ struct ice_rx_ring {
> >  	u16 count;			/* Number of descriptors */
> >  	u16 reg_idx;			/* HW register index of the ring */
> >  	u16 next_to_alloc;
> > -	/* CL2 - 2nd cacheline starts here */
> > +
> >  	union {
> >  		struct ice_rx_buf *rx_buf;
> >  		struct xdp_buff **xdp_buf;
> >  	};
> > -	struct xdp_buff xdp;
> > +	/* CL2 - 2nd cacheline starts here
> > +	 * Size of ice_xdp_buff is 72 bytes,
> > +	 * so it spills into CL3
> > +	 */
> > +	union {
> > +		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'?

> 
> >  	/* CL3 - 3rd cacheline starts here */
> >  	struct bpf_prog *xdp_prog;
> >  	u16 rx_offset;
> > @@ -328,6 +343,8 @@ struct ice_rx_ring {
> >  	u16 next_to_clean;
> >  	u16 first_desc;
> >  
> > +	struct ice_rx_ring *next;	/* pointer to next ring in q_vector */
> 
> It can be placed even farther, somewhere near rcu_head -- IIRC it's not
> used anywhere on hotpath. Even ::ring_stats below is hotter.

Ok, I'll try to but it further from the start.

> 
> > +
> >  	/* stats structs */
> >  	struct ice_ring_stats *ring_stats;
> >  
> > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.h b/drivers/net/ethernet/intel/ice/ice_txrx_lib.h
> > index e1d49e1235b3..2835a8348237 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.h
> > +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.h
> > @@ -151,4 +151,15 @@ ice_process_skb_fields(struct ice_rx_ring *rx_ring,
> >  		       struct sk_buff *skb);
> >  void
> >  ice_receive_skb(struct ice_rx_ring *rx_ring, struct sk_buff *skb, u16 vlan_tag);
> > +
> > +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.
> 
> > +		      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.

> 
> > +
> > +	xdp_ext->eop_desc = eop_desc;
> > +	xdp_ext->rx_ring = rx_ring;
> > +}
> >  #endif /* !_ICE_TXRX_LIB_H_ */
> 
> Thanks,
> Olek