Re: [patch 1/4] mempolicy: convert MPOL constants to enum

[David Rientjes]

On Tue, 12 Feb 2008, Paul Jackson wrote:

> I tend to agree with Lee on this one.  If I recall correctly, Christoph
> said something similar, regarding the change of the 'policy' field
> of struct mempolicy from a short to an enum.
> 

I've already made the change in my patchset as a result of Christoph's 
comment.

> I'm inclined toward the original types for the 'policy' field.
> 
> Also, rather than trying to pack the new flag, MPOL_F_STATIC_NODES,
> into the existing 'policy' field, I'd suggest instead adding a new
> field to 'struct mempolicy' for this flag.  Since 'policy' is only a
> short, and since the next field in that struct, is a union that
> includes a pointer that is aligned on most arch's to at least a 4 byte
> boundary, therefore there is a hole of at least two bytes, following
> the short policy field, in which another short or some flag bits can be
> placed, with no increase in the size of struct mempolicy.
> 

I disagree, that space can be reserved for future use that will perhaps be 
much needed at that time.

The type 'short' is obviously overkill to hold the value of the policy 
mode since we only have four currently defined modes.  We'll never exceed 
the capacity of type 'unsigned char' if mempolicies are going to remain 
useful.

If the flag bits are going to be stored in the same member as the mode, it 
is necessary to make the change, as I have, to be unsigned to avoid 
sign-extension when this is promoted to 'int' that is required as part of 
the API.

> Specifically, I'd suggest adding the one line for 'mode_f_static_nodes'
> as below, and leaving the code involving the encoding of the policy
> field alone.
> 
> struct mempolicy {
>         atomic_t refcnt;
>         short policy;   /* See MPOL_* above */
> 	int mode_f_static_nodes:1;			/* <== Added line <== */
>         union {
>                 struct zonelist  *zonelist;     /* bind */
>                 short            preferred_node; /* preferred */
>                 nodemask_t       nodes;         /* interleave */
>                 /* undefined for default */
>         } v;
>         nodemask_t cpuset_mems_allowed; /* mempolicy relative to these nodes */
> };
> 
> Single bit fields (The ":1" above) provide the simplest way to add
> boolean flags to structs.  Let the compiler do the work of packing
> and unpacking the field.
> 
> Then, rather than having to code double-negative explicit masking
> operations such as:
> 
> 	remap = !(mpol_flags(pol->policy) & MPOL_F_STATIC_NODES);
> 	if (!remap)
> 		blah blah ...
> 
> one can simply code:
> 
> 	if (pol->mode_f_static_nodes)
> 		blah blah ...
> 

The problem with your approach is that we need to pass the optional mode 
flags back to the user through get_mempolicy() and the user needs to pass 
them to us via set_mempolicy() or mbind().  So we'll still need the 
#define in linux/mempolicy.h:

	#define MPOL_F_STATIC_NODES	(1 << MPOL_FLAG_SHIFT)

We could deal with this as follows:

	if (mode & MPOL_F_STATIC_NODES)
		pol->mode_f_static_nodes = 1;

but that doesn't make much sense.

Once Christoph's comment is taken into consideration and we start passing 
around 'int policy' instead of 'enum mempolicy_mode mode' again, I don't 
think anybody will struggle with doing:

	if (mode & MPOL_F_STATIC_NODES)

to check for the prescence of a flag.

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