Re: [cpuops cmpxchg double V2 1/4] Generic support forthis_cpu_cmpxchg_double

[Christoph Lameter]

On Fri, 7 Jan 2011, Mathieu Desnoyers wrote:

> I have to admit that I also hate the current interface for two reasons:
>
> a) the way it splits the target address in two.

We are handling two different entities that are placed next to each other.

> b) the loss of the value read (the fact that the only current user of this API
>    does not need the value returned seems like a very weak argument to define an
>    API).

The other user of cmpxchg_double that I have in my tree also does not have
the need. Repeatability is not as simple to implement as with a single
word cmpxchg.

> I'm probably missing important compiler trickiness here, but why are we not
> rather relying on forced compiler alignment to declare the target address type ?
> e.g.:
>
> typedef struct {
>         unsigned long v[2];
> } __attribute__((aligned(2 * sizeof(long)))) cmpxchg_double_t;
>
> For the usage you are doing within the allocator code, you can use an union (the
> bitfield sizes are completely arbitrary).
>
> union alloc_cd {
>         cmpxchg_double_t cd;
>         struct {
>                 void *ptr;
>                 unsigned long seq:48;
>                 unsigned long cpu:16;
>         } v;
> };
>
> So this_cpu_cmpxchg_double would expect the type "cmpxchg_double_t *" as its
> first argument.

That requires conversions back and forth between words and this
type. We only need the bonding between the two per cpu variables for the
single cmpxchg_double operation. There is no need to do this for the two
old values or the two new values.

> One way to manage to return the value read would be to pass a pointer to
> "old_word1-2" as argument rather than the value itself, and return the value
> read in these locations. Anyway, we won't need the old words for comparison with
> the return value, because the returned "bool" takes care of telling us if they
> differ.

That would require allocating storage for the two word variable required,
passing an address etc. All not necessary right now. Would increase the
latency of the inner loop. Saving the returned values also would increase
the code uselessly.

> Adding the types just for clarity sake (should be handled differently in your
> macro scheme of course), this_cpu_cmpxchg_double would look like:
>
> typedef struct {
>         unsigned long v[2];
> } __attribute__((aligned(2 * sizeof(long)))) cmpxchg_double_t;
>
> bool this_cpu_cmpxchg_double(cmpxchg_double_t *pcp,
>                              unsigned long *old_ret_word1,
>                              unsigned long *old_ret_word2,
>                              unsigned long new_word1,
>                              unsigned long new_word2)
>
> Thoughts ?

Looks not symmetric. The proposed approach in the patch has a clear
association of the first and second percpu variable with the first and
second old and new values.

Also cmpxchg_double may be used with different types and different object
sizes. Are we going to define cmpxchg_double_t for all types supported
and maybe created by the user? this_cpu_cmpxchg_double also supports two 4
byte fields.

Plus you can have mixed types. In the current use case you have one
integer and one pointer. So the cmpxchg_double_t would not fit in cleanly.
We would have to define custom types. And with that we will then have
difficulties codeing methods to check the compatibility of types.

I tried something like this before I came up with the current
solution. It was significantly more code and the code was difficult
to understand. Never was as clean and this one.
--
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/