[PATCH v2] Cygwin: Implement sched_[gs]etaffinity()

Mark Geisert mark@maxrnd.com
Mon Apr 29 08:54:00 GMT 2019


Corinna Vinschen wrote:
> Hi Mark,
>

Howdy!  FTR Here's the intro paragraph left out of my patch submission:

Second version of CPU affinity patch set.  Attempts to mimic operation
of Linux affinity functions, both the sched_* and pthread_* varieties.
This v2 version assumes Windows processor groups always have 64 logical
processors.  I'm just trying to get the control structures laid out.  A
later version will deal with smaller-sized processor groups.

> 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.

This mod is to newlib but I figured it's not relevant to non-Cygwin platforms. 
Could you please confirm the __CYGWIN__ bracketing can be removed?

>
>> +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.

OK on 32 vs 64.  This XXX comment is to remind me to support the smaller 
processor groups before final patch submission.  We have been discussing this 
but I don't think I made it clear I'm considering the "big bitmask" set (like 
Linux uses) and how processor groups subdivide it.  It's an array of cpu_set_t 
(== uint64_t) but when subscripted by group number, it's an array of G-bit 
quantities, where G can be 48 or 36 or ...  Ergo, some bit-aligned reads and 
stores will be needed.

>
>> +	  // 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.

OK, got the general rule now.  Will fix.

> 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.

Very good.  I'll use that.

>> +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.

Sure, will do the call here.  Re the goto, I thought I had learned somewhere 
that if one must use a goto, it's better to goto a label already seen than some 
indeterminate distance forward in the code.  No biggie.

>> +          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:

It exists, but if the process you're querying is a multi-group process, the mask 
is returned as all zeroes.  The func only works for single-group processes.  I 
even use it for such a little earlier in this code.

That doc I referenced in my last submission talks about how support for >64 
logical processors was hacked into Windows to allow pre-existing code to 
continue to work.  The down-side of the hackwork is that one has to manually 
place threads into processor groups other than the one selected by Windows to 
run the primary thread.  You can't change the processor group of the process.

>   PROCESS_BASIC_INFORMATION pbi;
>   KAFFINITY affinity_mask;
>
>   NtQueryInformationProcess (process_handle, ProcessBasicInformation,
> 			     &pbi, sizeof pbi, NULL);
>   affinity_mask = pbi.AffinityMask;

This unfortunately has the same limitation.  It can only return 64 bits of info, 
though a multi-group process might have affinity (through its threads) to more 
than 64 logical processors.

>> +        }
>> +      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
>



More information about the Cygwin-patches mailing list