Re: [patch 3/4] mempolicy: add MPOL_F_STATIC_NODES flag

[Paul Jackson]

I had taken a vow of silence on this one, but couldn't resist one more
round.

David wrote:
> My preference (both mode and flags stored in the same member of struct 
> mempolicy):
> 
>    Advantages:
> 
> 	- completely consistent with the userspace API of passing modes
> 	  and flags together in a pointer to an int, and

True -- the necessary overloaded ugliness of the kernel-user system
call API is propogated throughout mempolicy.c.  However, I wouldn't
necessarily call that an advantage.

> 	- does not require additional formals to be added to several
> 	  functions, including functions outside mm/mempolicy.c.

... but does require the use of the new mpol_flags() and mpol_mode()
macros by code in mmshmem.c, outside of mm/mempolicy.c

>    Disadvantage:
> 
> 	- use of mpol_mode() throughout mm/mempolicy.c code to mask
> 	  off optional mode flags for conditionals or switch statements.

This will cause a bug in the future, that escapes into the wild, when
someone forgets one of these.  I'll bet on that.  It's fragile, because
(1) such errors are easy to make, and (2) hard to catch.

> Your preference (separate mode and flags members in struct mempolicy):
> 
>    Advantages:
> 
> 	- clearer implementation when dealing with modes: all existing
> 	  statements involving pol->policy can remain unchanged.

Clear, robust code - that's the biggie.

>    Disadvantages:
> 
> 	- requires additional formals to be added to several functions,
> 	  including functions outside mm/mempolicy.c, and

True -- though by this argument, we'd routinely aggregate multiple flags
and small words into single integer parameters, just to minimize the
parameter count.  Putting two flags in one parameter is a false
simplification, unless required by circumstance, such as communicating
with deep space probes or across the system call boundary with existing
API's.

Across the system call boundary, we have little choice, for compatibility
reasons.  But kernel internal interfaces are not so constrained, and the
ugliness at the system call boundary can be quarantined from most of the
mempolicy.c code.

> 	- takes additional space in struct mempolicy (two bytes) which
> 	  could eventually be used for something else.

To be clear to others, as you know, we're not talking here about
growing the sizeof(struct mempolicy) at present, but rather about using
some currently unused bytes in the struct.

More often than not, when someone adds complexity that is not needed at
present, because it might be needed in the future, they are making it
harder to maintain the code, not easier.

The single most important thing we can do to improve future
maintainability of code is to make it more readable.

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@xxxxxxx> 1.940.382.4214
--
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/