V2 was Re: Original cpuset.h for review
Sebastian Huber
sebastian.huber at embedded-brains.de
Wed Oct 16 07:49:09 UTC 2013
On 2013-10-15 16:26, Joel Sherrill wrote:
> Attached is version 2. All comments should be addressed
> and a bug fixed.
>
> Also.. the Linux and BSD versions are all macros. How do you all
> feel about them being static inlines instead?
I prefer inline functions for all function-like macros provided they have no
use case for static initialization.
>
> On 10/15/2013 1:00 AM, Thomas Doerfler wrote:
>> >Hi Joel,
>> >
>> >just some comments from me:
> Thanks. I added Gedare's below so this thread is all together.
>
>> >1.) In the typedef for cpu_set_t, it might be better to use the
>> >_NCPUWORDS macro instead of duplicating its expansion
> Nice call. That's what I get for looking at the FreeBSD code.:)
>
>> >2.) In the operations _OR, _AND etc, you used "(_set)" instead of
>> >"(_src2)" for the second operand
> Got it. I have some test code but it hasn't gotten to those yet.
>
>> >3.) The term "(_cpu)/_NCPUBITS" is duplicated to get the index into the
>> >cpuset table. This might be done through a macro (__cpu_set_idx ?) aswell.
> I wondered this about half way through. Neither the BSD nor Linux
> code do this. Done.
>
>> >Gedare: Pretty reasonable at first skimming. You could use the _NCPUWORDS
>> >macro in the definition of __bits[] array in cpu_set_t.
> Thomas also spotted this. See above. Fixed.
>
>> >Gedare: Not sure where
>> >the NBBY value comes from, is there an implicit dependency header file
>> >missing?
> sys/types.h - The test I started includes pthread.h.
>
> I just added a simpler test which just compiles a
> file including cpuset.h. I added sys/types.h to get
> it to compile.
>
> Should the include be inside or outside the inclusion
> guard?
It should be inside.
>
> --joel
>
>> >wkr,
>> >
>> >Thomas.
>> >
>> >
>> >Am 14.10.2013 20:47, schrieb Joel Sherrill:
>>> >>Hi
>>> >>
>>> >>This has had limited review but should close if you ignore
>>> >>that I need to add some comments.
>>> >>
>>> >>This will go into the RTEMS specific subdirectory of
>>> >>newlib and be accompanied by prototypes of various
>>> >>pthread methods that use this. This is just the
>>> >>first step.
>>> >>
>>> >>Comments.
>>> >>
>>> >>
>>> >>
>>> >>_______________________________________________
>>> >>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
>
>
> cpuset.h
>
>
> /*
> * Copyright (c) 2013 On-Line Applications Research Corporation.
> * All rights reserved.
> *
> * On-Line Applications Research Corporation
> * 7047 Old Madison Pike Suite 320
> * Huntsville Alabama 35806
> *<info at oarcorp.com>
> *
> * Redistribution and use in source and binary forms, with or without
> * modification, are permitted provided that the following conditions
> * are met:
> * 1. Redistributions of source code must retain the above copyright
> * notice, this list of conditions and the following disclaimer.
> * 2. Redistributions in binary form must reproduce the above copyright
> * notice, this list of conditions and the following disclaimer in the
> * documentation and/or other materials provided with the distribution.
> *
> * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
> * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
> * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
> * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
> * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
> * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> * SUCH DAMAGE.
> */
>
> /*
> * This file implements an API compatible with static portion of
> * the GNU/Linux cpu_set_t macros but is independently implemented.
> * The GNU/Linux manual page and the FreeBSD cpuset_t implementation
> * were used as reference material.
> *
> * Not implemented:
> * + Linux CPU_XXX_S
> * + FreeBSD CPU_SUBSET
> * + FreeBSD CPU_OVERLAP
> */
>
> #ifndef_SYS_CPUSET_H_
> #define_SYS_CPUSET_H_
>
> #include <sys/types.h>
>
> #define CPU_SETSIZE 32
I would use
#ifndef CPU_SETSIZE
#define CPU_SETSIZE 32
#endif
so that a user can change the default size.
I would use uint32_t instead of long. We may need the ability to statically
initialize such sets. One use case could be the RTEMS configuration table for
SMP. Currently we can only say, that we want to run on N processors 0 up to N
- 1. This should be more flexible, e.g. we want to run on processors B, C and
E, but not A and D.
>
> #define _NCPUBITS (sizeof(long) * NBBY) /* bits per mask */
> #define _NCPUWORDS howmany(CPU_SETSIZE, _NCPUBITS)
>
> typedef struct _cpuset {
> long __bits[_NCPUWORDS];
> } cpu_set_t;
>
> #define __cpuset_mask(_cpu) ((long)1 << ((_cpu) % _NCPUBITS))
> #define __cpuset_index(_cpu) ((_cpu)/_NCPUBITS)
>
> /* zero out _set */
> #define CPU_ZERO(_set) \
> do { \
> size_t _i; \
> for (_i = 0; _i < _NCPUWORDS; _i++) \
> (_set)->__bits[_i] = 0; \
> } while (0)
>
> #define CPU_FILL(_set) \
> do { \
> size_t _i; \
> for (_i = 0; _i < _NCPUWORDS; _i++) \
> (_set)->__bits[_i] = -1; \
> } while (0)
I would use memset() in the the above functions to make it a bit easier for the
compiler to optimize this.
>
> #define CPU_SET(_cpu, _set) \
> do { \
> ((_set)->__bits[__cpuset_index(_cpu)] |= __cpuset_mask(_cpu)); \
> } while (0)
>
> #define CPU_CLR(_cpu, _set) \
> do { \
> ((_set)->__bits[__cpuset_index(_cpu)] &= ~__cpuset_mask(_cpu)); \
> } while (0)
>
> /* Return 1 is _cpu is set in _set, 0 otherwise */
> #define CPU_ISSET(_cpu, _set) \
> (((_set)->__bits[__cpuset_index(_cpu)] & __cpuset_mask(_cpu)) != 0)
>
> #define CPU_COPY(_dest, _src) \
> (*(t) = *(f))
>
> #define CPU_AND(_dest, _src1, _src2) \
> do { \
> size_t _i; \
> for (_i = 0; _i < _NCPUWORDS; _i++) \
> (_dest)->__bits[_i] = (_src1)->__bits[_i] & (_src2)->__bits[_i]; \
> } while (0)
>
> #define CPU_OR(_dest, _src1, _src2) \
> do { \
> size_t _i; \
> for (_i = 0; _i < _NCPUWORDS; _i++) \
> (_dest)->__bits[_i] = (_src1)->__bits[_i] | (_src2)->__bits[_i]; \
> } while (0)
>
> #define CPU_XOR(_dest, _src1, _src2) \
> do { \
> size_t _i; \
> for (_i = 0; _i < _NCPUWORDS; _i++) \
> (_dest)->__bits[_i] = (_src1)->__bits[_i] ^ (_src2)->__bits[_i]; \
> } while (0)
>
> #define CPU_NAND(_dest, _src1, _src2) \
> do { \
> size_t _i; \
> for (_i = 0; _i < _NCPUWORDS; _i++) \
> (_dest)->__bits[_i] = (_src1)->__bits[_i] & ~(_src2)->__bits[_i]; \
> } while (0)
>
> static inline int CPU_COUNT(cpu_set_t *set)
We should use the const qualifier if possible.
> {
> size_t i;
> int count = 0;
>
> for (i=0; i < _NCPUWORDS; i++)
> if (CPU_ISSET(i, set) != 0)
> count++;
> return count;
> }
>
> static inline int CPU_EQUAL(cpu_set_t *set1, cpu_set_t *set2)
> {
> size_t i;
>
> for (i=0; i < _NCPUWORDS; i++)
> if (set1->__bits[i] != set2->__bits[i] )
> return 0;
> return 1;
> }
I would use memcmp() here.
>
> #define CPU_CMP(_set1, _set2) \
> CPU_EQUAL(_set1, _set2)
>
> static inline int CPU_EMPTY(cpu_set_t *set)
> {
> size_t i;
>
> for (i=0; i < _NCPUWORDS; i++)
> if (set->__bits[i] != 0 )
> return 0;
> return 1;
> }
>
> #endif
>
>
>
> _______________________________________________
> rtems-devel mailing list
> rtems-devel at rtems.org
> http://www.rtems.org/mailman/listinfo/rtems-devel
>
--
Sebastian Huber, embedded brains GmbH
Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone : +49 89 189 47 41-16
Fax : +49 89 189 47 41-09
E-Mail : sebastian.huber at embedded-brains.de
PGP : Public key available on request.
Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.
More information about the devel
mailing list