Re: [Xenomai-core] [PATCH 1/4] Refactor generic system.h

[Jan Kiszka <jan.kiszka@domain.hid>]

[-- Attachment #1: Type: text/plain, Size: 2742 bytes --]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>  > Gilles Chanteperdrix wrote:
>  > > Jan Kiszka wrote:
>  > >  > In order to allow further optimizations of xnlock, I started with
>  > >  > refactoring the related system.h. This improves the readability
>  > >  > significantly, IMHO. It also happen to reduce the text size of
>  > >  > __xnlock_get a bit by avoid redundant rthal_processor_id read-outs.
>  > >  > 
>  > >  > Another quirk I happen to remove: xnlock debugging depends on
>  > >  > XENO_OPT_DEBUG_NUCLEUS, but needlessly we used to pick the debug version
>  > >  > of xnlock_t already with XENO_OPT_DEBUG.
>  > > 
>  > > There is a lot of whitespace change in this patch, which make it hard to
>  > > read.
>  > 
>  > Well, this patch is mostly about whitespace and formatting fixes (among
>  > which ifdef reduction falls for me as well). But I can split it up if
>  > desired.
>  > 
>  > > 
>  > > Anyway, there are a few things I do not like in this patch:
>  > > - macro which make reference to symbols defined elsewhere
>  > 
>  > You mean XNLOCK_DBG_PREPARE_ACQUIRE vs. XNLOCK_DBG_SPINNING/ACQUIRED?
>  > Granted, not nice but so far the most compact approach I found. My goal
>  > was to keep the lock implementations as pure as possible (you can easily
>  > ignore the debug stuff now when reading xnlock_get/put).
>  > 
>  > > - functions arguments as macro, I find more readable the #ifdef with the
>  > >   different function prototypes, the code can be read without having to
>  > >   look at a different place.
>  > 
>  > I'm open to learn a third way to achieve what we need. I'm just
>  > convinced that the old way was far worse.
>  > 
>  > Please consider for a better suggestion that the number of variants
>  > increase with my ticket lock. That's why I tried to stuff things in
>  > macros. Hmm, maybe we should simply get rid of the file/line/function
>  > stuff completely and switch to IP + ksyms. What do you think?
> 
> I do not want to leave this in a dead end. IMO, your approach make
> xnlock_get readable in the non debugging case at the expense of its
> readability in the debugging case. I would better see the two
> implementations with a unique ifdef. Granted, there will be some code
> duplication, but it will not be that much, and this allows us to move
> the debugging version out of line while keeping the non debugging case
> inline.

Don't panic. I'm sitting on a new version of this patch series, only 
running a final benchmark to estimate the gain. This unfortunately takes 
a lot of time.

BTW, the series will also add lock debugging for UP, and it beautifies 
the refactoring a bit more, addressing your concerns.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]