Hi Mark, On Apr 28 22:38, Mark Geisert wrote: > There are a couple of multi-group affinity operations that cannot be done > without heroic measures. Those are marked with XXX in the code. Further > discussion would be helpful to me. > > --- > newlib/libc/include/sched.h | 13 ++ > winsup/cygwin/common.din | 4 + > winsup/cygwin/include/pthread.h | 2 + > winsup/cygwin/sched.cc | 237 ++++++++++++++++++++++++++++++++ > winsup/cygwin/thread.cc | 19 +++ > 5 files changed, 275 insertions(+) > > diff --git a/newlib/libc/include/sched.h b/newlib/libc/include/sched.h > index 1016235bb..a4d3fea6a 100644 > --- a/newlib/libc/include/sched.h > +++ b/newlib/libc/include/sched.h > @@ -92,6 +92,19 @@ int sched_yield( void ); > > #if __GNU_VISIBLE > int sched_getcpu(void); > + > +#ifdef __CYGWIN__
I don't think we really need that extra ifdef. #if __GNU_VISIBLE
bracketing is sufficient.
> +static int
> +whichgroup (size_t sizeof_set, const cpu_set_t *set)
> +{
> + //XXX code assumes __get_cpus_per_group() is fixed at 64
I don't understand this comment. It could also return 48 or 36
or any other value <= 64. Care to explain?
Oh and please keep in mind that 32 bit systems only support 32 CPUs, not
64 (sizeof(KAFFINITY) == 4 on 32 bit). I don't think this has much
influence on the code, if any, but it might be worthwhile to check the
code on any assumptions about the size of the affinity mask.
> + // There is no GetThreadAffinityMask() function, so simulate one by
> + // iterating through CPUs trying to set affinity, which returns the
> + // previous affinity. On success, restore original affinity.
> + // This strategy is due to Damon on StackOverflow.
Can you please use /* ... */ style comments? // style comments should
only be used for short, single-line comments after an expression.
Also, while there's no GetThreadAffinityMask() function, there is
an equivalent NT level function which allows to fetch the thread
affinity without having to manipulate it:
THREAD_BASIC_INFORMATION tbi;
KAFFINITY affinity_mask;
NtQueryInformationThread (thread_handle, ThreadBasicInformation,
&tbi, sizeof tbi, NULL);
affinity_mask = tbi.AffinityMask;
All required definitions already exist in ntdll.h and are used in
fhandler_process.cc, just for another purpose.
> +int
> +sched_getaffinity (pid_t pid, size_t sizeof_set, cpu_set_t *set)
> +{
> + HANDLE process = 0;
> + int status = 0;
> +
> + //XXX code assumes __get_cpus_per_group() is fixed at 64
> + pinfo p (pid ? pid : getpid ());
> + if (p)
> + {
> + process = pid && pid != myself->pid ?
> + OpenProcess (PROCESS_QUERY_LIMITED_INFORMATION, FALSE,
> + p->dwProcessId) : GetCurrentProcess ();
> + KAFFINITY procmask;
> + KAFFINITY sysmask;
> +
> + if (!GetProcessAffinityMask (process, &procmask, &sysmask))
> + {
> +oops:
> + status = geterrno_from_win_error (GetLastError (), EPERM);
> + goto done;
> + }
> + memset (set, 0, sizeof_set);
> + if (wincap.has_processor_groups ())
> + {
> + USHORT groupcount = CPU_GROUPMAX;
> + USHORT grouparray[CPU_GROUPMAX];
> +
> + if (!GetProcessGroupAffinity (process, &groupcount, grouparray))
> + goto oops;
Uhm... that's a bit ugly, imho. Rather than just jumping wildly to
another spot in the middle of the same function, create a matching label
at the end of the function. Or, in a simple case like this one, just
call geterrno_from_win_error here and goto done.
> + if (groupcount == 1)
> + set[grouparray[0]] = procmask;
> + else
> + status = ENOSYS;//XXX multi-group code TBD...
> + // There is no way to assemble the complete process affinity mask
> + // without querying at least one thread per group in grouparray,
> + // and we don't know which group a thread is in without querying
> + // it, so must query all threads. I'd call that a heroic measure.
I don't understand. GetProcessAffinityMask() exists. Am I missing
something? Also, if you don't like GetProcessAffinityMask(), there's an
equivalent NT function to NtQueryInformationThread:
PROCESS_BASIC_INFORMATION pbi;
KAFFINITY affinity_mask;
NtQueryInformationProcess (process_handle, ProcessBasicInformation,
&pbi, sizeof pbi, NULL);
affinity_mask = pbi.AffinityMask;
> + }
> + else
> + set[0] = procmask;
> + }
> + else
> + status = ESRCH;
> +
> +done:
> + if (process && process != GetCurrentProcess ())
> + CloseHandle (process);
> +
> + return status;
> +}
> +[...]
Other than that, the code looks good to me.
Thanks,
Corinna
--
Corinna Vinschen
Cygwin Maintainer
signature.asc
Description: PGP signature
