SMP: only enter scheduler if node holds BKL

[andybui01]

In the IPI remote call paths where target nodes enter the kernel under the protection of the calling core's BKL, the scheduler may be invoked after handling the interrupt. The MCS kernel protects against this by doing a check, however, the commit that added this check did not do the same for the non-MCS kernel, so we remove the ifdef and check for both MCS and non-MCS.

[lsf37]

Nice find. Do you have a test case that fails before and passes after?

[lsf37]

I'm not sure the change achieves what the commit message says, though. It looks to me like this this now ends up calling the scheduler for any interrupt, not just for IPIs or on SMP configs. E.g. the

if (SMP_TERNARY(clh_is_self_in_queue(), 1))

translates to just

if (1) {

in the verified unicore configs.

This might already be unintended on MCS, I'm not clear on that.

[lsf37]

Ignore my ramblings. We do want the scheduler to always be called on unicore, so this is fine -- it already is being called, I just misread the diff.

[JE-Archer]

I could be wrong about this but I don't think there is a problem. On non-MCS, if NODE_STATE(ksSchedulerAction) == SchedulerAction_ResumeCurrentThread then schedule() and activateThread() won't do anything. The scheduler action can only be changed cross-core by a reschedule IPI and before a remote core operates on the current TCB it will do a remote stall IPI, so changes to the relevant scheduler action and TCB state will be synchronised with handling the RPC IPI. So it should be the case that ksSchedulerAction is set to resume current thread. Maybe assert(clh_is_self_in_queue() || NODE_STATE(ksSchedulerAction) == SchedulerAction_ResumeCurrentThread) would make things clearer. But checking if the lock is held might be even better.

[andybui01]

Agree with @jearc. This might not currently break anything, however, IMO it is assumed that the scheduler is called under the protection of the BKL. We should make this explicit with either an assertion/check, and rely less on everything playing nicely (such as the scheduler action only being SchedulerAction_ResumeCurrentThread )

[lsf37]

Agree with @jearc. This might not currently break anything, however, IMO it is assumed that the scheduler is called under the protection of the BKL. We should make this explicit with either an assertion/check, and rely less on everything playing nicely (such as the scheduler action only being SchedulerAction_ResumeCurrentThread )

I'm happy with that line of reasoning. If we ever get to verify the SMP config, this would make it easier. @kent-mcleod any concerns from your side?

[Indanz]

The commit that added this if-check is 798fb76. I guess that MCS and SMP were added around the same time and that SMP issues found were fixed for MCS-only sometimes, like here.

[lsf37]

The proof test fails, which means this will need a (hopefully trivial) proof update before it can get merged.

[kent-mcleod]

IpiRemoteCall_VMCheckBoundNotification on x86 looks like it is relying on the call to schedule on the way out of the ipi. So there's potentially impact there if this were to be changed (and maybe it's broken on MCS as it is and could explain some MCS + SMP odd behavior reported in the past: #118 (comment)).

[andybui01]

I suppose the performance penalty for a IpiRemoteCall_VMCheckBoundNotification + irq_reschedule_ipi would be too big?

The MCS + SMP case here looks like it's incorrect and presents a race condition, as the target core's scheduling queues become a critical section.

[chrisguikema]

@andybui01 can you expand upon this a bit? I'm actually investigating a problem where a VM is getting starved of execution time, and as far as I can tell, the userspace VCPU execution loops aren't the culprit.

[andybui01]

I'm not really familiar with the x86 virtualization support but here's what I recall: currently the VMCheckBoundNotification() function that runs on the destination core may ask for a switch to a new thread. This new thread gets scheduled and activated in handleInterruptEntry@syscall.c. This can work because remote call IPIs operate with the acquired BKL of the calling core.

If we follow this PR so that schedule() checks that the current core holds the BKL, this IPI will break.

[Indanz]

@andybui01 can you expand upon this a bit? I'm actually investigating a problem where a VM is getting starved of execution time, and as far as I can tell, the userspace VCPU execution loops aren't the culprit.

If you are running multiple tasks on the same core that are woken up by notifications, perhaps #902 is related. To quickly test if this is the cause, enable signal fast paths and change the SCHED_ENQUEUE with SCHED_ENQUEUE at

seL4/src/fastpath/fastpath.c

Line 714 in 0260fba

SCHED_APPEND(dest);

But try removing the whole if-check and calling schedule unconditionally first.

I'm not really familiar with the x86 virtualization support but here's what I recall: currently the VMCheckBoundNotification() function that runs on the destination core may ask for a switch to a new thread. This new thread gets scheduled and activated in handleInterruptEntry@syscall.c. This can work because remote call IPIs operate with the acquired BKL of the calling core.

If we follow this PR so that schedule() checks that the current core holds the BKL, this IPI will break.

Everyone takes the lock if IPI is not a remote call, but only x86 wants a schedule if the remote call is IpiRemoteCall_VMCheckBoundNotification.

I don't understand why x86 has a IpiRemoteCall_VMCheckBoundNotification IPI instead of just using the reschedule IPI.

[chrisguikema]

This is an x86 system, so the Notification fastpath isn't an option I can work with.