Patches for classic affinity implementation and test

[Jennifer Averett]

> -----Original Message-----
> From: Joel Sherrill [mailto:joel.sherrill at oarcorp.com]
> Sent: Thursday, February 20, 2014 4:23 PM
> To: Gedare Bloom; Jennifer Averett
> Cc: rtems-devel at rtems.org
> Subject: Re: Patches for classic affinity implementation and test
> 
> 
> 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.
> > "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.
> > 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.

Actually the thread uses the name affinity as the attribute name so
the code usage is thread->affinity.set  and thread->affinity.setpreallocated.
But I can change it.

> 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
> > _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 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?
> >
> >
> > +  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