Message-ID: <20201208110125.GC36228@lore-desk>
Date:   Tue, 8 Dec 2020 12:01:25 +0100
From:   Lorenzo Bianconi <lorenzo.bianconi@...hat.com>
To:     Saeed Mahameed <saeed@...nel.org>
Cc:     Lorenzo Bianconi <lorenzo@...nel.org>, bpf@...r.kernel.org,
        netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
        ast@...nel.org, daniel@...earbox.net, shayagr@...zon.com,
        sameehj@...zon.com, john.fastabend@...il.com, dsahern@...nel.org,
        brouer@...hat.com, echaudro@...hat.com, jasowang@...hat.com
Subject: Re: [PATCH v5 bpf-next 03/14] xdp: add xdp_shared_info data structure

> On Mon, 2020-12-07 at 17:32 +0100, Lorenzo Bianconi wrote:
> > Introduce xdp_shared_info data structure to contain info about
> > "non-linear" xdp frame. xdp_shared_info will alias skb_shared_info
> > allowing to keep most of the frags in the same cache-line.
> > Introduce some xdp_shared_info helpers aligned to skb_frag* ones
> > 
> 
> is there or will be a more general purpose use to this xdp_shared_info
> ? other than hosting frags ?

I do not have other use-cases at the moment other than multi-buff but in
theory it is possible I guess.
The reason we introduced it is to have most of the frags in the first
shared_info cache-line to avoid cache-misses.

> 
> > Signed-off-by: Lorenzo Bianconi <lorenzo@...nel.org>
> > ---
> >  drivers/net/ethernet/marvell/mvneta.c | 62 +++++++++++++++--------
> > ----
> >  include/net/xdp.h                     | 52 ++++++++++++++++++++--
> >  2 files changed, 82 insertions(+), 32 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/marvell/mvneta.c
> > b/drivers/net/ethernet/marvell/mvneta.c
> > index 1e5b5c69685a..d635463609ad 100644
> > --- a/drivers/net/ethernet/marvell/mvneta.c
> > +++ b/drivers/net/ethernet/marvell/mvneta.c
> > @@ -2033,14 +2033,17 @@ int mvneta_rx_refill_queue(struct mvneta_port
> > *pp, struct mvneta_rx_queue *rxq)
> >  
> 
> [...]
> 
> >  static void
> > @@ -2278,7 +2281,7 @@ mvneta_swbm_add_rx_fragment(struct mvneta_port
> > *pp,
> >  			    struct mvneta_rx_desc *rx_desc,
> >  			    struct mvneta_rx_queue *rxq,
> >  			    struct xdp_buff *xdp, int *size,
> > -			    struct skb_shared_info *xdp_sinfo,
> > +			    struct xdp_shared_info *xdp_sinfo,
> >  			    struct page *page)
> >  {
> >  	struct net_device *dev = pp->dev;
> > @@ -2301,13 +2304,13 @@ mvneta_swbm_add_rx_fragment(struct
> > mvneta_port *pp,
> >  	if (data_len > 0 && xdp_sinfo->nr_frags < MAX_SKB_FRAGS) {
> >  		skb_frag_t *frag = &xdp_sinfo->frags[xdp_sinfo-
> > >nr_frags++];
> >  
> > -		skb_frag_off_set(frag, pp->rx_offset_correction);
> > -		skb_frag_size_set(frag, data_len);
> > -		__skb_frag_set_page(frag, page);
> > +		xdp_set_frag_offset(frag, pp->rx_offset_correction);
> > +		xdp_set_frag_size(frag, data_len);
> > +		xdp_set_frag_page(frag, page);
> >  
> 
> why three separate setters ? why not just one 
> xdp_set_frag(page, offset, size) ?

to be aligned with skb_frags helpers, but I guess we can have a single helper,
I do not have a strong opinion on it

> 
> >  		/* last fragment */
> >  		if (len == *size) {
> > -			struct skb_shared_info *sinfo;
> > +			struct xdp_shared_info *sinfo;
> >  
> >  			sinfo = xdp_get_shared_info_from_buff(xdp);
> >  			sinfo->nr_frags = xdp_sinfo->nr_frags;
> > @@ -2324,10 +2327,13 @@ static struct sk_buff *
> >  mvneta_swbm_build_skb(struct mvneta_port *pp, struct mvneta_rx_queue
> > *rxq,
> >  		      struct xdp_buff *xdp, u32 desc_status)
> >  {

[...]

> >  
> > -static inline struct skb_shared_info *
> > +struct xdp_shared_info {
> 
> xdp_shared_info is a bad name, we need this to have a specific purpose 
> xdp_frags should the proper name, so people will think twice before
> adding weird bits to this so called shared_info.

I named the struct xdp_shared_info to recall skb_shared_info but I guess
xdp_frags is fine too. Agree?

> 
> > +	u16 nr_frags;
> > +	u16 data_length; /* paged area length */
> > +	skb_frag_t frags[MAX_SKB_FRAGS];
> 
> why MAX_SKB_FRAGS ? just use a flexible array member 
> skb_frag_t frags[]; 
> 
> and enforce size via the n_frags and on the construction of the
> tailroom preserved buffer, which is already being done.
> 
> this is waste of unnecessary space, at lease by definition of the
> struct, in your use case you do:
> memcpy(frag_list, xdp_sinfo->frags, sizeof(skb_frag_t) * num_frags);
> And the tailroom space was already preserved for a full skb_shinfo.
> so i don't see why you need this array to be of a fixed MAX_SKB_FRAGS
> size.

In order to avoid cache-misses, xdp_shared info is built as a variable
on mvneta_rx_swbm() stack and it is written to "shared_info" area only on the
last fragment in mvneta_swbm_add_rx_fragment(). I used MAX_SKB_FRAGS to be
aligned with skb_shared_info struct but probably we can use even a smaller value.
Another approach would be to define two different struct, e.g.

stuct xdp_frag_metadata {
	u16 nr_frags;
	u16 data_length; /* paged area length */
};

struct xdp_frags {
	skb_frag_t frags[MAX_SKB_FRAGS];
};

and then define xdp_shared_info as

struct xdp_shared_info {
	stuct xdp_frag_metadata meta;
	skb_frag_t frags[];
};

In this way we can probably optimize the space. What do you think?

> 
> > +};
> > +
> > +static inline struct xdp_shared_info *
> >  xdp_get_shared_info_from_buff(struct xdp_buff *xdp)
> >  {
> > -	return (struct skb_shared_info *)xdp_data_hard_end(xdp);
> > +	BUILD_BUG_ON(sizeof(struct xdp_shared_info) >
> > +		     sizeof(struct skb_shared_info));
> > +	return (struct xdp_shared_info *)xdp_data_hard_end(xdp);
> > +}
> > +
> 
> Back to my first comment, do we have plans to use this tail room buffer
> for other than frag_list use cases ? what will be the buffer format
> then ? should we push all new fields to the end of the xdp_shared_info
> struct ? or deal with this tailroom buffer as a stack ? 
> my main concern is that for drivers that don't support frag list and
> still want to utilize the tailroom buffer for other usecases they will
> have to skip the first sizeof(xdp_shared_info) so they won't break the
> stack.

for the moment I do not know if this area is used for other purposes.
Do you think there are other use-cases for it?

> 
> > +static inline struct page *xdp_get_frag_page(const skb_frag_t *frag)
> > +{
> > +	return frag->bv_page;
> > +}
> > +
> > +static inline unsigned int xdp_get_frag_offset(const skb_frag_t
> > *frag)
> > +{
> > +	return frag->bv_offset;
> > +}
> > +
> > +static inline unsigned int xdp_get_frag_size(const skb_frag_t *frag)
> > +{
> > +	return frag->bv_len;
> > +}
> > +
> > +static inline void *xdp_get_frag_address(const skb_frag_t *frag)
> > +{
> > +	return page_address(xdp_get_frag_page(frag)) +
> > +	       xdp_get_frag_offset(frag);
> > +}
> > +
> > +static inline void xdp_set_frag_page(skb_frag_t *frag, struct page
> > *page)
> > +{
> > +	frag->bv_page = page;
> > +}
> > +
> > +static inline void xdp_set_frag_offset(skb_frag_t *frag, u32 offset)
> > +{
> > +	frag->bv_offset = offset;
> > +}
> > +
> > +static inline void xdp_set_frag_size(skb_frag_t *frag, u32 size)
> > +{
> > +	frag->bv_len = size;
> >  }
> >  
> >  struct xdp_frame {
> > @@ -120,12 +164,12 @@ static __always_inline void
> > xdp_frame_bulk_init(struct xdp_frame_bulk *bq)
> >  	bq->xa = NULL;
> >  }
> >  
> > -static inline struct skb_shared_info *
> > +static inline struct xdp_shared_info *
> >  xdp_get_shared_info_from_frame(struct xdp_frame *frame)
> >  {
> >  	void *data_hard_start = frame->data - frame->headroom -
> > sizeof(*frame);
> >  
> > -	return (struct skb_shared_info *)(data_hard_start + frame-
> > >frame_sz -
> > +	return (struct xdp_shared_info *)(data_hard_start + frame-
> > >frame_sz -
> >  				SKB_DATA_ALIGN(sizeof(struct
> > skb_shared_info)));
> >  }
> >  
> 
> need a comment here why we preserve the size of skb_shared_info, yet
> the usable buffer is of type xdp_shared_info.

ack, I will add it in v6.

Regards,
Lorenzo

> 

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists