Date: Thu, 10 Aug 2017 13:37:34 +0300
From: Yury Norov <ynorov@caviumnetworks.com>
To: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Andrew Morton <akpm@linux-foundation.org>,
        kbuild test robot <lkp@intel.com>, kbuild-all@01.org,
        Noam Camus <noamca@mellanox.com>,
        Matthew Wilcox <mawilcox@microsoft.com>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] lib: add test for bitmap_parselist()
Message-ID: <20170810103734.qkexg4lribuwkvre@yury-thinkpad>
References: <20170807225438.16161-2-ynorov@caviumnetworks.com>
 <201708091143.QqTgSaxI%fengguang.wu@intel.com>
 <20170809133309.746aafee5e22c9345debd671@linux-foundation.org>
 <20170809225231.ythrktk43gswfnjd@yury-thinkpad>
 <CAKwiHFipjgfmf6J_hkDD0o+W-4tO3rupU9SZtwi2w4CnBZvH8A@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAKwiHFipjgfmf6J_hkDD0o+W-4tO3rupU9SZtwi2w4CnBZvH8A@mail.gmail.com>
User-Agent: NeoMutt/20170113 (1.7.2)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Aug 2017 10:38:05.2886 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM3PR07MB2124
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 2866
Lines: 66

On Thu, Aug 10, 2017 at 09:30:31AM +0200, Rasmus Villemoes wrote:
> > From be0e663b804daff0d0512e72cf94b5143270bd29 Mon Sep 17 00:00:00 2001
> > From: Yury Norov <ynorov@caviumnetworks.com>
> > Date: Thu, 10 Aug 2017 01:25:46 +0300
> > Subject: [PATCH] bitmap: introduce BITMAP_FROM_U64() and use it in test for
> >  bitmap_parselist()
> >
> > The macro is the compile-time analogue of bitmap_from_u64() with the
> > same purpose: convert the 64-bit number to the properly ordered pair
> > of 32-bit parts to be suitable for filling the bitmap.
> >
> > Signed-off-by: Yury Norov <ynorov@caviumnetworks.com>
> > ---
> >  include/linux/bitmap.h |  6 ++++++
> >  lib/test_bitmap.c      | 23 +++++++++++++++++++----
> >  2 files changed, 25 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> > index 5797ca6fdfe2..bdc487e47de1 100644
> > --- a/include/linux/bitmap.h
> > +++ b/include/linux/bitmap.h
> > @@ -378,6 +378,12 @@ static inline void bitmap_from_u64(unsigned long *dst, u64 mask)
> >                 dst[1] = mask >> 32;
> >  }
> >
> > +#if __BITS_PER_LONG == 64
> > +#define BITMAP_FROM_U64(n) (n)
> > +#else
> > +#define BITMAP_FROM_U64(n) ((n) & ULONG_MAX), ((unsigned long long) (n) >> 32)
> > +#endif
> > +
> 
> The 32 bit version probably needs to come in two flavours depending on
> little/big endian, no?
 
Actually no. For completeness:

There are four combinations of endianess and length of the word in linux ABIs:
LE64, BE64, LE32 and BE32. 64-bit LE and BE are naturally ordered and therefore
don't require any special handling.

32-bit LE ABI orders lo word of 64-bit number prior to hi, and 32-bit BE
orders hi word prior to lo in the memory. The bitmap in other hand is
represented as the array of 32-bit words, and the position of the bit
N may therefore be calculated as: word (N/32) and bit (N%32) in that
word. For example, bit #42 is located at 10th position of 2nd word.
It matches 32-bit LE ABI, and we can simply let the compiler store 64-bit
value on the memory like it usually does. But for BE we therefore need
to swap words manually.

With all that, the macro (n & ULONG_MAX, n >> 32) does exactly what
needed for 32-bit case. In LE environment it does in fact nothing, and
in BE environment it swaps hi and lo words.

> Could you also throw in some casts so that the behaviour is consistent
> regardless of the type of the expression n, and so that both elements
> in the pair are guaranteed to have the same type (unsigned long,
> preferably, since that's what we're initializing). I.e., cast n to
> (u64), do arithmetic, cast result to (unsigned long).

If no objections, I'll change the patch like you proposed here, add
the explanation below to the comment of macro and resend it after
finishing the testing. (I need time to setup some 32-bit BE
environment to do test.)

Yury