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. 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.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 -- Joel Sherrill, Ph.D. Director of Research & Development joel.sherr...@oarcorp.com On-Line Applications Research Ask me about RTEMS: a free RTOS Huntsville AL 35805 Support Available (256) 722-9985 _______________________________________________ rtems-devel mailing list rtems-devel@rtems.org http://www.rtems.org/mailman/listinfo/rtems-devel
