[DTrace-devel] [PATCH 04/12] options: report EDT_NOREG for iregs < DIF_DIR_NREGS
Eugene Loh
eugene.loh at oracle.com
Thu Jan 11 22:19:59 UTC 2024
On 1/11/24 16:48, Kris Van Hees wrote:
> On Thu, Jan 11, 2024 at 04:15:29PM -0500, Eugene Loh via DTrace-devel wrote:
>> Hardwiring the use of %r7 and %r8 in trampoline cg has presumably nothing to
>> do with the number of DIF iregs -- that is, with our ability to handle
>> expressions like:
>>
>> a + b * (c + d * (e + f * (g + h * (i + j * (k + l)))))
>>
>> For example, I think the 3/12 patch would work just fine with something
>> like:
>>
>> if (drp->dr_size > 7)
>> dt_regset_xalloc(drp, BPF_REG_7);
>> if (drp->dr_size > 8)
>> dt_regset_xalloc(drp, BPF_REG_8);
>>
>> (plus similar regset_free code). That is, if regset manages %r7 or %r8,
>> tell regset we are using them. If not, do not worry since regset will not
>> try to use them anyhow. In short, patch 3/12 does not need -xiregs changes.
> Well, 3/12 doesn't directly affect xiregs, no, but iregs affects 3/12. We
> cannot reserve registers outside the range of valid registers.
Right, but clearly we can use BPF registers that exist that we choose
not to make available for regset allocation. E.g., %r9 is such a
register. In fact, if %r7 and %r8 are outside the range of valid regset
registers, that would be a great reason to use them for this purpose!
Anyhow...
> I don't think
> it makes sense to support xiregs with a value less than 8 because it is very
> unlikely to actually result in working code. E.g. xiregs=5 would certainly
> break everything (no free available register that is not clobbered on calls),
> and even xiregs=6 would only leave a single register that is not clobbered and
> I am pretty certain we generate code that requires more.
I am not against constraining -xiregs's value to the default value. The
option does not seem particularly useful, and our implementation has
kind of ignored it (for good reason: there aren't many BPF regs to play
around with, as you point out).
>> Now, if you simply want to deprecate -xiregs:
>>
>> 1) The EDT_NOREG check in dt_opt_iregs() does not make sense to me. How
>> about,
>>
>> dt_opt_iregs(dtrace_hdl_t *dtp, const char *arg, uintptr_t option)
>> {
>> int n;
>>
>> - if (arg == NULL || (n = atoi(arg)) <= 0 || n > DIF_DIR_NREGS)
>> + if (arg == NULL || (n = atoi(arg)) <= 0 || n != DIF_DIR_NREGS)
>> return dt_set_errno(dtp, EDT_BADOPTVAL);
>>
>> dtp->dt_conf.dtc_difintregs = n;
>> return 0;
>> }
>>
>> That is simpler and more direct than the EDT_NOREG error. Afterall, if your
>> entire D program is 'BEGIN {exit(0)}', then asking -xiregs=7 is not a matter
>> of too few DIF integer registers. It is a matter than we have hardwired our
>> code so that -xiregs needs a particular value.
> What I do in this patch (even if it seems rather convoluted) is make a value
> for iregs that is greater than what BPF can provide remain a BADOPTVAL, but
> making a vlaue for iregs that is less than what is needed for our code
> generation NOREG (not enough registers to generate code).
Yes, it does seem convoluted to me. The problem is not that there are
insufficient registers, but the way we are using those registers.
Again, "-xiregs=7 -n 'BEGIN {exit(0)}'".
Anyhow, not worth arguing about.
Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
>> 2) The tst.iregs.sh test no longer has anything to do with the complexity
>> of DIF expressions. It is simply checking whether a script is trying to
>> change -xiregs. As such, the following is simpler and more direct:
>>
>> for n in 10 9 8 7 6; do
>> $dtrace -xiregs=$n -qn 'BEGIN { exit(0); }'
>> echo "iregs $n gives $?"
>> done
>>
>> with the corresponding changes in the .r file. If we want testing on some
>> actual DIF reg spill/fill code, we can augment
>> test/unittest/codegen/tst.kernel_read_scalar_regspill.d
> I kept the test (aside from expected results) the same because one would expect
> that the existing iregs test is not invalidated by the change to what values
> are valid for iregs.
>
>> 3) The preceding (3/12) patch in its current form requires guardrails on
>> -xiregs, since we otherwise hit assertion failures when trying to generate
>> trampoline code. So, if patch 3/12 requires the hardwired value of -xiregs,
>> then the dt_options.c check on -xiregs should be part of that patch.
> The patches are part of a patch series and so it is quite typical that changes
> in different areas are in their own patches. I felt it would be more
> confusing in all to put it all in the same patch.
>
>> On 1/5/24 20:11, Kris Van Hees wrote:
>>> On Fri, Jan 05, 2024 at 04:22:09PM -0500, Eugene Loh via DTrace-devel wrote:
>>>> See my comments to the 03/12 patch.
>>> As you say in the comments there, -xiregs has become effectively obsolete,
>>> but since it is an option DTrace accepts, we probably should continue to
>>> accept it. This patch ensure sthat the only valid value is DIF_DIR_NREGS.
>>>
>>> As some point we can and shouold change this to be the number of BPF regs
>>> that are available for general use. But even then, we cannot really support
>>> lowering that limit because there are so few BPF registers we can really use
>>> anyway due to the function call clobberingh of %r0-%r5.
>>>
>>> But this patch is the minimal change needed to ensure dtrace continues to
>>> work.
>>>
>>>> On 1/5/24 00:26, Kris Van Hees via DTrace-devel wrote:
>>>>> The trampoline generation code hardcodes the use of %r7 and %r8, which
>>>>> means that iregs cannot be less than 8 (DIF_DIR_NREGS).
>>>>>
>>>>> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
>>>>> ---
>>>>> libdtrace/dt_options.c | 4 +++-
>>>>> test/unittest/options/tst.iregs.r | 26 +++++++-------------------
>>>>> 2 files changed, 10 insertions(+), 20 deletions(-)
>>>>>
>>>>> diff --git a/libdtrace/dt_options.c b/libdtrace/dt_options.c
>>>>> index 6f7cf9e2..18b8f43a 100644
>>>>> --- a/libdtrace/dt_options.c
>>>>> +++ b/libdtrace/dt_options.c
>>>>> @@ -1,6 +1,6 @@
>>>>> /*
>>>>> * Oracle Linux DTrace.
>>>>> - * Copyright (c) 2007, 2023, Oracle and/or its affiliates. All rights reserved.
>>>>> + * Copyright (c) 2007, 2024, Oracle and/or its affiliates. All rights reserved.
>>>>> * Licensed under the Universal Permissive License v 1.0 as shown at
>>>>> * http://oss.oracle.com/licenses/upl.
>>>>> */
>>>>> @@ -266,6 +266,8 @@ dt_opt_iregs(dtrace_hdl_t *dtp, const char *arg, uintptr_t option)
>>>>> if (arg == NULL || (n = atoi(arg)) <= 0 || n > DIF_DIR_NREGS)
>>>>> return dt_set_errno(dtp, EDT_BADOPTVAL);
>>>>> + if (n < DIF_DIR_NREGS)
>>>>> + return dt_set_errno(dtp, EDT_NOREG);
>>>>> dtp->dt_conf.dtc_difintregs = n;
>>>>> return 0;
>>>>> diff --git a/test/unittest/options/tst.iregs.r b/test/unittest/options/tst.iregs.r
>>>>> index 4926b28d..f67e7b07 100644
>>>>> --- a/test/unittest/options/tst.iregs.r
>>>>> +++ b/test/unittest/options/tst.iregs.r
>>>>> @@ -1,25 +1,13 @@
>>>>> 7
>>>>> iregs 8 gives 0
>>>>> -7
>>>>> -iregs 7 gives 0
>>>>> -7
>>>>> -iregs 6 gives 0
>>>>> +iregs 7 gives 1
>>>>> +iregs 6 gives 1
>>>>> iregs 5 gives 1
>>>>> iregs 4 gives 1
>>>>> iregs 3 gives 1
>>>>> -- @@stderr --
>>>>> -dtrace: invalid probe specifier BEGIN {
>>>>> - a = b = c = d = e = f = g = h = i = j = k = l = 1;
>>>>> - trace(a + b * (c + d * (e + f * (g + h * (i + j * (k + l))))));
>>>>> - exit(0);
>>>>> - }: Insufficient registers to generate code
>>>>> -dtrace: invalid probe specifier BEGIN {
>>>>> - a = b = c = d = e = f = g = h = i = j = k = l = 1;
>>>>> - trace(a + b * (c + d * (e + f * (g + h * (i + j * (k + l))))));
>>>>> - exit(0);
>>>>> - }: Insufficient registers to generate code
>>>>> -dtrace: invalid probe specifier BEGIN {
>>>>> - a = b = c = d = e = f = g = h = i = j = k = l = 1;
>>>>> - trace(a + b * (c + d * (e + f * (g + h * (i + j * (k + l))))));
>>>>> - exit(0);
>>>>> - }: Insufficient registers to generate code
>>>>> +dtrace: failed to set -x iregs: Insufficient registers to generate code
>>>>> +dtrace: failed to set -x iregs: Insufficient registers to generate code
>>>>> +dtrace: failed to set -x iregs: Insufficient registers to generate code
>>>>> +dtrace: failed to set -x iregs: Insufficient registers to generate code
>>>>> +dtrace: failed to set -x iregs: Insufficient registers to generate code
>>>> _______________________________________________
>>>> DTrace-devel mailing list
>>>> DTrace-devel at oss.oracle.com
>>>> https://oss.oracle.com/mailman/listinfo/dtrace-devel
>> _______________________________________________
>> DTrace-devel mailing list
>> DTrace-devel at oss.oracle.com
>> https://oss.oracle.com/mailman/listinfo/dtrace-devel
More information about the DTrace-devel
mailing list