On Sat, Nov 30, 2019 at 01:00:37AM +0100, Luc Van Oostenryck wrote:
> On Fri, Nov 29, 2019 at 06:11:59PM +0000, Christopher Lameter wrote:
> > On Wed, 27 Nov 2019, Luc Van Oostenryck wrote:
> >
> > > 1) it would strip any address space, not just __percpu, so:
> > > it would need to be combined with __verify_pcpu_ptr() or,
> > > * a better name should be used,
> >
> > typeof_cast_kernel() to express the fact that it creates a kernel pointer
> > and ignored the attributes??
>
> typeof_strip_address_space() would, I think, express this better.
> It's not obvious at all to me that 'kernel' in 'typeof_cast_kernel()'
> relates to the (default) kernel address space.
> Maybe it's just me. I don't know.
>
I think typeof_cast_kernel() or typeof_force_kernel() are reasonable
names. I kind of like the idea of cast/force over strip because we're
really still moving address spaces even if it is moving it back.
> > > * it should be defined in a generic header, any idea where?
> >
> > include/linux/compiler-types.h
>
> Yes, OK.
>
> > > 2) while I find the current solution:
> > > typeof(T) __kernel __force *ptr = ...;
> >
> > It would be
> >
> > typeof_cast_kernel(&T) *xx = xxx
> >
> > or so?
>
> No, it would not. __percpu, and more generally, the address space
> is a property of the object, not of its address.
Maybe for other address spaces that's true, but for percpu, __percpu is
a property of the address. An object can be referenced both from a
percpu address (via accessors) and the kernel address which is the
actual object.
> For example, let's say T is a __percpu object:
> int __percpu obj;
This can't exist. __percpu denotes address space not object.
> then '&T' is just a 'normal'/__kernel pointer to it:
> int __percpu *;
> There is nothing to strip (it would be if the __percpu
> would be 'on the other side of the *': int * __percpu).
> It's exactly the same as with 'const': a 'const char *'
> is not const, only a pointer to const.
>
> The situation with raw_cpu_generic_add_return() is:
> - pcp is a lvalue of of a __percpu object of type T, so:
> typeof(pcp) := T __percpu
> - pcp's address is given to raw_cpu_ptr(), so
> typeof(&pcp) := T __percpu *
> - raw_cpu_ptr() return the corresponding __kernel pointer
> (adjusted for the current percu offset), so:
> typeof(raw_cpu_ptr(&pcp)) := T *
> - so, the macro needs to declare a variable __p of type T*
> hence:
> typeof(pcp) __kernel __force *__p;
> or, with this new macro:
> typeof_cast_kernel(pcp) *__p;
>
> Maybe a better solution would be to directly play at pointer
> level and thus have something like this:
> typeof_<some good name>(&pcp) __p = raw_cpu_ptr(&pcp);
> or even:
> __kernel_pointer(&pcp) __p = raw_cpu_ptr(&pcp);
> I dunno.
>
> Note: at implementation level, it complicates things slightly
> to want this 'strip_percpu' macro to behaves like typeof()
> because it means that it can take in argument either an
> expression or a type. And if it's a type, you can't do a
> simple cast on it, you need to declare an intermediate
> variable, hence the horrible:
> typeof(({ typeof(T) __kernel __force __fakename; __fakename; }))
>
> Note: it would be much much nicer to do all these type generic
> macros with '__auto_type' (only supported in GCC 4.9 IIUC
> and supported in sparse but it shouldn't be very hard to do)..
>
>
> -- Luc
Thanks for debugging this. I'm still inclined to have a macro for either
cast/force. I do agree it could be misused, but it's no different doing
it in a macro than by just adding __force __kernel.
Thanks,
Dennis
