Re: [PATCH] Updates to Xen hypercall preemption

[Per Bilse <Per.Bilse@xxxxxxxxxx>]

On 6/21/2023 5:40 PM, Peter Zijlstra wrote:
> I don't understand it -- fundamentally, how can linux schedule when the
> guest isn't even running? Hypercall transfers control to the
> host/hypervisor and leaves the guest suspended.

Hi Peter, as noted in earlier note to Andy, this is essentially existing
code that other commits have rendered ineffective over time.  Hence,
the finer details of how or why it works haven't changed since it was
first introduced.

> This makes no sense; the race that warning warns about is:
> 
>       CPU0                    CPU1
>       per-cpu write
>       <preempt-out>
>                               <preempt-in>
>                               do-hypercall
> 
> So you wrote the value on CPU0, got migrated to CPU1 because you had
> preemptioned enabled, and then continue with the percpu value of CPU1
> because that's where you're at now.

This issue was raised internally, and it was noted that the only way
for the preemptible code to switch task is via an interrupt that goes
through xen_pv_evtchn_do_upcall(), which handles this.  I'm happy to
check with my sources, but it's holiday season right now.

>> 4) Update irqentry_exit_cond_resched() to raw_irqentry_exit_cond_resched().
>> The code will call irqentry_exit_cond_resched() if the flag (as noted
>> above) is set, but the dynamic preemption feature will livepatch that
>> function to a no-op unless full preemption is selected.  The code is
>> therefore updated to call raw_irqentry_exit_cond_resched().
> 
> That, again meeds more explanation. Why do you want this if not
> preemptible?

I'm not quite sure what you mean here.  Dynamic preemption
will livepatch irqentry_exit_cond_resched() to be a no-op, while
raw_irqentry_exit_cond_resched() remains functional.  This was 
introduced in commit 4624a14f4daa last year which was said to fix
the problem, but doesn't.  You may remember, it was signed off by 
yourself and Mark Rutland.

> You're doing 4 things, that should be 4 patches. Also, please give more
> clues for how this is supposed to work at all.

I respectfully have to disagree with that.  The fixes here are very
closely related, and we're not introducing anything new, we're merely
re-enabling code which has been rendered ineffective due to oversights
in commits made after the code was first introduced.  How the code is
supposed to work hasn't changed, and is beyond the scope of these fixes;
I'm sure it must have been discussed at great length at the time (commit 
fdfd811ddde3).

Best,

   -- Per