Re: [PATCH] xsk: Use pool->dma_pages to check for DMA

[Maciej Fijalkowski]

On Tue, Apr 25, 2023 at 12:52:53AM +0200, Kal Cutter Conley wrote:

Hey again!

> > > Compare pool->dma_pages instead of pool->dma_pages_cnt to check for an
> > > active DMA mapping. pool->dma_pages needs to be read anyway to access
> > > the map so this compiles to more efficient code.
> >
> > Was it noticable in some sort of performance test?
> 
> This patch is part of the patchset found at
> https://lore.kernel.org/all/20230412162114.19389-3-kal.conley@xxxxxxxxxxx/
> which is being actively discussed and needs to be resubmitted anyway
> because of a conflict. While the discussion continues, I am submitting
> this patch by itself because I think it's an improvement on its own
> (regardless of what happens with the rest of the linked patchset). On
> one system, I measured a performance regression of 2-3% with xdpsock
> and the linked changes without the current patch. With the current
> patch, the performance regression was no longer observed.

Okay, 2-3% but with what settings? rxdrop for unaligned mode? what chunk
size etc? We need this kind of info, "compiles to more efficient code"
from original commit message is too generic and speculative to me. 2-3% of
perf diff against specific xdpsock setup is real improvement and is a
strong argument for getting this patch as-is, by its own.

> 
> > > diff --git a/include/net/xsk_buff_pool.h b/include/net/xsk_buff_pool.h
> > > index d318c769b445..a8d7b8a3688a 100644
> > > --- a/include/net/xsk_buff_pool.h
> > > +++ b/include/net/xsk_buff_pool.h
> > > @@ -180,7 +180,7 @@ static inline bool xp_desc_crosses_non_contig_pg(struct xsk_buff_pool *pool,
> > >       if (likely(!cross_pg))
> > >               return false;
> > >
> > > -     return pool->dma_pages_cnt &&
> > > +     return pool->dma_pages &&
> > >              !(pool->dma_pages[addr >> PAGE_SHIFT] & XSK_NEXT_PG_CONTIG_MASK);
> > >  }
> 
> I would consider the above code part of the "fast path". It may be
> executed approximately once per frame in unaligned mode.
> 
> > This seems to be used in the setup/tear-down paths so your optimizing
> > a control side. Is there a fast path with this code? I walked the
> > ice driver. If its just setup code we should do whatever is more
> > readable.
> 
> It is not only used in setup/tear-down paths (see above).
> Additionally, I believe the code is also _more_ readable with this
> patch applied. In particular, this patch reduces cognitive complexity
> since people (and compilers) reading the code don't need to
> additionally think about pool->dma_pages_cnt.

John was referring to xp_dma_unmap() with the comment above which indeed
is a teardown path, so probably this doesn't matter from performance
perspective and you could avoid this chunk from your patch.

> 
> > Both the _alloc_ cases read neighboring free_heads_cnt so your saving a load I guess?
> > This is so deep into micro-optimizing I'm curious if you could measure it?
> 
> It is saving a load which also reduces code size. This will affect
> other decisions such as what to inline. Also in the linked patchset,
> dma_pages and dma_pages_cnt do not share a cache line (on x86_64).

Yes I believe that the with your patch on unaligned mode by touching the
dma_pages you're warming the relevant cache line for your setup.

> 
> >
> > >               } else {
> > >                       xskb = &pool->heads[xp_aligned_extract_idx(pool, addr)];
> >
> > I'm not actually against optimizing but maybe another idea. Why do we have to
> > check at all? Seems if the DMA has been disabled/unmapped the driver shouldn't
> > be trying to call xsk_buff_alloc_batch? Then you can just drop the 'if' check.
> >
> > It feels to me the drivers shouldn't even be calling this after unmapping
> > the dma. WDYT?
> 
> Many of these code paths are used both for ZC and copy modes. You
> might be right that this particular case is only used with DMA.

Map/unmap is when you do ZC, copy/skb mode work without these internal dma
mappings.