Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 22 Nov 2017 00:15:48 +0300
From:   Yury Norov <ynorov@caviumnetworks.com>
To:     Matthew Wilcox <mawilcox@microsoft.com>
Cc:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Ben Hutchings <ben@decadent.org.uk>,
        David Decotigny <decot@googlers.com>,
        "David S . Miller" <davem@davemloft.net>,
        Rasmus Villemoes <linux@rasmusvillemoes.dk>,
        Geert Uytterhoeven <geert@linux-m68k.org>
Subject: Re: [PATCH RFC] lib: simplify bitmap_from_u32array API
Message-ID: <20171121211548.xhnkdys567l6jdtr@yury-thinkpad>
References: <20171115174056.26537-1-ynorov@caviumnetworks.com>
 <MWHPR21MB084523580162E4BFCB56CDD3CB290@MWHPR21MB0845.namprd21.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <MWHPR21MB084523580162E4BFCB56CDD3CB290@MWHPR21MB0845.namprd21.prod.outlook.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Received-SPF: None (protection.outlook.com: cavium.com does not designate
 permitted sender hosts)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Nov 2017 21:16:05.6445 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 25c7dd59-ed06-43f9-071b-08d531251528
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 711e4ccf-2e9b-4bcf-a551-4094005b6194
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR0701MB3836
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Matthew, 

[+ Geert Uytterhoeven)

On Wed, Nov 15, 2017 at 07:24:15PM +0000, Matthew Wilcox wrote:
> I certainly approve.  The name sucks too ????

Yep. I changed it, didn't resist.
 
> > @@ -60,7 +60,7 @@
> >   * bitmap_find_free_region(bitmap, bits, order)	Find and allocate bit region
> >   * bitmap_release_region(bitmap, pos, order)	Free specified bit region
> >   * bitmap_allocate_region(bitmap, pos, order)	Allocate specified bit region
> > - * bitmap_from_u32array(dst, nbits, buf, nwords) *dst = *buf (nwords 32b
> > words)
> > + * bitmap_from_u32array(dst, buf, nbits)	*dst = *buf (nwords 32b
> > words)
> 
> I think this should read:
> + * bitmap_from_u32array(dst, buf, bits)	Copy 'bits' from buf to dst

Ack

> Also, on LE systems, shouldn't we just use memcpy() for the first bits/8 bytes?

Yes, but I did prefer to keep logic common for LE and BE arches. We
can switch to memcpy 32-bit LE and BE versions, see my thoughts below.

>From 6ab70ff5ea32ecc5c9e77cc4b84e6dd3843e5d8f Mon Sep 17 00:00:00 2001
From: Yury Norov <ynorov@caviumnetworks.com>
Date: Tue, 21 Nov 2017 22:42:57 +0300
Subject: [PATCH 1/2] bitmap: new bitmap_copy_safe and bitmap_{from,to}_arr32

'Safe' in bitmap_copy_safe stands for clearing tail bits in bitmap
beyond last bit till the end of last word. It is useful for hardening
API when bitmap is assumed to be exposed to userspace.

bitmap_{from,to}_arr32 functions are replacements for bitmap_{from,to}_u32array.
They don't take unneeded nwords argument, and so simpler in implementation
and understanding.

This patch suggests optimization for 32-bit systems - aliasing
bitmap_{from,to}_arr32 to bitmap_copy_safe. Other possible optimization
is to map 64-bit LE bitmap_{from,to}_arr32 to more generic function(s).

But I didn't end up with the function that would be helpful by itself,
and can be used to alias 64-bit LE bitmap_{from,to}_arr32, like
bitmap_copy_safe does. So I preferred to leave things as is.

Since this is RFC, I would like to ask people here for ideas. :)

The following patch switches kernel to new API and introduces new test.

Signed-off-by: Yury Norov <ynorov@caviumnetworks.com>
---
 include/linux/bitmap.h | 31 +++++++++++++++++++++++++++++
 lib/bitmap.c           | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 85 insertions(+)

diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
index 19748a5b0e77..1fa0de16b0a7 100644
--- a/include/linux/bitmap.h
+++ b/include/linux/bitmap.h
@@ -62,6 +62,8 @@
  * bitmap_allocate_region(bitmap, pos, order)	Allocate specified bit region
  * bitmap_from_u32array(dst, nbits, buf, nwords) *dst = *buf (nwords 32b words)
  * bitmap_to_u32array(buf, nwords, src, nbits)	*buf = *dst (nwords 32b words)
+ * bitmap_from_arr32(dst, buf, nbits)		Copy 'nbits' from u32 buf to dst
+ * bitmap_to_arr32(buf, src, nbits)		Copy 'nbits' from buf to u32 dst
  */
 
 /*
@@ -219,6 +221,35 @@ static inline void bitmap_copy(unsigned long *dst, const unsigned long *src,
 	}
 }
 
+/*
+ * Copy bitmap and clear tail bits in last word.
+ */
+static inline void bitmap_copy_safe(unsigned long *dst,
+		const unsigned long *src, unsigned int nbits)
+{
+	bitmap_copy(dst, src, nbits);
+	if (nbits % BITS_PER_LONG)
+		dst[nbits / BITS_PER_LONG] &= BITMAP_LAST_WORD_MASK(nbits);
+}
+
+/*
+ * On 32-bit systems bitmaps are represented as u32 arrays internally, and
+ * therefore conversion is not needed when copying data from/to arrays of u32.
+ */
+#if BITS_PER_LONG == 64
+extern void bitmap_from_arr32(unsigned long *bitmap, const u32 *buf,
+							unsigned int nbits);
+extern void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap,
+							unsigned int nbits);
+#else
+#define bitmap_from_arr32(bitmap, buf, nbits)		\
+	bitmap_copy_safe((unsigned long *) (bitmap),	\
+			(const unsigned long *) (buf), (nbits))
+#define bitmap_to_arr32(buf, bitmap, nbits)		\
+	bitmap_copy_safe((unsigned long *) (buf),	\
+			(const unsigned long *) (bitmap), (nbits))
+#endif
+
 static inline int bitmap_and(unsigned long *dst, const unsigned long *src1,
 			const unsigned long *src2, unsigned int nbits)
 {
diff --git a/lib/bitmap.c b/lib/bitmap.c
index c82c61b66e16..b0f79074ae84 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -1212,3 +1212,57 @@ void bitmap_copy_le(unsigned long *dst, const unsigned long *src, unsigned int n
 }
 EXPORT_SYMBOL(bitmap_copy_le);
 #endif
+
+#if BITS_PER_LONG == 64
+/**
+ * bitmap_from_arr32 - copy the contents of a u32 array of bits to bitmap
+ *	@bitmap: array of unsigned longs, the destination bitmap, non NULL
+ *	@buf: array of u32 (in host byte order), the source bitmap, non NULL
+ *	@nbits: number of bits in @bitmap
+ */
+void bitmap_from_arr32(unsigned long *bitmap, const u32 *buf,
+						unsigned int nbits)
+{
+	unsigned int i, halfwords;
+
+	if (!nbits)
+		return;
+
+	halfwords = DIV_ROUND_UP(nbits, 32);
+	for (i = 0; i < halfwords; i++) {
+		bitmap[i/2] = (unsigned long) buf[i];
+		if (++i < halfwords)
+			bitmap[i/2] |= ((unsigned long) buf[i]) << 32;
+	}
+
+	/* Clear tail bits in last word beyond nbits. */
+	bitmap[(halfwords - 1) / 2] &= BITMAP_LAST_WORD_MASK(nbits);
+}
+EXPORT_SYMBOL(bitmap_from_arr32);
+
+/**
+ * bitmap_to_arr32 - copy the contents of bitmap to a u32 array of bits
+ *	@buf: array of u32 (in host byte order), the dest bitmap, non NULL
+ *	@bitmap: array of unsigned longs, the source bitmap, non NULL
+ *	@nbits: number of bits in @bitmap
+ */
+void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap, unsigned int nbits)
+{
+	unsigned int i, halfwords;
+
+	if (!nbits)
+		return;
+
+	halfwords = DIV_ROUND_UP(nbits, 32);
+	for (i = 0; i < halfwords; i++) {
+		buf[i] = (u32) (bitmap[i/2] & UINT_MAX);
+		if (++i < halfwords)
+			buf[i] = (u32) (bitmap[i/2] >> 32);
+	}
+
+	/* Clear tail bits in last element of array beyond nbits. */
+	buf[halfwords - 1] &= (u32) (UINT_MAX >> ((-nbits) & 31));
+}
+EXPORT_SYMBOL(bitmap_to_arr32);
+
+#endif
-- 
2.11.0


From 1584165437354470018@xxx Wed Nov 15 20:31:30 +0000 2017
X-GM-THRID: 1584156130360803324
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread