On Tue, 8 Dec 2020 11:31:03 +0100
Lorenzo Bianconi [off-list ref] wrote:

quoted

On Mon, 2020-12-07 at 22:37 +0100, Maciej Fijalkowski wrote:  

quoted

On Mon, Dec 07, 2020 at 01:15:00PM -0800, Alexander Duyck wrote:  

quoted

On Mon, Dec 7, 2020 at 8:36 AM Lorenzo Bianconi <lorenzo@kernel.org  

quoted

wrote:
Initialize multi-buffer bit (mb) to 0 in all XDP-capable drivers.
This is a preliminary patch to enable xdp multi-buffer support.

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>  

I'm really not a fan of this design. Having to update every driver in
order to initialize a field that was fragmented is a pain. At a
minimum it seems like it might be time to consider introducing some
sort of initializer function for this so that you can update things in
one central place the next time you have to add a new field instead of
having to update every individual driver that supports XDP. Otherwise
this isn't going to scale going forward.  

+1

quoted

quoted

Also, a good example of why this might be bothering for us is a fact that
in the meantime the dpaa driver got XDP support and this patch hasn't been
updated to include mb setting in that driver.
  

something like
init_xdp_buff(hard_start, headroom, len, frame_sz, rxq);

would work for most of the drivers.
  

ack, agree. I will add init_xdp_buff() in v6.

I do like the idea of an initialize helper function.
Remember this is fast-path code and likely need to be inlined.

Further more, remember that drivers can and do optimize the number of
writes they do to xdp_buff.   There are a number of fields in xdp_buff
that only need to be initialized once per NAPI.  E.g. rxq and frame_sz
(some driver do change frame_sz per packet).  Thus, you likely need two
inlined helpers for init.

Again, remember that C-compiler will generate an expensive operation
(rep stos) for clearing a struct if it is initialized like this, where
all member are not initialized (do NOT do this):

 struct xdp_buff xdp = {
   .rxq = rxq,
   .frame_sz = PAGE_SIZE,
 };

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer