Re: [PATCH v4 2/2] pid: Remove pidhash

Re: [PATCH v4 2/2] pid: Remove pidhash

[Rik van Riel]

[-- Attachment #1: Type: text/plain, Size: 2750 bytes --]

On Wed, 2017-10-11 at 17:47 +0800, kbuild test robot wrote:

> > > '__compiletime_assert_33' declared with attribute error:
> > > BUILD_BUG_ON failed: sizeof(struct upid) != 32
> 
>      _compiletime_assert(condition, msg, __compiletime_assert_,
> __LINE__)
>                                          ^
>    include/linux/compiler.h:556:4: note: in definition of macro
> '__compiletime_assert'
>        prefix ## suffix();    \
>        ^~~~~~
>    include/linux/compiler.h:576:2: note: in expansion of macro
> '_compiletime_assert'
>      _compiletime_assert(condition, msg, __compiletime_assert_,
> __LINE__)
>      ^~~~~~~~~~~~~~~~~~~
>    include/linux/build_bug.h:46:37: note: in expansion of macro
> 'compiletime_assert'
>     #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond),
> msg)
>                                         ^~~~~~~~~~~~~~~~~~
>    include/linux/build_bug.h:70:2: note: in expansion of macro
> 'BUILD_BUG_ON_MSG'
>      BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
>      ^~~~~~~~~~~~~~~~
> > > arch/ia64/kernel/asm-offsets.c:33:2: note: in expansion of macro
> > > 'BUILD_BUG_ON'
> 
>      BUILD_BUG_ON(sizeof(struct upid) != 32);
>      ^~~~~~~~~~~~
> 
Looks like arch/ia64/kernel/asm-offsets.c throws a BUILD_BUG
if sizeof(struct upid) != 32.

Your patch reduced the size of struct upid, which is a nice
thing, but now IA64 no longer builds.

Lets see what IA64 was doing with the size of struct upid
in the first place:

in arch/ia64/kernel/asm-offsets.c:

        BUILD_BUG_ON(sizeof(struct upid) != 32);
        DEFINE(IA64_UPID_SHIFT, 5);

Grepping for IA64_UPID_SHIFT leads us to some assembly
code implementing fsys_getpid (why is that in assembly?!):

        add r8=IA64_PID_LEVEL_OFFSET,r17
        ;;
        ld4 r8=[r8]                             // r8 = pid->level
        add r17=IA64_PID_UPID_OFFSET,r17        // r17 = &pid->numbers[0]
        ;;
        shl r8=r8,IA64_UPID_SHIFT
        ;;
        add r17=r17,r8                          // r17 = &pid->numbers[pid->level]
        ;;
        ld4 r8=[r17]                            // r8 = pid->numbers[pid->level].nr
        ;;
        mov r17=0

Luckily it looks like this is only referencing the first members of struct upid,
and you are removing the last member, so I suspect you will be fine changing the IA64
to this:

        BUILD_BUG_ON(sizeof(struct upid) != 16);
        DEFINE(IA64_UPID_SHIFT, 4);

Tony, does that look ok to you?

-- 
All Rights Reversed.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

RE: [PATCH v4 2/2] pid: Remove pidhash

[Luck, Tony]

PsKgwqDCoMKgwqDCoMKgwqBERUZJTkUoSUE2NF9VUElEX1NISUZULCA1KTsNCj4NCj4gR3JlcHBp
bmcgZm9yIElBNjRfVVBJRF9TSElGVCBsZWFkcyB1cyB0byBzb21lIGFzc2VtYmx5DQo+IGNvZGUg
aW1wbGVtZW50aW5nIGZzeXNfZ2V0cGlkICh3aHkgaXMgdGhhdCBpbiBhc3NlbWJseT8hKToNCg0K
VGhlIGZhc3Qgc3lzdGVtIGNhbGwgcGF0aCBoYXMgYSB3aG9sZSBob3N0IG9mIHNlcmlvdXMgcmVz
dHJpY3Rpb25zIG9uIHdoYXQgaXQgY2FuDQp0b3VjaC4gU2VlIERvY3VtZW50YXRpb24vaWE2NC9m
c3lzLnR4dC4gIFdoeSBpcyBnZXRwaWQoKSBhIGZhc3Qgc3lzdGVtIGNhbGw/IEkgdGhpbmsNCnRo
ZXJlIHdhcyBzb21lIGFwcGxpY2F0aW9uIChvciBwZXJoYXBzIGJlbmNobWFyaykgdGhhdCB1c2Vk
IGl0IGEgbG90Lg0KDQo+ICAgICAgICBhZGQgcjg9SUE2NF9QSURfTEVWRUxfT0ZGU0VULHIxNw0K
PiAgICAgICAgOzsNCj4gICAgICAgIGxkNCByOD1bcjhdICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAvLyByOCA9IHBpZC0+bGV2ZWwNCj4gICAgICAgIGFkZCByMTc9SUE2NF9QSURfVVBJRF9P
RkZTRVQscjE3ICAgICAgICAvLyByMTcgPSAmcGlkLT5udW1iZXJzWzBdDQo+ICAgICAgICA7Ow0K
PiAgICAgICAgc2hsIHI4PXI4LElBNjRfVVBJRF9TSElGVA0KPiAgICAgICAgOzsNCj4gICAgICAg
IGFkZCByMTc9cjE3LHI4ICAgICAgICAgICAgICAgICAgICAgICAgICAvLyByMTcgPSAmcGlkLT5u
dW1iZXJzW3BpZC0+bGV2ZWxdDQo+ICAgICAgICA7Ow0KPiAgICAgICAgbGQ0IHI4PVtyMTddICAg
ICAgICAgICAgICAgICAgICAgICAgICAgIC8vIHI4ID0gcGlkLT5udW1iZXJzW3BpZC0+bGV2ZWxd
Lm5yDQo+ICAgICAgICA7Ow0KPiAgICAgICAgbW92IHIxNz0wDQo+DQo+IEx1Y2tpbHkgaXQgbG9v
a3MgbGlrZSB0aGlzIGlzIG9ubHkgcmVmZXJlbmNpbmcgdGhlIGZpcnN0IG1lbWJlcnMgb2Ygc3Ry
dWN0IHVwaWQsDQo+IGFuZCB5b3UgYXJlIHJlbW92aW5nIHRoZSBsYXN0IG1lbWJlciwgc28gSSBz
dXNwZWN0IHlvdSB3aWxsIGJlIGZpbmUgY2hhbmdpbmcgdGhlIElBNjQNCj4gdG8gdGhpczoNCj4N
Cj7CoMKgwqDCoMKgwqDCoMKgQlVJTERfQlVHX09OKHNpemVvZihzdHJ1Y3QgdXBpZCkgIT0gMTYp
Ow0KPsKgwqDCoMKgwqDCoMKgwqBERUZJTkUoSUE2NF9VUElEX1NISUZULCA0KTsNCj4NCj4gVG9u
eSwgZG9lcyB0aGF0IGxvb2sgb2sgdG8geW91Pw0KDQpJIHRoaW5rIHNvLiAgUmVzcGluIGFuZCBD
YzogbWUgb24gYm90aCBwYXRjaGVzIGluIHRoZSBzZXJpZXMgYW5kIEknbGwgdGFrZSBpdCBmb3Ig
YSBzcGluDQpvbiBoL3cgdG8gbWFrZSBzdXJlLg0KDQotVG9ueQ0K