Increasing the size of dma_entry_hash size by 327680 bytes
has reached some bootloaders limitations.
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.
Fixes: 5e76f564572b ("dma-debug: increase HASH_SIZE")
Signed-off-by: Eric Dumazet <[email protected]>
Reported-by: Geert Uytterhoeven <[email protected]>
Cc: Christoph Hellwig <[email protected]>
---
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;
--
2.25.0.341.g760bfbb309-goog
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 :/ ]
> 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?
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)
Robin.
> Fixes: 5e76f564572b ("dma-debug: increase HASH_SIZE")
> Signed-off-by: Eric Dumazet <[email protected]>
> Reported-by: Geert Uytterhoeven <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> ---
> 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;
>
>
On Thu, Jan 30, 2020 at 3:46 PM Robin Murphy <[email protected]> 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 <[email protected]>
> > Reported-by: Geert Uytterhoeven <[email protected]>
> > Cc: Christoph Hellwig <[email protected]>
> > ---
> > 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;
> >
> >
Hi Robin,
On Fri, Jan 31, 2020 at 12:46 AM Robin Murphy <[email protected]> wrote:
> 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 :/ ]
Increasing the size of a static array increases kernel size.
Some (all? ;-) bootloaders have limitations on the maximum size of a
kernel image they can boot (usually something critical gets overwritten
when handling a too large image). While boot loaders can be fixed and
upgraded, this is usually much more cumbersome than updating the
kernel.
Besides, a static array always consumes valuable unswapable memory,
even when the feature would not be used (e.g. disabled by a command
line option).
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On 2020-01-31 9:06 am, Geert Uytterhoeven wrote:
> Hi Robin,
>
> On Fri, Jan 31, 2020 at 12:46 AM Robin Murphy <[email protected]> wrote:
>> 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 :/ ]
>
> Increasing the size of a static array increases kernel size.
> Some (all? ;-) bootloaders have limitations on the maximum size of a
> kernel image they can boot (usually something critical gets overwritten
> when handling a too large image). While boot loaders can be fixed and
> upgraded, this is usually much more cumbersome than updating the
> kernel.
Ah, OK - I'm all too familiar with bootloaders having image size limits,
but I'm also used to implicitly-initialised statics being collected into
a runtime-initialised .bss section, so I hadn't realised that there
might still be platforms where that space is actually allocated in the
image at link-time.
> Besides, a static array always consumes valuable unswapable memory,
> even when the feature would not be used (e.g. disabled by a command
> line option).
Indeed, and that alone might have been a reasonable rationale for the
patch - I was merely querying the wording of the commit message, not its
intent :)
Thanks,
Robin.
On 2020-01-31 12:17 am, Eric Dumazet wrote:
> On Thu, Jan 30, 2020 at 3:46 PM Robin Murphy <[email protected]> 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
Thanks for the pointer - that had passed me by as it doesn't seem to
have been CCed to the IOMMU list or me as a reviewer.
>>> 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
OK, that's good - I wasn't sure off-hand, and I was on the wrong machine
to go digging into the source.
> I can not test all platforms, and this is a debugging
> feature no one uses in production.
...which is exactly why I'd prefer to see a stronger justification for
making a change which only benefits performance on large systems, while
potentially impacting usability on small ones.
>> 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.
Because it's actually a reasonable proxy for "system size" in this
context - there aren't many good reasons for a driver to maintain
hundreds of mappings of the *same* buffer, so in general the maximum
number of live mappings is effectively going to scale with the total
amount of memory, particularly at the smaller end. Consider it a pretty
safe assumption that an embedded system with RAM measured in megabytes
won't ever be running anything like an MLX4, let alone in a debug
situation. If those 256,000 mappings each represent a typical network
packet, that implies well over 300MB of data alone, not to mention the
size of the queues themselves, the actual DMA debug entries, and the
whole rest of the kernel beyond that one driver - I doubt it's
physically possible to 'need' 64K hash buckets without at very least 1GB
of total RAM, quite likely much more.
> 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.
...and when that represents ~5% of the total system RAM it is a *lot*
less reasonable than even 12KB. As I said, it's great to make a debug
option more efficient such that what it observes is more representative
of the non-debug behaviour, but it mustn't come at the cost of making
the entire option unworkable for other users.
Robin.
>>> Fixes: 5e76f564572b ("dma-debug: increase HASH_SIZE")
>>> Signed-off-by: Eric Dumazet <[email protected]>
>>> Reported-by: Geert Uytterhoeven <[email protected]>
>>> Cc: Christoph Hellwig <[email protected]>
>>> ---
>>> 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;
>>>
>>>
On Fri, Jan 31, 2020 at 4:30 AM Robin Murphy <[email protected]> wrote:
>
> ...and when that represents ~5% of the total system RAM it is a *lot*
> less reasonable than even 12KB. As I said, it's great to make a debug
> option more efficient such that what it observes is more representative
> of the non-debug behaviour, but it mustn't come at the cost of making
> the entire option unworkable for other users.
>
Then I suggest you send a patch to reduce PREALLOC_DMA_DEBUG_ENTRIES
because having 65536 preallocated entries consume 4 MB of memory.
Actually this whole attempt to re-implement slab allocations in this
file is suspect.
Do not get me wrong, but if you really want to run linux on a 16MB host,
I guess you need to add CONFIG_BASE_SMALL all over the places,
not only in this kernel/dma/debug.c file.
On 31/01/2020 2:42 pm, Eric Dumazet wrote:
> On Fri, Jan 31, 2020 at 4:30 AM Robin Murphy <[email protected]> wrote:
>>
>> ...and when that represents ~5% of the total system RAM it is a *lot*
>> less reasonable than even 12KB. As I said, it's great to make a debug
>> option more efficient such that what it observes is more representative
>> of the non-debug behaviour, but it mustn't come at the cost of making
>> the entire option unworkable for other users.
>>
>
> Then I suggest you send a patch to reduce PREALLOC_DMA_DEBUG_ENTRIES
> because having 65536 preallocated entries consume 4 MB of memory.
...unless it's overridden on the command-line ;)
> Actually this whole attempt to re-implement slab allocations in this
> file is suspect.
Again that's a matter of expected usage patterns - typically the
"reasonable default" or user-defined preallocation should still be
enough (as it *had* to be before), and the kind of systems that can
sustain so many live mappings as to regularly hit the dynamic expansion
path are also likely to have enough memory not to care that later-unused
entries never get 'properly' freed back to the kernel (plus as you've
observed, many workloads tend to reach a steady state where entries are
mostly only transiently free anyway). The reasoning for the exact
implementation details is there in the commit logs.
> Do not get me wrong, but if you really want to run linux on a 16MB host,
> I guess you need to add CONFIG_BASE_SMALL all over the places,
> not only in this kernel/dma/debug.c file.
Right, nobody's suggesting that defconfig should "just work" on your
router/watch/IoT shoelaces/whatever, I'm just saying that tuning any
piece of common code for datacenter behemoths, for quality-of-life
rather than functional necessity, and leaving no way to adjust it other
than hacking the source, would represent an unnecessary degree of
short-sightedness that we can and should avoid.
Taking a closer look at the code, it appears fairly straightforward to
make the hash size variable, and in fact making it self-adjusting
doesn't seem too big a jump from there. I'm happy to have a go at that
myself if you like.
Robin.
On Fri, Jan 31, 2020 at 9:43 AM Robin Murphy <[email protected]> wrote:
>
> On 31/01/2020 2:42 pm, Eric Dumazet wrote:
> > On Fri, Jan 31, 2020 at 4:30 AM Robin Murphy <[email protected]> wrote:
> >>
> >> ...and when that represents ~5% of the total system RAM it is a *lot*
> >> less reasonable than even 12KB. As I said, it's great to make a debug
> >> option more efficient such that what it observes is more representative
> >> of the non-debug behaviour, but it mustn't come at the cost of making
> >> the entire option unworkable for other users.
> >>
> >
> > Then I suggest you send a patch to reduce PREALLOC_DMA_DEBUG_ENTRIES
> > because having 65536 preallocated entries consume 4 MB of memory.
>
> ...unless it's overridden on the command-line ;)
>
> > Actually this whole attempt to re-implement slab allocations in this
> > file is suspect.
>
> Again that's a matter of expected usage patterns - typically the
> "reasonable default" or user-defined preallocation should still be
> enough (as it *had* to be before), and the kind of systems that can
> sustain so many live mappings as to regularly hit the dynamic expansion
> path are also likely to have enough memory not to care that later-unused
> entries never get 'properly' freed back to the kernel (plus as you've
> observed, many workloads tend to reach a steady state where entries are
> mostly only transiently free anyway). The reasoning for the exact
> implementation details is there in the commit logs.
>
> > Do not get me wrong, but if you really want to run linux on a 16MB host,
> > I guess you need to add CONFIG_BASE_SMALL all over the places,
> > not only in this kernel/dma/debug.c file.
>
> Right, nobody's suggesting that defconfig should "just work" on your
> router/watch/IoT shoelaces/whatever, I'm just saying that tuning any
> piece of common code for datacenter behemoths, for quality-of-life
> rather than functional necessity, and leaving no way to adjust it other
> than hacking the source, would represent an unnecessary degree of
> short-sightedness that we can and should avoid.
>
> Taking a closer look at the code, it appears fairly straightforward to
> make the hash size variable, and in fact making it self-adjusting
> doesn't seem too big a jump from there. I'm happy to have a go at that
> myself if you like.
Sure, go ahead, I have no plan implementing changes for 20 years old platforms.
Thanks.
On 2020-01-31 5:46 pm, Eric Dumazet wrote:
> On Fri, Jan 31, 2020 at 9:43 AM Robin Murphy <[email protected]> wrote:
>>
>> On 31/01/2020 2:42 pm, Eric Dumazet wrote:
>>> On Fri, Jan 31, 2020 at 4:30 AM Robin Murphy <[email protected]> wrote:
>>>>
>>>> ...and when that represents ~5% of the total system RAM it is a *lot*
>>>> less reasonable than even 12KB. As I said, it's great to make a debug
>>>> option more efficient such that what it observes is more representative
>>>> of the non-debug behaviour, but it mustn't come at the cost of making
>>>> the entire option unworkable for other users.
>>>>
>>>
>>> Then I suggest you send a patch to reduce PREALLOC_DMA_DEBUG_ENTRIES
>>> because having 65536 preallocated entries consume 4 MB of memory.
>>
>> ...unless it's overridden on the command-line ;)
>>
>>> Actually this whole attempt to re-implement slab allocations in this
>>> file is suspect.
>>
>> Again that's a matter of expected usage patterns - typically the
>> "reasonable default" or user-defined preallocation should still be
>> enough (as it *had* to be before), and the kind of systems that can
>> sustain so many live mappings as to regularly hit the dynamic expansion
>> path are also likely to have enough memory not to care that later-unused
>> entries never get 'properly' freed back to the kernel (plus as you've
>> observed, many workloads tend to reach a steady state where entries are
>> mostly only transiently free anyway). The reasoning for the exact
>> implementation details is there in the commit logs.
>>
>>> Do not get me wrong, but if you really want to run linux on a 16MB host,
>>> I guess you need to add CONFIG_BASE_SMALL all over the places,
>>> not only in this kernel/dma/debug.c file.
>>
>> Right, nobody's suggesting that defconfig should "just work" on your
>> router/watch/IoT shoelaces/whatever, I'm just saying that tuning any
>> piece of common code for datacenter behemoths, for quality-of-life
>> rather than functional necessity, and leaving no way to adjust it other
>> than hacking the source, would represent an unnecessary degree of
>> short-sightedness that we can and should avoid.
>>
>> Taking a closer look at the code, it appears fairly straightforward to
>> make the hash size variable, and in fact making it self-adjusting
>> doesn't seem too big a jump from there. I'm happy to have a go at that
>> myself if you like.
>
>
> Sure, go ahead, I have no plan implementing changes for 20 years old platforms.
Heh, I'm fairly confident the 20-year-old drivers are probably
well-debugged by now anyway - it's the future low-end SoCs for
Linux-powered IoT shoelaces that I see being a rich vein of new bugs :)
How does this look to start with? (only compile-tested so far, but I'll
pick it up again properly next week)
Robin.
----->8-----
From: Robin Murphy <[email protected]>
Subject: [PATCH] dma/debug: Size hash table dynamically
Having a fixed-size hash table fails to scale nicely - making it large
enough to perform well for the mapping behaviour of big network adapters
tends towards unreasonable memory consumption for smaller systems. It's
logical to size the table for the number of expected entries, and doing
so conveniently puts the size/performance tradeoff in the hands of the
user via the "dma_debug_entries" option. The dynamic allocation also
sidesteps the issue of the static array bloating the kernel image on
certain platforms.
Signed-off-by: Robin Murphy <[email protected]>
---
kernel/dma/debug.c | 31 ++++++++++++++++++++-----------
1 file changed, 20 insertions(+), 11 deletions(-)
diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c
index 2031ed1ad7fa..86a4c68d6ac1 100644
--- a/kernel/dma/debug.c
+++ b/kernel/dma/debug.c
@@ -27,9 +27,9 @@
#include <asm/sections.h>
-#define HASH_SIZE 16384ULL
+static int hash_size __read_mostly;
#define HASH_FN_SHIFT 13
-#define HASH_FN_MASK (HASH_SIZE - 1)
+#define HASH_FN_MASK (hash_size - 1)
#define PREALLOC_DMA_DEBUG_ENTRIES (1 << 16)
/* If the pool runs out, add this many new entries at once */
@@ -90,7 +90,7 @@ 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;
/* List of pre-allocated dma_debug_entry's */
static LIST_HEAD(free_entries);
/* Lock for the list above */
@@ -398,7 +398,7 @@ void debug_dma_dump_mappings(struct device *dev)
{
int idx;
- for (idx = 0; idx < HASH_SIZE; idx++) {
+ for (idx = 0; idx < hash_size; idx++) {
struct hash_bucket *bucket = &dma_entry_hash[idx];
struct dma_debug_entry *entry;
unsigned long flags;
@@ -818,7 +818,7 @@ static int dump_show(struct seq_file *seq, void *v)
{
int idx;
- for (idx = 0; idx < HASH_SIZE; idx++) {
+ for (idx = 0; idx < hash_size; idx++) {
struct hash_bucket *bucket = &dma_entry_hash[idx];
struct dma_debug_entry *entry;
unsigned long flags;
@@ -862,7 +862,7 @@ static int device_dma_allocations(struct device
*dev, struct dma_debug_entry **o
unsigned long flags;
int count = 0, i;
- for (i = 0; i < HASH_SIZE; ++i) {
+ for (i = 0; i < hash_size; ++i) {
spin_lock_irqsave(&dma_entry_hash[i].lock, flags);
list_for_each_entry(entry, &dma_entry_hash[i].list, list) {
if (entry->dev == dev) {
@@ -934,7 +934,13 @@ static int dma_debug_init(void)
if (global_disable)
return 0;
- for (i = 0; i < HASH_SIZE; ++i) {
+ hash_size = roundup_pow_of_two(nr_prealloc_entries) / 16;
+ dma_entry_hash = kvmalloc_array(max(hash_size, 16),
+ sizeof(*dma_entry_hash), GFP_KERNEL);
+ if (!dma_entry_hash)
+ goto out_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,10 +956,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 {
- pr_err("debugging out of memory error - disabled\n");
- global_disable = true;
-
- return 0;
+ goto out_err;
}
min_free_entries = num_free_entries;
@@ -961,6 +964,12 @@ static int dma_debug_init(void)
pr_info("debugging enabled by kernel config\n");
return 0;
+
+out_err:
+ pr_err("debugging out of memory error - disabled\n");
+ global_disable = true;
+
+ return 0;
}
core_initcall(dma_debug_init);
--
2.17.1