Re: [PATCH] RWSEM: Rewrite rwsem.c and rwsem-spinlock.c more simply.

["Robert P. J. Day" <rpjday@crashcourse.ca>]

On Tue, 25 Mar 2008, Andi Kleen wrote:

> "Robert P. J. Day" <rpjday@crashcourse.ca> writes:
>
> > Rewrite these source files more simply by deleting the superfluous
> > "tsk" task_struct pointer and rephrasing in terms of the "current"
> > task pointer.
>
> This is likely a code pessimization because "current" is inline
> assembler and many gcc versions cannot CSE it.

i'm not sure what this means -- which of the transformations in that
patch is considered unsafe?  here's a typical simplification:

-       tsk = current;
-       set_task_state(tsk, TASK_UNINTERRUPTIBLE);
+       set_current_state(TASK_UNINTERRUPTIBLE);


  there's all sorts of usage of set_current_state() throughout the
tree.  how is simplifying the code in these two files in exactly the
same way any different?  or am i missing something because this
involves semaphores?

rday

p.s.  given this bit from sched.h:

...
#define __set_task_state(tsk, state_value)              \
        do { (tsk)->state = (state_value); } while (0)
#define set_task_state(tsk, state_value)                \
        set_mb((tsk)->state, (state_value))

...

#define __set_current_state(state_value)                        \
        do { current->state = (state_value); } while (0)
#define set_current_state(state_value)          \
        set_mb(current->state, (state_value))
...

it's not clear why set_current_state() and __current_state() are
defined the way they are when it would seem to be simpler (and less
error-prone) to just write:

#define __set_current_state(sv)  __set_task_state(current, sv)
#define set_current_state(sv)   set_task_state(current, sv)

  the law of parsimony and all that.  or, once again, is there
something subtle i'm not seeing?

--


========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry:
    Have classroom, will lecture.

http://crashcourse.ca                          Waterloo, Ontario, CANADA
========================================================================