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 "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.) 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. +#if __RTEMS_HAVE_SYS_CPUSET_H__ I think we decided to prefer the more explicit "#if defined(...)" for checking for CPP defines. Also, what is this define checking against / where is it defined? _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. 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. +#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.)? +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? On Thu, Feb 20, 2014 at 3:07 PM, Jennifer Averett <jennifer.aver...@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@rtems.org > http://www.rtems.org/mailman/listinfo/rtems-devel > _______________________________________________ rtems-devel mailing list rtems-devel@rtems.org http://www.rtems.org/mailman/listinfo/rtems-devel
