[DTrace-devel] [PATCH] Adjust dynvarsize to avoid hitting BPF size limits

[Kris Van Hees]

On Thu, Dec 09, 2021 at 01:40:19PM +0000, Nick Alcock wrote:
> On 9 Dec 2021, Kris Van Hees via DTrace-devel told this:
> 
> > The number of entries in the dvars BPF map is calvulated as the
> > dynvarsize divided by the size of the value type, which is 1.  It
> > turns out that there is a limit on the number of entries and this
> > test hits that limit.
> 
> ... presumably because of the number of CPUs?
> 
> This sounds rather limiting: you get failures if you store even a single
> single-byte thing in a per-thread array? (And presumably even shorts are
> getting close to the limit, and ints aren't that far off.)

Actually, no, because dvars is not per-cpu (dvars is a hash map, not per-cpu).
But there still is a limit to how many elements there can be, which is a BPF
limit, not something we do.  And so it can change at will at the kernel level
without us knowing.

> That sounds like a feature users will not expect at all, and which will
> bite them by default... though I guess it's rare to store single chars
> anywhere, so maybe this isn't that problematical.

It would be extremely rare for a D script to only use TLS variables of type
'char', yes.  And not something we can really control.

> > THis patch sets dynvarsize to 1024 to force a lower limit so that
> > this issue is not triggered anymore.
> 
> Perhaps "to force an upper bound" or "to forcibly reduce the limit"?
> It's certainly not adding a *lower limit* (i.e. a lower bound) to
> anything.

Well no, the setting of dynvaesize forces the test to use a lower limit on the
size of the dynvar area, thereby ensuring that we do not hit this issue with
BPF imposing some limit on number of entries in the hash map.

> Really I think the code in dt_bpf.c should be bounding these things so
> that failure doesn't happen. (But maybe that's for the next release.)

That can become tricky because we may not know what limit the kernel is
enfocing (this can change at any time if the BPF devs make a change in the
kernel), and that means that we need to start rejecting dynvarsize option
settings as well based on the program needs.  And expect users to understand
the exact nature of this issue.

And honestly, since dynvarsize is an option to tune this, making use of that
option seems quite correct to make the test work.  The test is not exercising
anything that depends on dynvarsize itself anyway, so it is safe to lower it.

> > Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> 
> With the caveat that this really shouldn't be necessary at all and
> should be removed as soon as something better is done,

No, no need to remove it because the test is valid even for low dynvarsize
settings so even if the kernel no longer imposes a limit, there is no need to
revert this.  THe test is and aremains valid.

> Reviewed-by: Nick Alcock <nick.alcock at oracle.com>
> 
> on the basis that it does fix the bug, but ew.

I don't really see where it is really 'ew' because we are simply setting a
specific dynvarsize for one test.  Nothing changing in terms of how Dtrace
works for regular use.  Yes, people may hit this issue, but that is a system
limitation that we currently do not have control over (and probably never
really do).  Likewise, you could set dynvarsize to be so high that the system
will fail to create the hashmap.  All system limitations that even in legacy
DTrace formed the basis for being able to tune dynvarsize (and other tuneables).