Patches for classic affinity implementation and test

Fri Feb 21 01:55:36 UTC 2014

On Thu, Feb 20, 2014 at 5:22 PM, Joel Sherrill
<joel.sherrill at oarcorp.com> wrote:
>
> On 2/20/2014 3:11 PM, Gedare Bloom wrote:
>> I mostly focused on the 0002-score... file. The others seemed ok at a
>> glance... with respect to the 0002 patch I have the following:
>>
>> Copyright should only be applied to code from the time it is written.
>> We should avoid copy-paste and extending copyright into the past.
>>
>> fix the license address as rtems.org not rtems.com
> Ok. But Chris has a sweep script pending for this and there is a URL
> redirector
> in place for the indefinite future.
Sure, but no need to keep re-introducing it.

>> "thread cpuset Handler" should be given a properly capitalized name,
>> such as "Thread CPU Set Handler". The Doxygen mnemonic should be
>> similarly defined like ScoreThreadCpuSet. However, I think the Thread
>> can be dropped from the name and we just call it the Cpu Set handler.
>>
>> I would prefer to see symmetry between our choice to use cpu_set_t as
>> the opaque type for the cpu bit sets. Thus I recommend using the score
>> name _Cpu_set for the handler. (It is unfortunate that we can't use
>> _CPU_set because that would clash with the score CPU namespace.)
> If we drop the _, it could be _CPUSet_Xxx.
_CPUSet is ok.

>> Is there a reason not to use the same names in the score
>> Cpuset_Control as in the pthread_attr_t? i.e. affinitysetsize,
>> affinityset, and affinitysetpreallocated.
> No. It would be better if they were the same.
>> +#if __RTEMS_HAVE_SYS_CPUSET_H__
>> I think we decided to prefer the more explicit "#if defined(...)" for
>> checking for CPP defines.
> OK. But this is far from universal. There is a warning in gcc which
> warns about this
> and when i tried it, there were >10K warnings.
>> Also, what is this define checking against / where is it defined?
> config.h from cpukit/configure.ac
I'd like to see if this check and the SMP one can be removed and still
have a working cpuset implementation.

cpuset is actually a bitset wrapper that we give the meaning of
affinity in the affinity-aware scheduler implementations. I almost
wonder if offering a generic bitset in the score is better.

>> _Cpuset_Handler_is_valid():
>> Why include "Handler" in the function name? same goes for other
>> functions in this file. The only one that usually gets "Handler" is
>> the initialization function.
> No reason to include it.
>> cpusetimpl.h:
>> this file seemingly includes some public API functions (in the
>> rtems_cpuset namespace), which it should not do. Either make these
>> functions internal to score, or provide them in a classic API handler.
> They are useful helpers and we didn't know what to do. We could make them
> score named and add a wrapper. Is that OK with you?
>> +#if __RTEMS_HAVE_SYS_CPUSET_H__ && defined( RTEMS_SMP )
>> Is there no valid use case for cpu_set in uniprocessor mode? what if
>> an application can work in either uni or multicore setting, and it
>> determines thread placement in multicore with affinity (and in single
>> core it will just end up with one cpu available.)?
> The way the cpu set error checking is defined on Linux, it would be an
> error to
> ever set more than 1 bit. The user APIs COULD be present and the score code
> pretty much be non-existent.
>
I think supporting UP would simplify the code. there would be some
space overhead though the way you have included the affinity field in
the thread definition. My intuition here is that the CPUSet_Control
should be part of the per-thread scheduler data and included only by
schedulers that support affinity.

> I don't know.
>> +static Cpuset_Control _Default_Cpuset;
>> Should use some local/file scope name for this variable if you like.
>>
>> +#if HAVE_SYS_CPUSET_H
>> Here is another define that is different from the others. where is it
>> defined, and do we really need to check for it?
>>
Is this cpp check the same as the one with _RTEMS_ prepended?

>>
>> +  int max_cpus = 1;
>> +
>> +  /* We do not support a cpu count over CPU_SETSIZE  */
>> +  max_cpus = _SMP_Get_processor_count();
>> the initialization of max_cpus = 1 is unnecessary.
>>
>> + * _Affinity_Handler_is_valid
>> ...
>> +int _Cpuset_Handler_is_valid(
>> mismatch between doco and code.
>>
>> for Cpuset_Handler_is_valid() does it make sense to return more
>> informative error codes? I'm aware the posix_nr implementation does
>> not require it, but the classic interface could use it to make easier
>> debugging.
>>
>> Do we need this cpusetprintsupport.c code?
> The tests use it for debug output and it knows the implementation. I
> don't mind
> it being wrapped at the API level and being a score helper.
>
> --joel
>>
>> On Thu, Feb 20, 2014 at 3:07 PM, Jennifer Averett
>> <Jennifer.Averett at oarcorp.com> wrote:
>>> Attached is a set of patches for review that implement classic affinity.
>>>
>>> Jennifer Averett
>>> On-Line Applications Research
>>> _______________________________________________
>>> rtems-devel mailing list
>>> rtems-devel at rtems.org
>>> http://www.rtems.org/mailman/listinfo/rtems-devel
>>>
>> _______________________________________________
>> rtems-devel mailing list
>> rtems-devel at rtems.org
>> http://www.rtems.org/mailman/listinfo/rtems-devel
>
> --
> Joel Sherrill, Ph.D.             Director of Research & Development
> joel.sherrill at OARcorp.com        On-Line Applications Research
> Ask me about RTEMS: a free RTOS  Huntsville AL 35805
> Support Available                (256) 722-9985
>