Messages in this thread | | | Date | Thu, 10 Dec 2009 17:49:02 -0800 | From | "H. Peter Anvin" <> | Subject | Re: [PATCH] x86, msr: add support for non-contiguous cpumasks |
| |
On 12/07/2009 04:21 AM, Borislav Petkov wrote: > > struct msr { > + int cpu; > union { > struct { > u32 l;
I really don't like this patch, for multiple reasons. One of them is the above: this has no business being part of struct msr, which reflects an MSR value (and ideally should replace at least the use of two pointers in other places over time). Having a CPU field and not an MSR number field particular doesn't make any sense.
The second is a linear(!!) search executed on every CPU... at the same time, over the same data structure.
The ideal probably would be to use a percpu variable. Now, this would either have to be a dynamic percpu allocation (which would have to be the responsibility of the caller, and reused, lest this would be a *very* expensive proposition), or we would have to make these functions run under a mutex. However, as long as the expected callers of this are things that get set up once and then pretty much stick around, a percpu variable might just work.
The slightly less radical version would be to simply require an array that spans the extremes of the CPU numbers in the mask. Even on a 4096-CPU system, we're only talking about 32K worth of memory here. This is basically the original solution, just accounting for the fact that the bitmap may be sparse when allocating it.
The third option would be to at least require that the struct msr contents are at least serial in the order of the bitmask, not by adding another field.
-hpa
| |