Re: puzzling code in pcpu stuff

Alban Hertroys schrieb:
> On 2 Aug 2009, at 12:34, Christoph Mallon wrote:
> 
>> Julian Elischer schrieb:
>>> I simplified the output of the preprocessor for a PCPU_SET(xx, newval)
>>> (to look at it).
>>> I came down to: (after formatting) for i386..
>>> {
>>>        __typeof(((struct pcpu *)0)->pc_xx) __val;
>>>        struct __s
>>>        {
>>>                u_char __b[(((sizeof(__val)) < (4)) ?
>>>                                   (sizeof(__val)) : (4))];
>>>        } __s;
>>>        __val = (newval); /* aligned */
>>>        if (sizeof(__val) == 1
>>>        || sizeof(__val) == 2
>>>        || sizeof(__val) == 4) {
>>>                __s = *(struct __s *)(void *)&__val;
>>>                __asm volatile("mov %1,%%fs:%0" : "=m"
>>>                  (*(struct __s *)(__builtin_offsetof(
>>>                       struct pcpu,  pc_xx))) : "r" (__s));
>>>        } else {
>>>                *__extension__ (
>>>                {
>>>                        __typeof(__val) *__p;
>>>                        __asm volatile("movl %%fs:%1,%0;
>>>                        addl %2,%0" : "=r" (__p) : "m"
>>>  (*(struct pcpu *)(__builtin_offsetof(struct pcpu, pc_prvspace))),
>>>  "i"
>>>  (__builtin_offsetof(struct pcpu, pc_xx)));
>>>   __p;
>>>                }) = __val;
>>>        }
>>> }
>>> having had my brain explode on this several times,
>>> I can't figure out exactly what teh clause after the else is doing.
>>> anyone better at reading __asm better than me care to explain it in
>>> simple words?
>>
>> First, ({}) is a statement expression - a GCC extension (not to be 
>> confused with expression statement, which is an expression followed by 
>> a semicolon). It wraps a compound statement, i.e. {}, and turns it 
>> into an expression. The value of the last statement is the value of 
>> the expression. In this case it's __p;.
> 
> Speaking as an outsider I'd better be careful with any criticism, but 
> the first thing I noticed here was the lack of comments. From Julian's 
> question it seems obvious that this function could do with some. I 
> wonder what people would make of this in a couple of years when none of 
> the (then) active developers has any intimate knowledge of the workings 
> of functions like this one?
> 
> I got from the posted code sample that __extension__ is probably a no-op 
> meant for documentation purposes only. I think it would help to add what 
> extension is being used though, maybe as a comment (but what would be 
> the use of defining __extension__ then) or as a parameter. That's up to 
> you though.

GCC can be switched into a mode in which it rejects extensions like 
({}). If you prepend an extension with __extension__ in this mode, it 
accepts it.
Of course you can do fine here without this extension, just by using a 
static inline function which returns the per-cpu pointer.

> Not being familiar with x86 assembly doesn't help me here of course, the 
> last time I touched assembly was on a Z80 a couple of decades back...

Similarily, if you are not familiar with C, then ++i; will puzzle you. 
Do you want to add /* add one to the integer variable named i */ everywhere?
But you've got one point: There should be a comment added, which states 
that the if () is just an optimisation.

>> Let's have a closer look at the else clause: The asm reads the pointer 
>> to per-cpu information into __p and the statement expression returns 
>> it. This pointer gets dereferenced (mind the * before __extension__ - 
>> __extension__ does nothing, it just marks that the following is a GCC 
>> extension) and __val is assigned.
> 
> 
> As I read it the value of __val is read into the memory address 
> calculated by the expression in the statement expression (__p 
> internally), am I right?

It is
   __p = GET_PCPU_PTR() + OFFSET_OF_ENTRY_IN_STRUCT(name);
   *__p = val;

The stuff in uppercase is done in the inline asm. Though there is no 
good reason to do the addition of the offset inside the inline asm. 
Simpler would be GET_PCPU_PTR()->name (it probably leads to better code, 
too). I have a diff for this simplification and adding a static inline 
function to get the pcpu pointer somewhere...

	Christoph