From owner-dev-commits-src-main@freebsd.org Tue Jan 12 22:22:42 2021 Return-Path: Delivered-To: dev-commits-src-main@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 883D24E84AD; Tue, 12 Jan 2021 22:22:42 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-wm1-x32c.google.com (mail-wm1-x32c.google.com [IPv6:2a00:1450:4864:20::32c]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4DFlSf3G3sz4mTQ; Tue, 12 Jan 2021 22:22:42 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by mail-wm1-x32c.google.com with SMTP id k10so3268490wmi.3; Tue, 12 Jan 2021 14:22:42 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=UTo/iJHSXt3qNECSM519+6wJ0PTYKh+FpsOnOHYv3ls=; b=HJW9vuYePylmsKfqkNeQK7+fpcfYgNOQgDgpw6KrkXz+VcUStMMbvllExb+EENNyyt iT7nNAThqiQ4Jya+Wt+8tM9G2quRneOFtmwcPYJbKUsD0On4/TX87779XIkYSxHR7z6H brPYH2CWwMRFkYL4KkN8IVn+k1xS8EphCdIGcbWUob4id+rFaF6Ia06fe63um3BCGPPJ XNhbc4cq5cZXzm613bYE+sGtpLTsQj9weiBcCwcgomCq41HZURoPcx2CjzCySq+4aDE9 vFWdSFPbFQJmaDzEELz7dcxMRqrVxdNPUC4liqNYSn0ZRl9ZtCsZp2CPfJ359X0rpN1C hZ8Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=UTo/iJHSXt3qNECSM519+6wJ0PTYKh+FpsOnOHYv3ls=; b=JeKhdGdkC3drd/47HYbBUAls9phpDr8SPXA9//lEzpoCdUXdk+ysXDuQ69LujjgJxx XeZUlP4b15771YjcZ6fu/QFNT4GEl7RR/lodbsWYkDskLO3ChS0BLlVX0WPvgvoRH3S7 bruhaM7QYqy8YcOYT2A2f5rlovNjF/vnXPw6hYSs6tLK2SwpQRZBCpjMwsfu8ThLWqSJ 9gJGK4nkzMCifdD7wC6z3zx2+1m5Wavwem9Gj1MSj1nK79jN7H0ge7KW7n9BPRxWO6H2 FzKVjpYqsiFJ5fKCUni66JolWZKgOqV1jexWfIyJ2+2zNgiwp3FC5hvAD7/c8tVlL45f 13Uw== X-Gm-Message-State: AOAM530+M70K7V29v9S3qiSEOwMryQc96OAB3H7+lhGr6jCpZDNqDvkN Y1u2mbfH6b3hq+R8gPWPde8TgBP/e7ZLMan7W0G+FwYd9ts= X-Google-Smtp-Source: ABdhPJy2p6JF3NUagCdDJsoS8RLIczaxCi0mpzlOBt5Sn1sHGuaXhZjTD0JrwXe+JdMajY8/99ZUszPpK5+PWyVMCjw= X-Received: by 2002:a1c:68d5:: with SMTP id d204mr1218495wmc.178.1610490159741; Tue, 12 Jan 2021 14:22:39 -0800 (PST) MIME-Version: 1.0 Received: by 2002:adf:f811:0:0:0:0:0 with HTTP; Tue, 12 Jan 2021 14:22:38 -0800 (PST) In-Reply-To: <912ef087-e9e8-1bb5-5063-88bccb834db1@FreeBSD.org> References: <202101120854.10C8s6Gr075398@gitrepo.freebsd.org> <912ef087-e9e8-1bb5-5063-88bccb834db1@FreeBSD.org> From: Mateusz Guzik Date: Tue, 12 Jan 2021 23:22:38 +0100 Message-ID: Subject: Re: git: 44121a0fbee0 - main - amd64: fix tlb shootdown when all cpus are passed in the bitmap To: John Baldwin Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: 4DFlSf3G3sz4mTQ X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] X-BeenThere: dev-commits-src-main@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for the main branch of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 12 Jan 2021 22:22:42 -0000 On 1/12/21, John Baldwin 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 >> AuthorDate: 2021-01-12 08:47:32 +0000 >> Commit: Mateusz Guzik >> 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