> -----Original Message----- > From: Joel Sherrill [mailto:joel.sherr...@oarcorp.com] > Sent: Thursday, February 20, 2014 4:23 PM > To: Gedare Bloom; Jennifer Averett > Cc: rtems-devel@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.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
