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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 25 Mar 2008 08:27:35 -0400 (EDT)
From:	"Robert P. J. Day" <rpjday@...shcourse.ca>
To:	Andi Kleen <andi@...stfloor.org>
cc:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...l.org>
Subject: Re: [PATCH] RWSEM: Rewrite rwsem.c and rwsem-spinlock.c more
 simply.

On Tue, 25 Mar 2008, Andi Kleen wrote:

> "Robert P. J. Day" <rpjday@...shcourse.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
========================================================================
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/