Re: [RFC][PATCH 0/2] PF_flags cleaups

[Linus Torvalds]

On Mon, Sep 20, 2010 at 8:13 AM, Peter Zijlstra <a.p.zijlstra@xxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/