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

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

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

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?

> 
> Something we could be interesting would be to be able to enable
> spinlocks debug in UP, which would enable real debugging xnlocks in this
> case. I made an attempt of doing this on ARM some time ago, this
> generated a kernel that would lockup at boot. But I think this is
> something we should sort out.
> 

Yeah, sounds good and should be feasible. Will check.

Jan


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