Bug 264383 - SMP: Miss access to a global variable in `smp_rendezvous_action` function.
Summary: SMP: Miss access to a global variable in `smp_rendezvous_action` function.
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: Mark Johnston
URL:
Keywords: needs-qa
Depends on:
Blocks:
 
Reported: 2022-06-01 02:43 UTC by Yuichiro NAITO
Modified: 2022-06-13 12:47 UTC (History)
5 users (show)

See Also:
koobs: maintainer-feedback? (markj)
koobs: maintainer-feedback? (mjg)
koobs: maintainer-feedback? (kib)
koobs: maintainer-feedback? (jhb)
koobs: mfc-stable13?
koobs: mfc-stable12?

Attachments
smp_rendezvous_action.patch (574 bytes, patch)
2022-06-01 02:43 UTC, Yuichiro NAITO 		no flags Details | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Yuichiro NAITO 2022-06-01 02:43:46 UTC 
Created attachment 234367 [details]
smp_rendezvous_action.patch

I'm studying `smp_rendezvous` code now.
It's a cool feature in the kernel and surprising how long history it has.

It seems that arguments of `smp_rendezvous_action` is loaded in local variables
as soon as `smp_rendezvous_action` takes a barrier.
And the code design seems to use local variables in the barrier.

But I think only two lines violate to the design policy
and should be fixed as attached `smp_rendezvous_action.patch`.

I believe there is no issues about this.
`smp_rendezvous` works fine for a long time.

I think local variable access is a little bit faster than global variable access.
Because the global variables are defined as `volatile`.
Comment 1 Kubilay Kocak freebsd_committer freebsd_triage 2022-06-01 02:56:36 UTC 
^Triage: Can't identify a suitable key maintainer to this component, request feedback from a few folks who have played with, fixed or improved rendezvous in the recent(ish) past.
Comment 2 Mark Johnston freebsd_committer freebsd_triage 2022-06-01 12:39:50 UTC 
The patch looks right to me.  I'll test and commit it within the next couple of days if no one objects.
Comment 3 John Baldwin freebsd_committer freebsd_triage 2022-06-01 21:04:08 UTC 
This seems fine to me.  I think it was just a simple oversight in the initial commit that added the locals (f53d15fe1b8a7a7df972f253b38c698f6ca45b71f53d15fe1b8a7a7df972f253b38c698f6ca45b71)
Comment 4 commit-hook freebsd_committer freebsd_triage 2022-06-06 15:30:17 UTC 
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=8d95f500521128c2b40270bdd703a54b56dbccf2

commit 8d95f500521128c2b40270bdd703a54b56dbccf2
Author:     Yuichiro NAITO <naito.yuichiro@gmail.com>
AuthorDate: 2022-06-06 15:21:33 +0000
Commit:     Mark Johnston <markj@FreeBSD.org>
CommitDate: 2022-06-06 15:29:51 +0000

    smp: Use local copies of the setup function pointer and argument

    No functional change intended.

    PR:             264383
    Reviewed by:    jhb, markj
    MFC after:      1 week

 sys/kern/subr_smp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Comment 5 commit-hook freebsd_committer freebsd_triage 2022-06-13 12:43:59 UTC 
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=a39788bd281c3b297be37a2df79594c5cbb3ded8

commit a39788bd281c3b297be37a2df79594c5cbb3ded8
Author:     Yuichiro NAITO <naito.yuichiro@gmail.com>
AuthorDate: 2022-06-06 15:21:33 +0000
Commit:     Mark Johnston <markj@FreeBSD.org>
CommitDate: 2022-06-13 12:43:06 +0000

    smp: Use local copies of the setup function pointer and argument

    No functional change intended.

    PR:             264383
    Reviewed by:    jhb, markj

    (cherry picked from commit 8d95f500521128c2b40270bdd703a54b56dbccf2)

 sys/kern/subr_smp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Comment 6 Mark Johnston freebsd_committer freebsd_triage 2022-06-13 12:47:52 UTC 
Thanks for the patch.