Re: [PATCH] dma-debug: dynamic allocation of hash table

[Eric Dumazet]

On Thu, Jan 30, 2020 at 3:46 PM Robin Murphy <robin.murphy@xxxxxxx> wrote:
>
> Hi Eric,
>
> On 2020-01-30 7:10 pm, Eric Dumazet via iommu wrote:
> > Increasing the size of dma_entry_hash size by 327680 bytes
> > has reached some bootloaders limitations.
>
> [ That might warrant some further explanation - I don't quite follow how
> this would relate to a bootloader specifically :/ ]

I had no details, please look at the prior thread where this has been discussed.

https://www.spinics.net/lists/linux-renesas-soc/msg46157.html


>
> > Simply use dynamic allocations instead, and take
> > this opportunity to increase the hash table to 65536
> > buckets. Finally my 40Gbit mlx4 NIC can sustain
> > line rate with CONFIG_DMA_API_DEBUG=y.
>
> That's pretty cool, but I can't help but wonder if making the table
> bigger caused a problem in the first place, whether making it bigger yet
> again in the name of a fix is really the wisest move. How might this
> impact DMA debugging on 32-bit embedded systems with limited vmalloc
> space and even less RAM, for instance? More to the point, does vmalloc()
> even work for !CONFIG_MMU builds? Obviously we don't want things to be
> *needlessly* slow if avoidable, but is there a genuine justification for
> needing to optimise what is fundamentally an invasive heavyweight
> correctness check - e.g. has it helped expose race conditions that were
> otherwise masked?

Not sure what you are saying.

vmalloc() _is_ supported by all arches, even !CONFIG_MMU

I can not test all platforms, and this is a debugging
feature no one uses in production.

>
> That said, by moving to dynamic allocation maybe there's room to be
> cleverer and make HASH_SIZE scale with, say, system memory size? (I
> assume from the context it's not something we can expand on-demand like
> we did for the dma_debug_entry pool)
>

How memory size can serve as a proxy of the number of entries ?
Current 10Gbit NIC need about 256,000 entries in the table.
Needless to say, the prior hash size was unusable.

As I suggested one month ago, HASH_SIZE can be tuned by a developper
eager to use this facility.

65536 slots are 768 KB on 32bit platforms.

> Robin.
>
> > Fixes: 5e76f564572b ("dma-debug: increase HASH_SIZE")
> > Signed-off-by: Eric Dumazet <edumazet@xxxxxxxxxx>
> > Reported-by: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> > Cc: Christoph Hellwig <hch@xxxxxx>
> > ---
> >   kernel/dma/debug.c | 10 ++++++++--
> >   1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c
> > index 2031ed1ad7fa109bb8a8c290bbbc5f825362baba..a310dbb1515e92c081f8f3f9a7290dd5e53fc889 100644
> > --- a/kernel/dma/debug.c
> > +++ b/kernel/dma/debug.c
> > @@ -27,7 +27,7 @@
> >
> >   #include <asm/sections.h>
> >
> > -#define HASH_SIZE       16384ULL
> > +#define HASH_SIZE       65536ULL
> >   #define HASH_FN_SHIFT   13
> >   #define HASH_FN_MASK    (HASH_SIZE - 1)
> >
> > @@ -90,7 +90,8 @@ struct hash_bucket {
> >   };
> >
> >   /* Hash list to save the allocated dma addresses */
> > -static struct hash_bucket dma_entry_hash[HASH_SIZE];
> > +static struct hash_bucket *dma_entry_hash __read_mostly;
> > +
> >   /* List of pre-allocated dma_debug_entry's */
> >   static LIST_HEAD(free_entries);
> >   /* Lock for the list above */
> > @@ -934,6 +935,10 @@ static int dma_debug_init(void)
> >       if (global_disable)
> >               return 0;
> >
> > +     dma_entry_hash = vmalloc(HASH_SIZE * sizeof(*dma_entry_hash));
> > +     if (!dma_entry_hash)
> > +             goto err;
> > +
> >       for (i = 0; i < HASH_SIZE; ++i) {
> >               INIT_LIST_HEAD(&dma_entry_hash[i].list);
> >               spin_lock_init(&dma_entry_hash[i].lock);
> > @@ -950,6 +955,7 @@ static int dma_debug_init(void)
> >               pr_warn("%d debug entries requested but only %d allocated\n",
> >                       nr_prealloc_entries, nr_total_entries);
> >       } else {
> > +err:
> >               pr_err("debugging out of memory error - disabled\n");
> >               global_disable = true;
> >
> >