linux-kernel - Re: [RFC][PATCH 0/2] PF

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:	Mon, 20 Sep 2010 08:40:16 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Peter Zijlstra <a.p.zijlstra@...llo.nl>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Ingo Molnar <mingo@...e.hu>,
	Thomas Gleixner <tglx@...utronix.de>,
	linux-kernel@...r.kernel.org, Jens Axboe <axboe@...nel.dk>,
	Tejun Heo <tj@...nel.org>, Avi Kivity <avi@...hat.com>
Subject: Re: [RFC][PATCH 0/2] PF_flags cleaups

On Mon, Sep 20, 2010 at 8:13 AM, Peter Zijlstra <a.p.zijlstra@...llo.nl> wrote:
> Because we recently ran out of PF_flags, try and clean up.
>
> Patches are on top of -tip, which already includes the PF_ALIGNWARN
> removal.

Looks ok by me conceptually, but I _really_ hate the naming of that
second patch and the pointless churn.

I also find it disgusting to have that [set_]task_type() "helper"
abstraction layer. It doesn't actually help anything at all, and just
makes it less clear what is going on.

So instead of code like this:

   if (task_type(p) >= tt_kernel)

why the heck isn't it just a much more readable old code (with just a
renamed new field) and just do

   if (task->task_type & TT_KTHREAD)

instead? Even if you were to want to use an enum rather than a set of
#define's, the all-caps thing makes it visually obvious that we're
talking about some constant rather than a variable named "tt_kernel".

I dunno. I'd personally also just keep it as a bitmask thing, if only
to make the patch more readable. IOW, the patch _should_ have just
turned

        tsk->flags |= PF_MEMALLOC | PF_SWAPWRITE | PF_KSWAPD;

into

        tsk->flags |= PF_MEMALLOC | PF_SWAPWRITE;
        tsk->task_type |= TT_KSWAPD;

and then the patch would always have been an "obvious straightforward
identity transform".

For example, look at the mess you did:

-       new_flags &= ~(PF_SUPERPRIV | PF_WQ_WORKER);
+       if (task_type(p) >= tt_kernel)
+               new_type = tt_kernel;
+       else
+               new_type = tt_user;
+
+       task_set_type(p, new_type);
+

and look how much straightforward it would have been had you just kept
the same simple semantics with just a new field:

-       new_flags &= ~(PF_SUPERPRIV | PF_WQ_WORKER);
+       new_type &= ~(TT_SUPERPRIV | TT_WQ_WORKER);

and nobody could possibly have any objections to a straightforward
"move the task type flags into a separate field" patch.

No?

                       Linus
--
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/