git: 44121a0fbee0 - main - amd64: fix tlb shootdown when all cpus are passed in the bitmap

[Mateusz Guzik]

On 1/12/21, John Baldwin <jhb at freebsd.org> wrote:
> On 1/12/21 12:54 AM, Mateusz Guzik wrote:
>> The branch main has been updated by mjg:
>>
>> URL:
>> https://cgit.FreeBSD.org/src/commit/?id=44121a0fbee0272e23bfb90601d177ba9e9cda9b
>>
>> commit 44121a0fbee0272e23bfb90601d177ba9e9cda9b
>> Author:     Mateusz Guzik <mjg at FreeBSD.org>
>> AuthorDate: 2021-01-12 08:47:32 +0000
>> Commit:     Mateusz Guzik <mjg at FreeBSD.org>
>> CommitDate: 2021-01-12 08:47:32 +0000
>>
>>     amd64: fix tlb shootdown when all cpus are passed in the bitmap
>>
>>     Right now the routine leaves the current CPU in the map, later
>> tripping
>>     on an assert when filling in the scoreboard: panic: IPI scoreboard is
>>     zero, initiator 1 target 1
>>
>>     Instead pre-check if all CPUs are present in the map and remember
>> that
>>     outcome for later.
>>
>>     Fixes:  7eaea04a5bb1dc86 ("amd64: compare TLB shootdown target to
>> all_cpus")
>>     Reviewed by:    kib
>>     Differential Revision:  https://reviews.freebsd.org/D28111
>> ---
>>  sys/amd64/amd64/mp_machdep.c | 15 ++++++---------
>>  1 file changed, 6 insertions(+), 9 deletions(-)
>>
>> diff --git a/sys/amd64/amd64/mp_machdep.c b/sys/amd64/amd64/mp_machdep.c
>> index 794a11bf1276..aa62c2086fa4 100644
>> --- a/sys/amd64/amd64/mp_machdep.c
>> +++ b/sys/amd64/amd64/mp_machdep.c
>> @@ -660,6 +660,7 @@ smp_targeted_tlb_shootdown(cpuset_t mask, pmap_t pmap,
>> vm_offset_t addr1,
>>  	 * (zeroing slot) and reading from it below (wait for
>>  	 * acknowledgment).
>>  	 */
>> -	if (!CPU_CMP(&mask, &all_cpus)) {
>> +	if (is_all) {
>>  		ipi_all_but_self(IPI_INVLOP);
>>  		other_cpus = all_cpus;
>>  		CPU_CLR(PCPU_GET(cpuid), &other_cpus);
>
> At this point isn't other_cpus == mask, so we could just drop other_cpus and
> use 'mask'
> directly now?
>

The previous iteration is:

        while ((cpu = CPU_FFS(&mask1)) != 0) {
                cpu--;
                CPU_CLR(cpu, &mask1);
                KASSERT(*invl_scoreboard_slot(cpu) != 0,
                    ("IPI scoreboard is zero, initiator %d target %d",
                    PCPU_GET(cpuid), cpu));
                *invl_scoreboard_slot(cpu) = 0;
        }

and the one after that:

        while ((cpu = CPU_FFS(&other_cpus)) != 0) {
                cpu--;
                CPU_CLR(cpu, &other_cpus);
                p_cpudone = invl_scoreboard_slot(cpu);
                while (atomic_load_int(p_cpudone) != generation)
                        ia32_pause();
        }

iow when someone(tm) implements CPU_FOREACH_MASK or similar instead of
needlessly modifying it the code will get itself a good cleanup.

The point of the change at hand was to fix the immediate problem.
-- 
Mateusz Guzik <mjguzik gmail.com>