Re: Re: [PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw.

[Jason Gunthorpe <jgg@ziepe.ca>]

On Tue, Aug 06, 2019 at 05:01:49PM +0000, Bernard Metzler wrote:
> 
> >To: "Bernard Metzler" <BMT@zurich.ibm.com>
> >From: "Jason Gunthorpe" <jgg@ziepe.ca>
> >Date: 08/06/2019 06:39PM
> >Cc: linux-rdma@vger.kernel.org
> >Subject: Re: Re: [PATCH 1/1] Make user mmapped CQ arming flags field
> >32 bit size to remove 64 bit architecture dependency of siw.
> >
> >On Tue, Aug 06, 2019 at 04:36:26PM +0000, Bernard Metzler wrote:
> >> 
> >> >To: "Bernard Metzler" <BMT@zurich.ibm.com>
> >> >From: "Jason Gunthorpe" <jgg@ziepe.ca>
> >> >Date: 08/06/2019 05:31PM
> >> >Cc: linux-rdma@vger.kernel.org
> >> >Subject: Re: Re: [PATCH 1/1] Make user mmapped CQ arming flags
> >field
> >> >32 bit size to remove 64 bit architecture dependency of siw.
> >> >
> >> >On Tue, Aug 06, 2019 at 02:53:49PM +0000, Bernard Metzler wrote:
> >> >
> >> >> >> index 7de68f1dc707..af735f55b291 100644
> >> >> >> +++ b/include/uapi/rdma/siw-abi.h
> >> >> >> @@ -180,6 +180,7 @@ struct siw_cqe {
> >> >> >>   * to control CQ arming.
> >> >> >>   */
> >> >> >>  struct siw_cq_ctrl {
> >> >> >> -	__aligned_u64 notify;
> >> >> >> +	__u32 flags;
> >> >> >> +	__u32 pad;
> >> >> >
> >> >> >The commit message needs to explain why this is compatible with
> >> >> >existing user space, if it is even is safe..
> >> >> >
> >> >> Old libsiw would remain compatible with the new layout, since it
> >> >> simply reads the 32bit 'flags' and zeroed 32bit 'pad' into a
> >64bit
> >> >> 'notify', ending with reading the same bits.
> >> >
> >> >Even on big endian?
> >> >
> >> Well I do not have access to a BE system right now to verify.
> >> But on a BE system, the lowest 3 bits (which are in use) of the
> >first
> >> 32bit variable 'flags' shall be the lowest (leftmost) 3 bits of an
> >> 'overlayed' 64bit variable 'notify' as well...
> >
> >One of LE or BE won't work with this scheme, it can't, the flag bit
> >will end up in the pad.
> >
> Sitting here on a x86 (LE), and it works. On a 64bits machine,
> two consecutive 32bits are obviously reordered in memory. Leaves
> 32bit LE broken, which is currently not supported.

? I still think 64 bit BE will be broken as the low two bits will
overlap the pad, not the new flags.

> Anyway, what would you suggest as the best path forward? A new ABI
> version? If we move to test_and_clear_bit(), 'flags' size would
> become architecture dependent...and we would break the ABI as well,
> no?

Maybe a #ifdef __big_endian__ and swap the order of the pad/flags ?

Jason