On Wed, 2024-07-10 at 11:49 -0700, Andrii Nakryiko wrote:
> On Wed, Jul 10, 2024 at 11:41 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
> >
> > On Wed, 2024-07-10 at 10:50 -0700, Andrii Nakryiko wrote:
> >
> > [...]
> >
> > > > > Ok, I see, this wasn't obvious that you want this behavior. I actually
> > > > > find it surprising (and so at least let's call this possibility out).
> > > > >
> > > > > But, tbh, I'd make this stricter. I'd dictate that within a subprogram
> > > > > all no_csr patterns *should* end with the same stack offset and I
> > > > > would check for that (so what I mentioned before that instead of min
> > > > > or max we need assignment and check for equality if we already
> > > > > assigned it once).
> > > >
> > > > This makes sense, but does not cover a theoretical corner case:
> > > > - suppose there are two nocsr functions A and B on the kernel side,
> > > > but only was A marked as nocsr during program compilation;
> > > > - the spill/fill "bracket" was generated for a call to function B by
> > > > the compiler (because this is just a valid codegen).
> > >
> > > In this case the call to A shouldn't be changing nocsr_offset at all,
> > > though? You should find no spill/fill and thus even though A is
> > > *allowed* to use no_csr, it doesn't, so it should have no effect on
> > > nocsr offsets, no?
> >
> > Consider the following code:
> >
> > *(u64 *)(r10 - 24) = r1;
> > call A // kernel and clang know that A is nocsr
> > r1 = *(u64 *)(r10 - 24); // kernel assumes .nocsr_stack_offset -24
> > ...
> > *(u64 *)(r10 - 16) = r1;
> > call B // only kernel knows that B is nocsr
> > r1 = *(u64 *)(r10 - 16); // with stricter logic this would disable
> > // nocsr for the whole sub-probram
>
> oh, you mean that r10-16 accesses are valid instructions emitted by
> the compiler but not due to nocsr? I mean, tough luck?... We shouldn't
> reject this, but nocsr is ultimately a performance optimization, so
> not critical if it doesn't work within some subprogram.
Not critical, but the difference between allowing and disallowing nocsr
for this case is '<' (current) vs '==' (suggested) check for .nocsr_stack_offset.
I think that current algo should not be made more strict.
> > > But your example actually made me think about another (not theoretical
> > > at all) case. What if we have calls to A and B, the kernel is slightly
> > > old and knows that B is nocsr, but A is not. But the BPF program was
> > > compiled with the latest helper/kfunc definitions marking A as no_csr
> > > eligible (as well as B). (btw, THAT'S THE WORD for allow_csr --
> > > ELIGIBLE. csr_eligible FTW! but I digress...)
> > >
> > > With the case above we'll disable nocsr for both A and B, right? That
> > > sucks, but not sure if we can do anything about that. (We can probably
> > > assume no_csr pattern and thus allow spill/fill and not disable nocsr
> > > in general, but not remove spill/fills... a bit more complication
> > > upfront for longer term extensibility.. not sure, maybe performance
> > > regression is a fine price, hmm)
> > >
> > > So I take it back about unmarking csr in the *entire BPF program*,
> > > let's just limit it to the subprog scope. But I still think we should
> > > do it eagerly, rather than double checking in do_misc_followups().
> > > WDYT?
> >
> > With current algo this situation would disable nocsr indeed.
> > The problem is that checks for spilled stack slots usage is too simplistic.
> > However, it could be covered if the check is performed using e.g.
> > process I described earlier:
> > - for spill, remember the defining instruction in the stack slot structure;
> > - for fill, "merge" the defining instruction index;
> > - for other stack access mark defining instruction as escaping.
>
> Sorry, I have no idea what the above means and implies. "defining
> instruction", "merge", "escaping"
>
> As I mentioned above, I'd keep it as simple as reasonably possible.
It should not be much more complex compared to current implementation.
1: *(u64 *)(r10 - 16) = r1; // for stack slot -16 remember that
// it is defined at insn (1)
2: call %[nocsr_func]
3: r1 = *(u64 *)(r10 - 16); // the value read from stack is defined
// at (1), so remember this in insn aux
If (1) is the only defining instruction for (3) and value written at (1)
is not used by other instructions (e.g. not passed as function argument,
"escapes"), the pair (1) and (3) could be removed.
> > >
> > - on the first pass true .nocsr_stack_off is not yet known,
> > so .nocsr_pattern is set optimistically;
> > - on the second pass .nocsr_stack_off is already known,
> > so .nocsr_pattern can be removed from spills/fills outside the range;
> > - check_nocsr_stack_contract() does not need to scan full sub-program
> > on each violation, it can set a flag disabling nocsr in subprogram info.
>
> I'm not even sure anymore if we are agreeing or disagreeing, let's
> just look at the next revision or something, I'm getting lost.
>
> I was saying that as soon as check_nocsr_stack_contract() detects that
> contract is breached, we eagerly set nocsr_pattern and
> nocsr_spills_num to 0 for all instructions within the current subprog
> and setting nocsr_stack_off for subprog info to 0 (there is no more
> csr). There will be no other violations after that within that
> subprog. Maybe that's what you mean as well and I just can't read.
Ok, I'll send v3, let's proceed from there.
[...]
