Page MenuHomeFreeBSD

libthr: Fix pthread_attr_[g|s]etaffinity_np to match it's manual and kernel.
ClosedPublic

Authored by dchagin on Jan 18 2023, 7:27 PM.
Tags
None
Referenced Files
Unknown Object (File)
Oct 1 2024, 6:06 PM
Unknown Object (File)
Oct 1 2024, 6:06 PM
Unknown Object (File)
Oct 1 2024, 6:06 PM
Unknown Object (File)
Sep 25 2024, 3:38 AM
Unknown Object (File)
Sep 24 2024, 8:14 PM
Unknown Object (File)
Sep 24 2024, 7:26 PM
Unknown Object (File)
Sep 24 2024, 8:17 AM
Unknown Object (File)
Sep 22 2024, 5:45 PM
Subscribers

Details

Summary

Since f35093f8 Linux semantics of a threads affinity functions is in use:

Minimum cpuset_t size that the Linux kernel permits in case of getaffinity()
is the maximum CPU id, present in the system / NBBY, the maximum size is not
limited. For setaffinity(), Linux does not limit the size of the user-provided
cpuset_t, internally using only the meaningful part of the set, where the upper
bound is the maximum CPU id, present in the system, no larger than the size of
the kernel cpuset_t.

Test Plan

glibc affinity tests

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

dchagin added reviewers: kib, jhb.
lib/libthr/thread/thr_attr.c
609

This doesn't seem quite right. You need to roundup to be a multiple of longs? That is, this can come up with a value like "3" if the maxid is 20, but 3 is not a valid cpuset size AFAIK. I think you probably want something more like:

roundup2(howmany(maxid, NBBY), sizeof(long));

All that said, is there really a reason to not use kern.sched.cpusetsize? The kernel can choose to define that value dynamically using a formula like the above in the kernel, and I think I would prefer the kernel to export the right size for userland to use rather than userland doing the math.

lib/libthr/thread/thr_attr.c
609

This doesn't seem quite right. You need to roundup to be a multiple of longs? That is, this can come up with a value like "3" if the maxid is 20, but 3 is not a valid cpuset size AFAIK. I think you probably want something more like:

roundup2(howmany(maxid, NBBY), sizeof(long));

All that said, is there really a reason to not use kern.sched.cpusetsize? The kernel can choose to define that value dynamically using a formula like the above in the kernel, and I think I would prefer the kernel to export the right size for userland to use rather than userland doing the math.

At first I did as you suggest )) Maybe don't touch kern.sched.cpusetsize, make a new sysctl - kern.sched.cpusetsizemin?

Export the right size of cpuset from the kernel and use it,
Im not rounding here because this would be incompatible with the current implementation
of cpuset_setaffinity() syscall and Linux.

lib/libthr/thread/thr_attr.c
606

This mean that slightly newer libthr on slightly older kernel would terminate process. Could you provide some (non-optimal) backward compat to easier moving installation past the flag day?

sys/kern/kern_cpuset.c
148

You might just initialize some int variable by the formula after smp started. Then you could use SYSCTL_UINT instead of having proc.

160

mp_maxid is id of the last CPU, not the number of CPUs in the system. So for e.g. UP kernel mp_maxid is zero.

lib/libthr/thread/thr_attr.c
606
if (sysctlbyname() != 0 && sysctlbyname() != 0)
      PANIC();
sys/kern/kern_cpuset.c
161

Why not do this directly in mp_setmaxid(). This would eliminate implicit dependency, provided by SI_ORDER_ANY.

Also I am somewhat worried about the comment above mp_maxid();

/*
 * Let the MD SMP code initialize mp_maxid very early if it can.
 */
static void
mp_setmaxid(void *dummy)
sys/kern/kern_cpuset.c
161

hmm, mp_setmaxid() is under ifdef SMP

fixed, init cpusetsizemin to 1 for UP kernels

dchagin added inline comments.
sys/kern/kern_cpuset.c
161

Why not do this directly in mp_setmaxid(). This would eliminate implicit dependency, provided by SI_ORDER_ANY.

Also I am somewhat worried about the comment above mp_maxid();

/*
 * Let the MD SMP code initialize mp_maxid very early if it can.
 */
static void
mp_setmaxid(void *dummy)

It looks mp_setmaxid() SHOULD initialize mp_maxid, so "if it can" part is wrong.

kib added inline comments.
sys/sys/smp.h
180 ↗(On Diff #115971)

I suggest moving this declaration into sys/cpuset.h _KERNEL block.

This revision is now accepted and ready to land.Jan 28 2023, 6:28 PM