Re: [PATCH v19 bpf-next 12/23] bpf: add multi-buff support to the bpf_xdp_adjust_tail() API

[John Fastabend <john.fastabend@gmail.com>]

Lorenzo Bianconi wrote:
> > Lorenzo Bianconi wrote:
> > > > Lorenzo Bianconi wrote:
> > > > > From: Eelco Chaudron <echaudro@redhat.com>
> > > > > 
> > > > > This change adds support for tail growing and shrinking for XDP multi-buff.
> > > > > 
> > > > > When called on a multi-buffer packet with a grow request, it will work
> > > > > on the last fragment of the packet. So the maximum grow size is the
> > > > > last fragments tailroom, i.e. no new buffer will be allocated.
> > > > > A XDP mb capable driver is expected to set frag_size in xdp_rxq_info data
> > > > > structure to notify the XDP core the fragment size. frag_size set to 0 is
> > > > > interpreted by the XDP core as tail growing is not allowed.
> > > > > Introduce __xdp_rxq_info_reg utility routine to initialize frag_size field.
> > > > > 
> > > > > When shrinking, it will work from the last fragment, all the way down to
> > > > > the base buffer depending on the shrinking size. It's important to mention
> > > > > that once you shrink down the fragment(s) are freed, so you can not grow
> > > > > again to the original size.
> > > > > 
> > > > > Acked-by: Jakub Kicinski <kuba@kernel.org>
> > > > > Co-developed-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > > Signed-off-by: Eelco Chaudron <echaudro@redhat.com>

[...]

> > Then later there is the check 'if (unlikely(offset > 0) { ...}', but that
> > wont hit this case and we shrunk it back to a single frag. Did we want
> > to clear the mb in this case? I'm not seeing how it harms things to have
> > the mb bit set just trying to follow code here.
> 
> If I followed correctly your example, we will have sinfo->nr_frags = 1 at the
> end of the processing (since the first fragment has 2k size), right?
> If so mb bit must be set to 1. Am I missing something?
> Re-looking at the code I guess we should clear mb bit using sinfo->nr_frags
> instead:
> 
> 	if (!sinfo->nr_frags)
> 		xdp_buff_clear_mb(xdp);
> 
> Agree?

Agree that is more correct. I maybe messed up my example a bit, but I think
checking nr_frags clears it up.

Thanks,
John

> 
> Regards,
> Lorenzo