On Tue, Jul 14, 2020 at 1:55 PM Maciej Fijalkowski
<maciej.fijalkowski@xxxxxxxxx> wrote:
>
> On Mon, Jul 13, 2020 at 08:36:30PM -0700, Alexei Starovoitov wrote:
> > On Tue, Jul 14, 2020 at 03:00:45AM +0200, Maciej Fijalkowski wrote:
> > > On Fri, Jul 10, 2020 at 08:25:20PM -0700, Alexei Starovoitov wrote:
> > > > On Fri, Jul 10, 2020 at 8:20 PM Alexei Starovoitov
> > > > <alexei.starovoitov@xxxxxxxxx> wrote:
> > > > >
> > > > > Of course you are right.
> > > > > pop+nop+push is incorrect.
> > > > >
> > > > > How about the following instead:
> > > > > - during JIT:
> > > > > emit_jump(to_skip_below) <- poke->tailcall_bypass
> > >
> > > That's the jump to the instruction right after the poke->tailcall_target.
> >
> > right. Mainly looking for better names than ip and ip_aux.
> >
> > > > > pop_callee_regs
> > > > > emit_jump(to_tailcall_target) <- poke->tailcall_target
> > >
> > > During JIT there's no tailcall_target so this will be nop5, right?
> >
> > I thought it will be always jmp, but with new info I agree that
> > it will start with nop.
> >
> > >
> > > > >
> > > > > - Transition from one target to another:
> > > > > text_poke(poke->tailcall_target, MOD_JMP, old_jmp, new_jmp)
> > > > > if (new_jmp != NULL)
> > > > > text_poke(poke->tailcall_bypass, MOD jmp into nop);
> > > > > else
> > > > > text_poke(poke->tailcall_bypass, MOD nop into jmp);
> > > >
> > > > One more correction. I meant:
> > > >
> > > > if (new_jmp != NULL) {
> > > > text_poke(poke->tailcall_target, MOD_JMP, old_jmp, new_jmp)
> > >
> > > Problem with having the old_jmp here is that you could have the
> > > tailcall_target removed followed by the new program being inserted. So for
> > > that case old_jmp is NULL but we decided to not poke the
> > > poke->tailcall_target when removing the program, only the tailcall_bypass
> > > is poked back to jmp from nop. IOW old_jmp is not equal to what
> > > poke->tailcall_target currently stores. This means that
> > > bpf_arch_text_poke() would not be successful for this update and that is
> > > the reason of faking it in this patch.
> >
> > got it.
> > I think it can be solved two ways:
> > 1. add synchronize_rcu() after poking of tailcall_bypass into jmp
> > and then update tailcall_target into nop.
> > so the race you've described in cover letter won't happen.
> > In the future with sleepable progs we'd need to call sync_rcu_tasks_trace too.
> > Which will make poke_run even slower.
> >
> > 2. add a flag to bpf_arch_text_poke() to ignore 5 bytes in there
> > and update tailcall_target to new jmp.
> > The speed of poke_run will be faster,
> > but considering the speed of text_poke_bp() it's starting to feel like
> > premature optimization.
> >
> > I think approach 1 is cleaner.
> > Then the pseudo code will be:
> > if (new_jmp != NULL) {
> > text_poke(poke->tailcall_target, MOD_JMP, old ? old_jmp : NULL, new_jmp);
> > if (!old)
> > text_poke(poke->tailcall_bypass, MOD_JMP, bypass_addr, NULL /* into nop */);
> > } else {
> > text_poke(poke->tailcall_bypass, MOD_JMP, NULL /* from nop */, bypass_addr);
> > sync_rcu(); /* let progs finish */
> > text_poke(poke->tailcall_target, MOD_JMP, old_jmp, NULL /* into nop */)
> > }
>
> Seems like this does the job :) clever stuff with sync_rcu.
> I tried this approach and one last thing that needs to be covered
> separately is the case of nop->nop update. We should simply avoid poking
> in this case. With this in place everything is functional.
>
> I will update the patch and descriptions and send the non-RFC revision, if
> you don't mind of course.
Yes. Please. Cannot wait actually :)
Please think through Daniel's comment in prog_array_map_poke_run().
Especially points 3 and 4. The new logic will be hitting the same cases,
but in a more elaborate way.
That comment also makes clear why memcmp(poke->ip, nop5...);
was not the correct approach... poke->ip address can be gone at that time.
