Skip to contentSkip to conversation

bitmap: Introduce alloc/free helpers

PATCH Superseded Andy Shevchenko 6 participants 18 messages Jun 18, 2018 – Jun 26, 2018

Subsystems (6)
Library code HID MD RAID Input drivers +2 more subsystems

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Andy Shevchenko

author
#0 lore raw

v3 cover bitmap: Introduce alloc/free helpers

A lot of code is using allocation of bitmaps using BITS_PER_LONG() macro and sizeof(unsigned long) operator. The readability suffers because of this.

The series introduces three helpers, i.e. bitmap_alloc(), bitmap_zalloc() and bitmap_free(), to make it more cleaner.

Patch 1 is a preparatory to avoid namespace collisions between bitmap API and MD bitmap. No functional changes intended.

Patch 2 is just orphaned from previous release cycle.

Patch 3 introduces new helpers.

Patches 4 and 5 is just an example how to use new helpers. Locally I have like dozen of them against different subsystems and drivers.

Ideally it would go through Input subsystem, thus, needs an Ack from MD maintainer(s).

Since v2:
- fix compilation issue in MD bitmap code
- elaborate changes in commit message of patch 5

Since v1:
- added namespace fix patch against MD bitmap API
- moved functions to lib/bitmap.c to avoid circular dependencies
- appended Dmitry's tags

Andy Shevchenko (5):
md: Avoid namespace collision with bitmap API
bitmap: Drop unnecessary 0 check for u32 array operations
bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()
Input: gpio-keys - Switch to bitmap_zalloc()
Input: evdev - Switch to bitmap API

 drivers/input/evdev.c                         |  16 +-
 drivers/input/keyboard/gpio_keys.c            |   8 +-
 drivers/md/dm-raid.c                          |   6 +-
 drivers/md/md-bitmap.c                        | 301 +++++++++---------
 drivers/md/md-bitmap.h                        |  46 +--
 drivers/md/md-cluster.c                       |  16 +-
 drivers/md/md.c                               |  44 +--
 .../md/persistent-data/dm-space-map-common.c  |  12 +-
 drivers/md/raid1.c                            |  20 +-
 drivers/md/raid10.c                           |  26 +-
 drivers/md/raid5-cache.c                      |   2 +-
 drivers/md/raid5.c                            |  24 +-
 include/linux/bitmap.h                        |   8 +
 lib/bitmap.c                                  |  28 +-
 14 files changed, 283 insertions(+), 274 deletions(-)
Signature · 1 line
2.17.1
author
#1 lore raw

PATCH v3 1/5 md: Avoid namespace collision with bitmap API

bitmap API (include/linux/bitmap.h) has 'bitmap' prefix for its methods.

On the other hand MD bitmap API is special case. Adding 'md' prefix to it to avoid name space collision.

No functional changes intended.

Signed-off-by Andy Shevchenko <[email protected]>
---
 drivers/md/dm-raid.c                          |   6 +-
 drivers/md/md-bitmap.c                        | 301 +++++++++---------
 drivers/md/md-bitmap.h                        |  46 +--
 drivers/md/md-cluster.c                       |  16 +-
 drivers/md/md.c                               |  44 +--
 .../md/persistent-data/dm-space-map-common.c  |  12 +-
 drivers/md/raid1.c                            |  20 +-
 drivers/md/raid10.c                           |  26 +-
 drivers/md/raid5-cache.c                      |   2 +-
 drivers/md/raid5.c                            |  24 +-
 10 files changed, 244 insertions(+), 253 deletions(-)
drivers/md/dm-raid.c +3 -3
index ab13fcec3fca..71d158560e69 100644
@@ -3859,7 +3859,7 @@ static int __load_dirty_region_bitmap(struct raid_set *rs)
 	/* Try loading the bitmap unless "raid0", which does not have one */
 	if (!rs_is_raid0(rs) &&
 	    !test_and_set_bit(RT_FLAG_RS_BITMAP_LOADED, &rs->runtime_flags)) {
-		r = bitmap_load(&rs->md);
+		r = md_bitmap_load(&rs->md);
 		if (r)
 			DMERR("Failed to load bitmap");
 	}

@@ -3987,8 +3987,8 @@ static int raid_preresume(struct dm_target *ti)
 	/* Resize bitmap to adjust to changed region size (aka MD bitmap chunksize) */
 	if (test_bit(RT_FLAG_RS_BITMAP_LOADED, &rs->runtime_flags) && mddev->bitmap &&
 	    mddev->bitmap_info.chunksize != to_bytes(rs->requested_bitmap_chunk_sectors)) {
-		r = bitmap_resize(mddev->bitmap, mddev->dev_sectors,
-				  to_bytes(rs->requested_bitmap_chunk_sectors), 0);
+		r = md_bitmap_resize(mddev->bitmap, mddev->dev_sectors,
+				     to_bytes(rs->requested_bitmap_chunk_sectors), 0);
 		if (r)
 			DMERR("Failed to resize bitmap");
 	}
drivers/md/md-bitmap.c +146 -155

891 lines not on this page · raw message

drivers/md/md-bitmap.h +23 -23

66 lines not on this page · raw message

drivers/md/md-cluster.c +8 -8
index 021cbf9ef1bf..8f40c8e50c69 100644
@@ -326,7 +326,7 @@ static void recover_bitmaps(struct md_thread *thread)
 					str, ret);
 			goto clear_bit;
 		}
-		ret = bitmap_copy_from_slot(mddev, slot, &lo, &hi, true);
+		ret = md_bitmap_copy_from_slot(mddev, slot, &lo, &hi, true);
 		if (ret) {
 			pr_err("md-cluster: Could not copy data from bitmap %d\n", slot);
 			goto clear_bit;

@@ -480,7 +480,7 @@ static void process_suspend_info(struct mddev *mddev,
 	 * resync thread is running in another node,
 	 * so we don't need to do the resync again
 	 * with the same section */
-	bitmap_sync_with_cluster(mddev, cinfo->sync_low,
+	md_bitmap_sync_with_cluster(mddev, cinfo->sync_low,
 					cinfo->sync_hi,
 					lo, hi);
 	cinfo->sync_low = lo;

@@ -829,7 +829,7 @@ static int gather_all_resync_info(struct mddev *mddev, int total_slots)
 		}
 
 		/* Read the disk bitmap sb and check if it needs recovery */
-		ret = bitmap_copy_from_slot(mddev, i, &lo, &hi, false);
+		ret = md_bitmap_copy_from_slot(mddev, i, &lo, &hi, false);
 		if (ret) {
 			pr_warn("md-cluster: Could not gather bitmaps from slot %d", i);
 			lockres_free(bm_lockres);

@@ -1127,13 +1127,13 @@ static int cluster_check_sync_size(struct mddev *mddev)
 		bm_lockres = lockres_init(mddev, str, NULL, 1);
 		if (!bm_lockres) {
 			pr_err("md-cluster: Cannot initialize %s\n", str);
-			bitmap_free(bitmap);
+			md_bitmap_free(bitmap);
 			return -1;
 		}
 		bm_lockres->flags |= DLM_LKF_NOQUEUE;
 		rv = dlm_lock_sync(bm_lockres, DLM_LOCK_PW);
 		if (!rv)
-			bitmap_update_sb(bitmap);
+			md_bitmap_update_sb(bitmap);
 		lockres_free(bm_lockres);
 
 		sb = kmap_atomic(bitmap->storage.sb_page);

@@ -1141,11 +1141,11 @@ static int cluster_check_sync_size(struct mddev *mddev)
 			sync_size = sb->sync_size;
 		else if (sync_size != sb->sync_size) {
 			kunmap_atomic(sb);
-			bitmap_free(bitmap);
+			md_bitmap_free(bitmap);
 			return -1;
 		}
 		kunmap_atomic(sb);
-		bitmap_free(bitmap);
+		md_bitmap_free(bitmap);
 	}
 
 	return (my_sync_size == sync_size) ? 0 : -1;

@@ -1442,7 +1442,7 @@ static int gather_bitmaps(struct md_rdev *rdev)
 	for (sn = 0; sn < mddev->bitmap_info.nodes; sn++) {
 		if (sn == (cinfo->slot_number - 1))
 			continue;
-		err = bitmap_copy_from_slot(mddev, sn, &lo, &hi, false);
+		err = md_bitmap_copy_from_slot(mddev, sn, &lo, &hi, false);
 		if (err) {
 			pr_warn("md-cluster: Could not gather bitmaps from slot %d", sn);
 			goto out;
drivers/md/md.c +22 -22

154 lines not on this page · raw message

drivers/md/persistent-data/dm-space-map-common.c +6 -6
index 829b4ce057d8..049e999fc6c4 100644
@@ -69,7 +69,7 @@ static struct dm_block_validator index_validator = {
  */
 #define BITMAP_CSUM_XOR 240779
 
-static void bitmap_prepare_for_write(struct dm_block_validator *v,
+static void md_bitmap_prepare_for_write(struct dm_block_validator *v,
 				     struct dm_block *b,
 				     size_t block_size)
 {

@@ -81,7 +81,7 @@ static void bitmap_prepare_for_write(struct dm_block_validator *v,
 						       BITMAP_CSUM_XOR));
 }
 
-static int bitmap_check(struct dm_block_validator *v,
+static int md_bitmap_check(struct dm_block_validator *v,
 			struct dm_block *b,
 			size_t block_size)
 {

@@ -108,8 +108,8 @@ static int bitmap_check(struct dm_block_validator *v,
 
 static struct dm_block_validator dm_sm_bitmap_validator = {
 	.name = "sm_bitmap",
-	.prepare_for_write = bitmap_prepare_for_write,
-	.check = bitmap_check
+	.prepare_for_write = md_bitmap_prepare_for_write,
+	.check = md_bitmap_check,
 };
 
 /*----------------------------------------------------------------*/

@@ -124,7 +124,7 @@ static void *dm_bitmap_data(struct dm_block *b)
 
 #define WORD_MASK_HIGH 0xAAAAAAAAAAAAAAAAULL
 
-static unsigned bitmap_word_used(void *addr, unsigned b)
+static unsigned md_bitmap_word_used(void *addr, unsigned b)
 {
 	__le64 *words_le = addr;
 	__le64 *w_le = words_le + (b >> ENTRIES_SHIFT);

@@ -170,7 +170,7 @@ static int sm_find_free(void *addr, unsigned begin, unsigned end,
 {
 	while (begin < end) {
 		if (!(begin & (ENTRIES_PER_WORD - 1)) &&
-		    bitmap_word_used(addr, begin)) {
+		    md_bitmap_word_used(addr, begin)) {
 			begin += ENTRIES_PER_WORD;
 			continue;
 		}
drivers/md/raid1.c +10 -10
index 8e05c1092aef..ee6e0c26fada 100644
@@ -385,7 +385,7 @@ static void close_write(struct r1bio *r1_bio)
 		r1_bio->behind_master_bio = NULL;
 	}
 	/* clear the bitmap if all writes complete successfully */
-	bitmap_endwrite(r1_bio->mddev->bitmap, r1_bio->sector,
+	md_bitmap_endwrite(r1_bio->mddev->bitmap, r1_bio->sector,
 			r1_bio->sectors,
 			!test_bit(R1BIO_Degraded, &r1_bio->state),
 			test_bit(R1BIO_BehindIO, &r1_bio->state));

@@ -781,7 +781,7 @@ static int raid1_congested(struct mddev *mddev, int bits)
 static void flush_bio_list(struct r1conf *conf, struct bio *bio)
 {
 	/* flush any pending bitmap writes to disk before proceeding w/ I/O */
-	bitmap_unplug(conf->mddev->bitmap);
+	md_bitmap_unplug(conf->mddev->bitmap);
 	wake_up(&conf->wait_barrier);
 
 	while (bio) { /* submit pending writes */

@@ -1470,7 +1470,7 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
 				alloc_behind_master_bio(r1_bio, bio);
 			}
 
-			bitmap_startwrite(bitmap, r1_bio->sector,
+			md_bitmap_startwrite(bitmap, r1_bio->sector,
 					  r1_bio->sectors,
 					  test_bit(R1BIO_BehindIO,
 						   &r1_bio->state));

@@ -1880,7 +1880,7 @@ static void end_sync_write(struct bio *bio)
 		long sectors_to_go = r1_bio->sectors;
 		/* make sure these bits doesn't get cleared. */
 		do {
-			bitmap_end_sync(mddev->bitmap, s,
+			md_bitmap_end_sync(mddev->bitmap, s,
 					&sync_blocks, 1);
 			s += sync_blocks;
 			sectors_to_go -= sync_blocks;

@@ -2626,12 +2626,12 @@ static sector_t raid1_sync_request(struct mddev *mddev, sector_t sector_nr,
 		 * We can find the current addess in mddev->curr_resync
 		 */
 		if (mddev->curr_resync < max_sector) /* aborted */
-			bitmap_end_sync(mddev->bitmap, mddev->curr_resync,
+			md_bitmap_end_sync(mddev->bitmap, mddev->curr_resync,
 						&sync_blocks, 1);
 		else /* completed sync */
 			conf->fullsync = 0;
 
-		bitmap_close_sync(mddev->bitmap);
+		md_bitmap_close_sync(mddev->bitmap);
 		close_sync(conf);
 
 		if (mddev_is_clustered(mddev)) {

@@ -2651,7 +2651,7 @@ static sector_t raid1_sync_request(struct mddev *mddev, sector_t sector_nr,
 	/* before building a request, check if we can skip these blocks..
 	 * This call the bitmap_start_sync doesn't actually record anything
 	 */
-	if (!bitmap_start_sync(mddev->bitmap, sector_nr, &sync_blocks, 1) &&
+	if (!md_bitmap_start_sync(mddev->bitmap, sector_nr, &sync_blocks, 1) &&
 	    !conf->fullsync && !test_bit(MD_RECOVERY_REQUESTED, &mddev->recovery)) {
 		/* We can skip this block, and probably several more */
 		*skipped = 1;

@@ -2669,7 +2669,7 @@ static sector_t raid1_sync_request(struct mddev *mddev, sector_t sector_nr,
 	 * sector_nr + two times RESYNC_SECTORS
 	 */
 
-	bitmap_cond_end_sync(mddev->bitmap, sector_nr,
+	md_bitmap_cond_end_sync(mddev->bitmap, sector_nr,
 		mddev_is_clustered(mddev) && (sector_nr + 2 * RESYNC_SECTORS > conf->cluster_sync_high));
 
 

@@ -2828,7 +2828,7 @@ static sector_t raid1_sync_request(struct mddev *mddev, sector_t sector_nr,
 		if (len == 0)
 			break;
 		if (sync_blocks == 0) {
-			if (!bitmap_start_sync(mddev->bitmap, sector_nr,
+			if (!md_bitmap_start_sync(mddev->bitmap, sector_nr,
 					       &sync_blocks, still_degraded) &&
 			    !conf->fullsync &&
 			    !test_bit(MD_RECOVERY_REQUESTED, &mddev->recovery))

@@ -3165,7 +3165,7 @@ static int raid1_resize(struct mddev *mddev, sector_t sectors)
 	    mddev->array_sectors > newsize)
 		return -EINVAL;
 	if (mddev->bitmap) {
-		int ret = bitmap_resize(mddev->bitmap, newsize, 0, 0);
+		int ret = md_bitmap_resize(mddev->bitmap, newsize, 0, 0);
 		if (ret)
 			return ret;
 	}
drivers/md/raid10.c +13 -13
index 478cf446827f..2f71130ffd6c 100644
@@ -440,7 +440,7 @@ static void raid10_end_read_request(struct bio *bio)
 static void close_write(struct r10bio *r10_bio)
 {
 	/* clear the bitmap if all writes complete successfully */
-	bitmap_endwrite(r10_bio->mddev->bitmap, r10_bio->sector,
+	md_bitmap_endwrite(r10_bio->mddev->bitmap, r10_bio->sector,
 			r10_bio->sectors,
 			!test_bit(R10BIO_Degraded, &r10_bio->state),
 			0);

@@ -917,7 +917,7 @@ static void flush_pending_writes(struct r10conf *conf)
 		blk_start_plug(&plug);
 		/* flush any pending bitmap writes to disk
 		 * before proceeding w/ I/O */
-		bitmap_unplug(conf->mddev->bitmap);
+		md_bitmap_unplug(conf->mddev->bitmap);
 		wake_up(&conf->wait_barrier);
 
 		while (bio) { /* submit pending writes */

@@ -1102,7 +1102,7 @@ static void raid10_unplug(struct blk_plug_cb *cb, bool from_schedule)
 
 	/* we aren't scheduling, so we can do the write-out directly. */
 	bio = bio_list_get(&plug->pending);
-	bitmap_unplug(mddev->bitmap);
+	md_bitmap_unplug(mddev->bitmap);
 	wake_up(&conf->wait_barrier);
 
 	while (bio) { /* submit pending writes */

@@ -1519,7 +1519,7 @@ static void raid10_write_request(struct mddev *mddev, struct bio *bio,
 	}
 
 	atomic_set(&r10_bio->remaining, 1);
-	bitmap_startwrite(mddev->bitmap, r10_bio->sector, r10_bio->sectors, 0);
+	md_bitmap_startwrite(mddev->bitmap, r10_bio->sector, r10_bio->sectors, 0);
 
 	for (i = 0; i < conf->copies; i++) {
 		if (r10_bio->devs[i].bio)

@@ -2991,12 +2991,12 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
 
 		if (mddev->curr_resync < max_sector) { /* aborted */
 			if (test_bit(MD_RECOVERY_SYNC, &mddev->recovery))
-				bitmap_end_sync(mddev->bitmap, mddev->curr_resync,
+				md_bitmap_end_sync(mddev->bitmap, mddev->curr_resync,
 						&sync_blocks, 1);
 			else for (i = 0; i < conf->geo.raid_disks; i++) {
 				sector_t sect =
 					raid10_find_virt(conf, mddev->curr_resync, i);
-				bitmap_end_sync(mddev->bitmap, sect,
+				md_bitmap_end_sync(mddev->bitmap, sect,
 						&sync_blocks, 1);
 			}
 		} else {

@@ -3018,7 +3018,7 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
 			}
 			conf->fullsync = 0;
 		}
-		bitmap_close_sync(mddev->bitmap);
+		md_bitmap_close_sync(mddev->bitmap);
 		close_sync(conf);
 		*skipped = 1;
 		return sectors_skipped;

@@ -3112,7 +3112,7 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
 			 * we only need to recover the block if it is set in
 			 * the bitmap
 			 */
-			must_sync = bitmap_start_sync(mddev->bitmap, sect,
+			must_sync = md_bitmap_start_sync(mddev->bitmap, sect,
 						      &sync_blocks, 1);
 			if (sync_blocks < max_sync)
 				max_sync = sync_blocks;

@@ -3158,7 +3158,7 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
 				}
 			}
 
-			must_sync = bitmap_start_sync(mddev->bitmap, sect,
+			must_sync = md_bitmap_start_sync(mddev->bitmap, sect,
 						      &sync_blocks, still_degraded);
 
 			any_working = 0;

@@ -3335,12 +3335,12 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
 		 * safety reason, which ensures curr_resync_completed is
 		 * updated in bitmap_cond_end_sync.
 		 */
-		bitmap_cond_end_sync(mddev->bitmap, sector_nr,
+		md_bitmap_cond_end_sync(mddev->bitmap, sector_nr,
 				     mddev_is_clustered(mddev) &&
 				     (sector_nr + 2 * RESYNC_SECTORS >
 				      conf->cluster_sync_high));
 
-		if (!bitmap_start_sync(mddev->bitmap, sector_nr,
+		if (!md_bitmap_start_sync(mddev->bitmap, sector_nr,
 				       &sync_blocks, mddev->degraded) &&
 		    !conf->fullsync && !test_bit(MD_RECOVERY_REQUESTED,
 						 &mddev->recovery)) {

@@ -4015,7 +4015,7 @@ static int raid10_resize(struct mddev *mddev, sector_t sectors)
 	    mddev->array_sectors > size)
 		return -EINVAL;
 	if (mddev->bitmap) {
-		int ret = bitmap_resize(mddev->bitmap, size, 0, 0);
+		int ret = md_bitmap_resize(mddev->bitmap, size, 0, 0);
 		if (ret)
 			return ret;
 	}

@@ -4280,7 +4280,7 @@ static int raid10_start_reshape(struct mddev *mddev)
 	spin_unlock_irq(&conf->device_lock);
 
 	if (mddev->delta_disks && mddev->bitmap) {
-		ret = bitmap_resize(mddev->bitmap,
+		ret = md_bitmap_resize(mddev->bitmap,
 				    raid10_size(mddev, 0,
 						conf->geo.raid_disks),
 				    0, 0);
drivers/md/raid5-cache.c +1 -1
index 2b775abf377b..99f9081aefce 100644
@@ -324,7 +324,7 @@ void r5c_handle_cached_data_endio(struct r5conf *conf,
 		if (sh->dev[i].written) {
 			set_bit(R5_UPTODATE, &sh->dev[i].flags);
 			r5c_return_dev_pending_writes(conf, &sh->dev[i]);
-			bitmap_endwrite(conf->mddev->bitmap, sh->sector,
+			md_bitmap_endwrite(conf->mddev->bitmap, sh->sector,
 					STRIPE_SECTORS,
 					!test_bit(STRIPE_DEGRADED, &sh->state),
 					0);
drivers/md/raid5.c +12 -12
index 2031506a0ecd..8ce2c5467736 100644
@@ -3301,7 +3301,7 @@ static int add_stripe_bio(struct stripe_head *sh, struct bio *bi, int dd_idx,
 		 */
 		set_bit(STRIPE_BITMAP_PENDING, &sh->state);
 		spin_unlock_irq(&sh->stripe_lock);
-		bitmap_startwrite(conf->mddev->bitmap, sh->sector,
+		md_bitmap_startwrite(conf->mddev->bitmap, sh->sector,
 				  STRIPE_SECTORS, 0);
 		spin_lock_irq(&sh->stripe_lock);
 		clear_bit(STRIPE_BITMAP_PENDING, &sh->state);

@@ -3392,7 +3392,7 @@ handle_failed_stripe(struct r5conf *conf, struct stripe_head *sh,
 			bi = nextbi;
 		}
 		if (bitmap_end)
-			bitmap_endwrite(conf->mddev->bitmap, sh->sector,
+			md_bitmap_endwrite(conf->mddev->bitmap, sh->sector,
 				STRIPE_SECTORS, 0, 0);
 		bitmap_end = 0;
 		/* and fail all 'written' */

@@ -3438,7 +3438,7 @@ handle_failed_stripe(struct r5conf *conf, struct stripe_head *sh,
 			}
 		}
 		if (bitmap_end)
-			bitmap_endwrite(conf->mddev->bitmap, sh->sector,
+			md_bitmap_endwrite(conf->mddev->bitmap, sh->sector,
 					STRIPE_SECTORS, 0, 0);
 		/* If we were in the middle of a write the parity block might
 		 * still be locked - so just clear all R5_LOCKED flags

@@ -3779,7 +3779,7 @@ static void handle_stripe_clean_event(struct r5conf *conf,
 					bio_endio(wbi);
 					wbi = wbi2;
 				}
-				bitmap_endwrite(conf->mddev->bitmap, sh->sector,
+				md_bitmap_endwrite(conf->mddev->bitmap, sh->sector,
 						STRIPE_SECTORS,
 					 !test_bit(STRIPE_DEGRADED, &sh->state),
 						0);

@@ -5539,7 +5539,7 @@ static void make_discard_request(struct mddev *mddev, struct bio *bi)
 			for (d = 0;
 			     d < conf->raid_disks - conf->max_degraded;
 			     d++)
-				bitmap_startwrite(mddev->bitmap,
+				md_bitmap_startwrite(mddev->bitmap,
 						  sh->sector,
 						  STRIPE_SECTORS,
 						  0);

@@ -6020,11 +6020,11 @@ static inline sector_t raid5_sync_request(struct mddev *mddev, sector_t sector_n
 		}
 
 		if (mddev->curr_resync < max_sector) /* aborted */
-			bitmap_end_sync(mddev->bitmap, mddev->curr_resync,
+			md_bitmap_end_sync(mddev->bitmap, mddev->curr_resync,
 					&sync_blocks, 1);
 		else /* completed sync */
 			conf->fullsync = 0;
-		bitmap_close_sync(mddev->bitmap);
+		md_bitmap_close_sync(mddev->bitmap);
 
 		return 0;
 	}

@@ -6053,7 +6053,7 @@ static inline sector_t raid5_sync_request(struct mddev *mddev, sector_t sector_n
 	}
 	if (!test_bit(MD_RECOVERY_REQUESTED, &mddev->recovery) &&
 	    !conf->fullsync &&
-	    !bitmap_start_sync(mddev->bitmap, sector_nr, &sync_blocks, 1) &&
+	    !md_bitmap_start_sync(mddev->bitmap, sector_nr, &sync_blocks, 1) &&
 	    sync_blocks >= STRIPE_SECTORS) {
 		/* we can skip this block, and probably more */
 		sync_blocks /= STRIPE_SECTORS;

@@ -6061,7 +6061,7 @@ static inline sector_t raid5_sync_request(struct mddev *mddev, sector_t sector_n
 		return sync_blocks * STRIPE_SECTORS; /* keep things rounded to whole stripes */
 	}
 
-	bitmap_cond_end_sync(mddev->bitmap, sector_nr, false);
+	md_bitmap_cond_end_sync(mddev->bitmap, sector_nr, false);
 
 	sh = raid5_get_active_stripe(conf, sector_nr, 0, 1, 0);
 	if (sh == NULL) {

@@ -6084,7 +6084,7 @@ static inline sector_t raid5_sync_request(struct mddev *mddev, sector_t sector_n
 	}
 	rcu_read_unlock();
 
-	bitmap_start_sync(mddev->bitmap, sector_nr, &sync_blocks, still_degraded);
+	md_bitmap_start_sync(mddev->bitmap, sector_nr, &sync_blocks, still_degraded);
 
 	set_bit(STRIPE_SYNC_REQUESTED, &sh->state);
 	set_bit(STRIPE_HANDLE, &sh->state);

@@ -6285,7 +6285,7 @@ static void raid5d(struct md_thread *thread)
 			/* Now is a good time to flush some bitmap updates */
 			conf->seq_flush++;
 			spin_unlock_irq(&conf->device_lock);
-			bitmap_unplug(mddev->bitmap);
+			md_bitmap_unplug(mddev->bitmap);
 			spin_lock_irq(&conf->device_lock);
 			conf->seq_write = conf->seq_flush;
 			activate_bit_delay(conf, conf->temp_inactive_list);

@@ -7741,7 +7741,7 @@ static int raid5_resize(struct mddev *mddev, sector_t sectors)
 	    mddev->array_sectors > newsize)
 		return -EINVAL;
 	if (mddev->bitmap) {
-		int ret = bitmap_resize(mddev->bitmap, sectors, 0, 0);
+		int ret = md_bitmap_resize(mddev->bitmap, sectors, 0, 0);
 		if (ret)
 			return ret;
 	}
Signature · 1 line
2.17.1
#2 lore raw

Re: [PATCH v3 1/5] md: Avoid namespace collision with bitmap API

Quoted 8 lines · Andy Shevchenko

bitmap API (include/linux/bitmap.h) has 'bitmap' prefix for its methods.

On the other hand MD bitmap API is special case. Adding 'md' prefix to it to avoid name space collision.

No functional changes intended.

Signed-off-by Andy Shevchenko <[email protected]>
Quoted 12 lines
---
 drivers/md/dm-raid.c                          |   6 +-
 drivers/md/md-bitmap.c                        | 301 +++++++++---------
 drivers/md/md-bitmap.h                        |  46 +--
 drivers/md/md-cluster.c                       |  16 +-
 drivers/md/md.c                               |  44 +--
 .../md/persistent-data/dm-space-map-common.c  |  12 +-
 drivers/md/raid1.c                            |  20 +-
 drivers/md/raid10.c                           |  26 +-
 drivers/md/raid5-cache.c                      |   2 +-
 drivers/md/raid5.c                            |  24 +-
 10 files changed, 244 insertions(+), 253 deletions(-)

Seems my previous mail just missed your v3 cutoff but...

<snip>

All of these drivers/md/persistent-data/dm-space-map-common.c renames are wrong. DM is the only consumer (it is confusing because all DM and MD code lives in drivers/md/ despite only sharing a bit of MD code).

Anyway, please rename bitmap methods in this file to have a "dm_bitmap" prefix instead. Thanks.

Mike

Quoted 51 lines
drivers/md/persistent-data/dm-space-map-common.c +6 -6
index 829b4ce057d8..049e999fc6c4 100644
@@ -69,7 +69,7 @@ static struct dm_block_validator index_validator = {
  */
 #define BITMAP_CSUM_XOR 240779
 
-static void bitmap_prepare_for_write(struct dm_block_validator *v,
+static void md_bitmap_prepare_for_write(struct dm_block_validator *v,
 				     struct dm_block *b,
 				     size_t block_size)
 {

@@ -81,7 +81,7 @@ static void bitmap_prepare_for_write(struct dm_block_validator *v,
 						       BITMAP_CSUM_XOR));
 }
 
-static int bitmap_check(struct dm_block_validator *v,
+static int md_bitmap_check(struct dm_block_validator *v,
 			struct dm_block *b,
 			size_t block_size)
 {

@@ -108,8 +108,8 @@ static int bitmap_check(struct dm_block_validator *v,
 
 static struct dm_block_validator dm_sm_bitmap_validator = {
 	.name = "sm_bitmap",
-	.prepare_for_write = bitmap_prepare_for_write,
-	.check = bitmap_check
+	.prepare_for_write = md_bitmap_prepare_for_write,
+	.check = md_bitmap_check,
 };
 
 /*----------------------------------------------------------------*/

@@ -124,7 +124,7 @@ static void *dm_bitmap_data(struct dm_block *b)
 
 #define WORD_MASK_HIGH 0xAAAAAAAAAAAAAAAAULL
 
-static unsigned bitmap_word_used(void *addr, unsigned b)
+static unsigned md_bitmap_word_used(void *addr, unsigned b)
 {
 	__le64 *words_le = addr;
 	__le64 *w_le = words_le + (b >> ENTRIES_SHIFT);

@@ -170,7 +170,7 @@ static int sm_find_free(void *addr, unsigned begin, unsigned end,
 {
 	while (begin < end) {
 		if (!(begin & (ENTRIES_PER_WORD - 1)) &&
-		    bitmap_word_used(addr, begin)) {
+		    md_bitmap_word_used(addr, begin)) {
 			begin += ENTRIES_PER_WORD;
 			continue;
 		}
author
#3 lore raw

Re: [PATCH v3 1/5] md: Avoid namespace collision with bitmap API

Quoted 26 lines · Mike Snitzer
Quoted 9 lines · Andy Shevchenko

bitmap API (include/linux/bitmap.h) has 'bitmap' prefix for its methods.

On the other hand MD bitmap API is special case. Adding 'md' prefix to it to avoid name space collision.

No functional changes intended.

Signed-off-by Andy Shevchenko <[email protected]>
Quoted 13 lines
---
 drivers/md/dm-raid.c                          |   6 +-
 drivers/md/md-bitmap.c                        | 301 +++++++++----
-----
 drivers/md/md-bitmap.h                        |  46 +--
 drivers/md/md-cluster.c                       |  16 +-
 drivers/md/md.c                               |  44 +--
 .../md/persistent-data/dm-space-map-common.c  |  12 +-
 drivers/md/raid1.c                            |  20 +-
 drivers/md/raid10.c                           |  26 +-
 drivers/md/raid5-cache.c                      |   2 +-
 drivers/md/raid5.c                            |  24 +-
 10 files changed, 244 insertions(+), 253 deletions(-)
Quoted 6 lines

Seems my previous mail just missed your v3 cutoff but...

<snip>

All of these drivers/md/persistent-data/dm-space-map-common.c renames are wrong.

So, let me understand this clear, the naming problem appears in one file only. The rest is fine. Correct?

Quoted 6 lines

DM is the only consumer (it is confusing because all DM and
MD code lives in drivers/md/ despite only sharing a bit of MD code).

Anyway, please rename bitmap methods in this file to have a "dm_bitmap"
prefix instead. Thanks.

Ah, okay, will do this in v4 later this week.

Thanks for review!

Signature · 2 lines
Andy Shevchenko <[email protected]>
Intel Finland Oy
author
#4 lore raw

PATCH v3 2/5 bitmap: Drop unnecessary 0 check for u32 array operations

In reply to Andy Shevchenko

The nbits == 0 is safe to be supplied to the function body, so, remove unnecessary checks in bitmap_to_arr32() and bitmap_from_arr32().

Acked-by Yury Norov <[email protected]>
Signed-off-by Andy Shevchenko <[email protected]>
---
 lib/bitmap.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)
lib/bitmap.c +1 -8
index 58f9750e49c6..33e95cd359a2 100644
@@ -1132,14 +1132,10 @@ EXPORT_SYMBOL(bitmap_copy_le);
  *	@buf: array of u32 (in host byte order), the source bitmap
  *	@nbits: number of bits in @bitmap
  */
-void bitmap_from_arr32(unsigned long *bitmap, const u32 *buf,
-						unsigned int nbits)
+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];

@@ -1163,9 +1159,6 @@ 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);
Signature · 1 line
2.17.1
author
#5 lore raw

PATCH v3 3/5 bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()

In reply to Andy Shevchenko

A lot of code become ugly because of open coding allocations for bitmaps.

Introduce three helpers to allow users be more clear of intention and keep their code neat.

Signed-off-by Andy Shevchenko <[email protected]>
---
 include/linux/bitmap.h |  8 ++++++++
 lib/bitmap.c           | 19 +++++++++++++++++++
 2 files changed, 27 insertions(+)
include/linux/bitmap.h +8 -0
index 1ee46f492267..acf5e8df3504 100644
@@ -104,6 +104,14 @@
  * contain all bit positions from 0 to 'bits' - 1.
  */
 
+/*
+ * Allocation and deallocation of bitmap.
+ * Provided in lib/bitmap.c to avoid circular dependency.
+ */
+extern unsigned long *bitmap_alloc(unsigned int nbits, gfp_t flags);
+extern unsigned long *bitmap_zalloc(unsigned int nbits, gfp_t flags);
+extern void bitmap_free(const unsigned long *bitmap);
+
 /*
  * lib/bitmap.c provides these functions:
  */
lib/bitmap.c +19 -0
index 33e95cd359a2..09acf2fd6a35 100644
@@ -13,6 +13,7 @@
 #include <linux/bitops.h>
 #include <linux/bug.h>
 #include <linux/kernel.h>
+#include <linux/slab.h>
 #include <linux/string.h>
 #include <linux/uaccess.h>
 

@@ -1125,6 +1126,24 @@ void bitmap_copy_le(unsigned long *dst, const unsigned long *src, unsigned int n
 EXPORT_SYMBOL(bitmap_copy_le);
 #endif
 
+unsigned long *bitmap_alloc(unsigned int nbits, gfp_t flags)
+{
+	return kmalloc_array(BITS_TO_LONGS(nbits), sizeof(unsigned long), flags);
+}
+EXPORT_SYMBOL(bitmap_alloc);
+
+unsigned long *bitmap_zalloc(unsigned int nbits, gfp_t flags)
+{
+	return bitmap_alloc(nbits, flags | __GFP_ZERO);
+}
+EXPORT_SYMBOL(bitmap_zalloc);
+
+void bitmap_free(const unsigned long *bitmap)
+{
+	kfree(bitmap);
+}
+EXPORT_SYMBOL(bitmap_free);
+
 #if BITS_PER_LONG == 64
 /**
  * bitmap_from_arr32 - copy the contents of u32 array of bits to bitmap
Signature · 1 line
2.17.1
maintainer
#6 lore raw

Re: [PATCH v3 3/5] bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()

Quoted 15 lines · On Mon, 18 Jun 2018 16:10:01 +0300 Andy Shevchenko…

A lot of code become ugly because of open coding allocations for bitmaps.

Introduce three helpers to allow users be more clear of intention and keep their code neat.

...

lib/bitmap.c +8 -0
+unsigned long *bitmap_alloc(unsigned int nbits, gfp_t flags)
+{
+	return kmalloc_array(BITS_TO_LONGS(nbits), sizeof(unsigned long), flags);
+}
+EXPORT_SYMBOL(bitmap_alloc);
+
+unsigned long *bitmap_zalloc(unsigned int nbits, gfp_t flags)
+{

lib/bitmap.c void bitmap_copy_le(unsigned long *dst, const unsigned long *src, unsigned int n · line 1146

Quoted 10 lines
lib/bitmap.c +10 -0
+	return bitmap_alloc(nbits, flags | __GFP_ZERO);
+}
+EXPORT_SYMBOL(bitmap_zalloc);
+
+void bitmap_free(const unsigned long *bitmap)
+{
+	kfree(bitmap);
+}
+EXPORT_SYMBOL(bitmap_free);
+

I suggest these functions are small and simple enough to justify inlining them.

maintainer
#7 lore raw

Re: [PATCH v3 3/5] bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()

Quoted 23 lines · Andrew Morton
Quoted 20 lines · On Mon, 18 Jun 2018 16:10:01 +0300 Andy Shevchenko…

A lot of code become ugly because of open coding allocations for bitmaps.

Introduce three helpers to allow users be more clear of intention and keep their code neat.

...

lib/bitmap.c +13 -0
+unsigned long *bitmap_alloc(unsigned int nbits, gfp_t flags)
+{
+     return kmalloc_array(BITS_TO_LONGS(nbits), sizeof(unsigned long), flags);
+}
+EXPORT_SYMBOL(bitmap_alloc);
+
+unsigned long *bitmap_zalloc(unsigned int nbits, gfp_t flags)
+{
+     return bitmap_alloc(nbits, flags | __GFP_ZERO);
+}
+EXPORT_SYMBOL(bitmap_zalloc);
+
+void bitmap_free(const unsigned long *bitmap)
Quoted 9 lines

lib/bitmap.c void bitmap_copy_le(unsigned long *dst, const unsigned long *src, unsigned int n · line 1146

Quoted 5 lines
lib/bitmap.c +5 -0
+{
+     kfree(bitmap);
+}
+EXPORT_SYMBOL(bitmap_free);
+

I suggest these functions are small and simple enough to justify inlining them.

We can't as we end up including bitmap.h (by the way of cpumask.h) form slab.h, so we gen circular dependency. Maybe if we removed memcg stuff from slab.h so we do not need to include workqueue.h...

Signature · 1 line
Dmitry
#8 lore raw

Re: [PATCH v3 3/5] bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()

Quoted 34 lines · Dmitry Torokhov
Quoted 23 lines · Andrew Morton
Quoted 20 lines · On Mon, 18 Jun 2018 16:10:01 +0300 Andy Shevchenko…

A lot of code become ugly because of open coding allocations for bitmaps.

Introduce three helpers to allow users be more clear of intention and keep their code neat.

...

lib/bitmap.c +13 -0
+unsigned long *bitmap_alloc(unsigned int nbits, gfp_t flags)
+{
+     return kmalloc_array(BITS_TO_LONGS(nbits), sizeof(unsigned long), flags);
+}
+EXPORT_SYMBOL(bitmap_alloc);
+
+unsigned long *bitmap_zalloc(unsigned int nbits, gfp_t flags)
+{
+     return bitmap_alloc(nbits, flags | __GFP_ZERO);
+}
+EXPORT_SYMBOL(bitmap_zalloc);
+
+void bitmap_free(const unsigned long *bitmap)
Quoted 9 lines

lib/bitmap.c void bitmap_copy_le(unsigned long *dst, const unsigned long *src, unsigned int n · line 1146

Quoted 5 lines
lib/bitmap.c +5 -0
+{
+     kfree(bitmap);
+}
+EXPORT_SYMBOL(bitmap_free);
+

I suggest these functions are small and simple enough to justify inlining them.

Quoted 3 lines

We can't as we end up including bitmap.h (by the way of cpumask.h) form slab.h, so we gen circular dependency. Maybe if we removed memcg stuff from slab.h so we do not need to include workqueue.h...

I will look at it. It might be doable.
Though I dunno what MM people would say about this. Anyone's name comes to your mind to ask?

Signature · 2 lines
With Best Regards,
Andy Shevchenko
maintainer
#9 lore raw

Re: [PATCH v3 3/5] bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()

In reply to Dmitry Torokhov
Quoted 23 lines · On Mon, 18 Jun 2018 15:01:43 -0700 Dmitry Torokhov <[email protected]>…
Quoted 13 lines
Quoted 13 lines
lib/bitmap.c +13 -0
+unsigned long *bitmap_alloc(unsigned int nbits, gfp_t flags)
+{
+     return kmalloc_array(BITS_TO_LONGS(nbits), sizeof(unsigned long), flags);
+}
+EXPORT_SYMBOL(bitmap_alloc);
+
+unsigned long *bitmap_zalloc(unsigned int nbits, gfp_t flags)
+{
+     return bitmap_alloc(nbits, flags | __GFP_ZERO);
+}
+EXPORT_SYMBOL(bitmap_zalloc);
+
+void bitmap_free(const unsigned long *bitmap)
Quoted 9 lines

lib/bitmap.c void bitmap_copy_le(unsigned long *dst, const unsigned long *src, unsigned int n · line 1146

Quoted 5 lines
lib/bitmap.c +5 -0
+{
+     kfree(bitmap);
+}
+EXPORT_SYMBOL(bitmap_free);
+

I suggest these functions are small and simple enough to justify inlining them.

Quoted 2 lines

We can't as we end up including bitmap.h (by the way of cpumask.h) form slab.h, so we gen circular dependency.

That info should have been in the changelog, and probably a code comment.

Quoted 2 lines

Maybe if we removed memcg
stuff from slab.h so we do not need to include workqueue.h...

Or move the basic slab API stuff out of slab.h into a new header. Or create a new, standalone work_struct.h - that looks pretty simple.

#10 lore raw

Re: [PATCH v3 3/5] bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()

Quoted 25 lines · Andrew Morton
Quoted 23 lines · On Mon, 18 Jun 2018 15:01:43 -0700 Dmitry Torokhov <[email protected]>…
Quoted 22 lines
Quoted 8 lines
lib/bitmap.c +8 -0
+unsigned long *bitmap_alloc(unsigned int nbits, gfp_t flags)
+{
+     return kmalloc_array(BITS_TO_LONGS(nbits), sizeof(unsigned long), flags);
+}
+EXPORT_SYMBOL(bitmap_alloc);
+
+unsigned long *bitmap_zalloc(unsigned int nbits, gfp_t flags)
+{

lib/bitmap.c void bitmap_copy_le(unsigned long *dst, const unsigned long *src, unsigned int n · line 1146

Quoted 10 lines
lib/bitmap.c +10 -0
+     return bitmap_alloc(nbits, flags | __GFP_ZERO);
+}
+EXPORT_SYMBOL(bitmap_zalloc);
+
+void bitmap_free(const unsigned long *bitmap)
+{
+     kfree(bitmap);
+}
+EXPORT_SYMBOL(bitmap_free);
+

I suggest these functions are small and simple enough to justify inlining them.

Quoted 4 lines
Quoted 2 lines

We can't as we end up including bitmap.h (by the way of cpumask.h) form slab.h, so we gen circular dependency.

That info should have been in the changelog,

I put it in cover letter, though it perhaps better to have in commit message itself.

Quoted 2 lines

and probably a code
comment.

This is done in header file. You meant C-file?

Quoted 5 lines
Quoted 2 lines

Maybe if we removed memcg
stuff from slab.h so we do not need to include workqueue.h...

Or move the basic slab API stuff out of slab.h into a new header. Or create a new, standalone work_struct.h - that looks pretty simple.

Latter one seems requires least effort without potentially breaking things. I take it as your suggestion, though I would still give a glance if it is possible to split exactly memcg part out of slab.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#11 lore raw

Re: [PATCH v3 3/5] bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()

In reply to Andrew Morton
Quoted 5 lines · Andrew Morton
Quoted 2 lines · On Mon, 18 Jun 2018 15:01:43 -0700 Dmitry Torokhov <[email protected]>…

We can't as we end up including bitmap.h (by the way of cpumask.h) form slab.h, so we gen circular dependency.

It's not just so easy. See below.

Quoted 8 lines

That info should have been in the changelog, and probably a code comment.

Quoted 2 lines

Maybe if we removed memcg
stuff from slab.h so we do not need to include workqueue.h...

Or move the basic slab API stuff out of slab.h into a new header. Or create a new, standalone work_struct.h - that looks pretty simple.

I tried to move out work_struct, it didn't help. There are actually several circular dependencies that ends in bitmap.h either way or another.

First one is

slab.h -> gfp.h -> mmzone.h -> nodemask.h -> bitmap.h

And so on...

Splitting out kXalloc stuff to a separate header won't help, I think, because of the above.
Splitting out struct work_struct is just a tip of an iceberg. Splitting out memcg stuff won't help in the similar way.

I'm all ears for (a better) solution.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
maintainer
#12 lore raw

Re: [PATCH v3 3/5] bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()

Quoted 27 lines · Andy Shevchenko
Quoted 5 lines · Andrew Morton
Quoted 2 lines · On Mon, 18 Jun 2018 15:01:43 -0700 Dmitry Torokhov <[email protected]>…

We can't as we end up including bitmap.h (by the way of cpumask.h) form slab.h, so we gen circular dependency.

It's not just so easy. See below.

Quoted 8 lines

That info should have been in the changelog, and probably a code comment.

Quoted 2 lines

Maybe if we removed memcg
stuff from slab.h so we do not need to include workqueue.h...

Or move the basic slab API stuff out of slab.h into a new header. Or create a new, standalone work_struct.h - that looks pretty simple.

I tried to move out work_struct, it didn't help. There are actually several circular dependencies that ends in bitmap.h either way or another.

First one is

slab.h -> gfp.h -> mmzone.h -> nodemask.h -> bitmap.h

Quoted 8 lines

And so on...

Splitting out kXalloc stuff to a separate header won't help, I think, because of the above.
Splitting out struct work_struct is just a tip of an iceberg. Splitting out memcg stuff won't help in the similar way.

I'm all ears for (a better) solution.

I think ultimately we'd want to untangle this, but allocating bitmaps is not in any hot paths so having them as non-inlined functions should not hurt us that much for time being.

Just my 2 cents...

Signature · 1 line
Dmitry
author
#13 lore raw

Re: [PATCH v3 3/5] bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()

Quoted 37 lines · Dmitry Torokhov
Quoted 36 lines · Andy Shevchenko
Quoted 5 lines · Andrew Morton
Quoted 3 lines · On Mon, 18 Jun 2018 15:01:43 -0700 Dmitry Torokhov <dmitry.torokho…

We can't as we end up including bitmap.h (by the way of cpumask.h)
form slab.h, so we gen circular dependency.

It's not just so easy. See below.

Quoted 9 lines

That info should have been in the changelog, and probably a code comment.

Quoted 2 lines

Maybe if we removed memcg
stuff from slab.h so we do not need to include workqueue.h...

Or move the basic slab API stuff out of slab.h into a new header. Or
create a new, standalone work_struct.h - that looks pretty simple.

I tried to move out work_struct, it didn't help. There are actually several circular dependencies that ends in bitmap.h either way or another.

First one is

slab.h -> gfp.h -> mmzone.h -> nodemask.h -> bitmap.h

And so on...

Splitting out kXalloc stuff to a separate header won't help, I think,
because of the above.
Splitting out struct work_struct is just a tip of an iceberg. Splitting out memcg stuff won't help in the similar way.

Quoted 7 lines
Quoted 1 line

I'm all ears for (a better) solution.

I think ultimately we'd want to untangle this, but allocating bitmaps is
not in any hot paths so having them as non-inlined functions should not
hurt us that much for time being.

Perhaps I can elaborate a bit in a commit message.

Thanks for review!

Signature · 2 lines
Andy Shevchenko <[email protected]>
Intel Finland Oy
author
#14 lore raw

PATCH v3 4/5 Input: gpio-keys - Switch to bitmap_zalloc()

In reply to Andy Shevchenko

Switch to bitmap_zalloc() to show clearly what we are allocating. Besides that it returns pointer of bitmap type instead of opaque void *.

Acked-by Dmitry Torokhov <[email protected]>
Signed-off-by Andy Shevchenko <[email protected]>
---
 drivers/input/keyboard/gpio_keys.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/input/keyboard/gpio_keys.c +4 -4
index 052e37675086..492a971b95b5 100644
@@ -196,7 +196,7 @@ static ssize_t gpio_keys_attr_show_helper(struct gpio_keys_drvdata *ddata,
 	ssize_t ret;
 	int i;
 
-	bits = kcalloc(BITS_TO_LONGS(n_events), sizeof(*bits), GFP_KERNEL);
+	bits = bitmap_zalloc(n_events, GFP_KERNEL);
 	if (!bits)
 		return -ENOMEM;
 

@@ -216,7 +216,7 @@ static ssize_t gpio_keys_attr_show_helper(struct gpio_keys_drvdata *ddata,
 	buf[ret++] = '\n';
 	buf[ret] = '\0';
 
-	kfree(bits);
+	bitmap_free(bits);
 
 	return ret;
 }

@@ -240,7 +240,7 @@ static ssize_t gpio_keys_attr_store_helper(struct gpio_keys_drvdata *ddata,
 	ssize_t error;
 	int i;
 
-	bits = kcalloc(BITS_TO_LONGS(n_events), sizeof(*bits), GFP_KERNEL);
+	bits = bitmap_zalloc(n_events, GFP_KERNEL);
 	if (!bits)
 		return -ENOMEM;
 

@@ -284,7 +284,7 @@ static ssize_t gpio_keys_attr_store_helper(struct gpio_keys_drvdata *ddata,
 	mutex_unlock(&ddata->disable_lock);
 
 out:
-	kfree(bits);
+	bitmap_free(bits);
 	return error;
 }
Signature · 1 line
2.17.1
author
#15 lore raw

PATCH v3 5/5 Input: evdev - Switch to bitmap API

In reply to Andy Shevchenko

Switch to bitmap API, i.e. bitmap_alloc(), bitmap_zalloc(), to show clearly what we are allocating. Besides that it returns pointer of bitmap type instead of opaque void *.

While here, replace memcpy() with bitmap_copy() for sake of consistency.

Acked-by Dmitry Torokhov <[email protected]>
Signed-off-by Andy Shevchenko <[email protected]>
---
 drivers/input/evdev.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)
drivers/input/evdev.c +7 -9
index c81c79d01d93..370206f987f9 100644
@@ -481,7 +481,7 @@ static int evdev_release(struct inode *inode, struct file *file)
 	evdev_detach_client(evdev, client);
 
 	for (i = 0; i < EV_CNT; ++i)
-		kfree(client->evmasks[i]);
+		bitmap_free(client->evmasks[i]);
 
 	kvfree(client);
 

@@ -925,17 +925,15 @@ static int evdev_handle_get_val(struct evdev_client *client,
 {
 	int ret;
 	unsigned long *mem;
-	size_t len;
 
-	len = BITS_TO_LONGS(maxbit) * sizeof(unsigned long);
-	mem = kmalloc(len, GFP_KERNEL);
+	mem = bitmap_alloc(maxbit, GFP_KERNEL);
 	if (!mem)
 		return -ENOMEM;
 
 	spin_lock_irq(&dev->event_lock);
 	spin_lock(&client->buffer_lock);
 
-	memcpy(mem, bits, len);
+	bitmap_copy(mem, bits, maxbit);
 
 	spin_unlock(&dev->event_lock);
 

@@ -947,7 +945,7 @@ static int evdev_handle_get_val(struct evdev_client *client,
 	if (ret < 0)
 		evdev_queue_syn_dropped(client);
 
-	kfree(mem);
+	bitmap_free(mem);
 
 	return ret;
 }

@@ -1003,13 +1001,13 @@ static int evdev_set_mask(struct evdev_client *client,
 	if (!cnt)
 		return 0;
 
-	mask = kcalloc(sizeof(unsigned long), BITS_TO_LONGS(cnt), GFP_KERNEL);
+	mask = bitmap_zalloc(cnt, GFP_KERNEL);
 	if (!mask)
 		return -ENOMEM;
 
 	error = bits_from_user(mask, cnt - 1, codes_size, codes, compat);
 	if (error < 0) {
-		kfree(mask);
+		bitmap_free(mask);
 		return error;
 	}
 

@@ -1018,7 +1016,7 @@ static int evdev_set_mask(struct evdev_client *client,
 	client->evmasks[type] = mask;
 	spin_unlock_irqrestore(&client->buffer_lock, flags);
 
-	kfree(oldmask);
+	bitmap_free(oldmask);
 
 	return 0;
 }
Signature · 1 line
2.17.1
#16 lore raw
In reply to Andy Shevchenko
Quoted 7 lines · Andy Shevchenko

External Email

A lot of code is using allocation of bitmaps using BITS_PER_LONG() macro and sizeof(unsigned long) operator. The readability suffers because of this.

The series introduces three helpers, i.e. bitmap_alloc(), bitmap_zalloc() and bitmap_free(), to make it more cleaner.

tools/include/linux/bitmap.h already has bitmap_alloc(), and it corresponds to bitmap_zalloc() in this patch. It may cause problems in future if people will copy functions that use bitmap_alloc between kernel code and tools. So I think we have to propagate this API to tools and update existing users of bitmap_alloc() in tools.

What about code that calls specific alloc functions, like memblock_virt_alloc() and pcpu_mem_zalloc() in mm/percpu.c, or devm_kcalloc() in drivers/dma/edma.c? If we are going to unify bitmap allocations in kernel, we should think about unification of that cases too. Should it be additional flag or optional pointer to the exact allocator in bitmap_{,z}alloc()?

Yury

Quoted 46 lines

Patch 1 is a preparatory to avoid namespace collisions between bitmap API and MD bitmap. No functional changes intended.

Patch 2 is just orphaned from previous release cycle.

Patch 3 introduces new helpers.

Patches 4 and 5 is just an example how to use new helpers. Locally I have like dozen of them against different subsystems and drivers.

Ideally it would go through Input subsystem, thus, needs an Ack from MD maintainer(s).

Since v2:
- fix compilation issue in MD bitmap code
- elaborate changes in commit message of patch 5

Since v1:
- added namespace fix patch against MD bitmap API
- moved functions to lib/bitmap.c to avoid circular dependencies
- appended Dmitry's tags

Andy Shevchenko (5):
md: Avoid namespace collision with bitmap API
bitmap: Drop unnecessary 0 check for u32 array operations
bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()
Input: gpio-keys - Switch to bitmap_zalloc()
Input: evdev - Switch to bitmap API

 drivers/input/evdev.c                         |  16 +-
 drivers/input/keyboard/gpio_keys.c            |   8 +-
 drivers/md/dm-raid.c                          |   6 +-
 drivers/md/md-bitmap.c                        | 301 +++++++++---------
 drivers/md/md-bitmap.h                        |  46 +--
 drivers/md/md-cluster.c                       |  16 +-
 drivers/md/md.c                               |  44 +--
 .../md/persistent-data/dm-space-map-common.c  |  12 +-
 drivers/md/raid1.c                            |  20 +-
 drivers/md/raid10.c                           |  26 +-
 drivers/md/raid5-cache.c                      |   2 +-
 drivers/md/raid5.c                            |  24 +-
 include/linux/bitmap.h                        |   8 +
 lib/bitmap.c                                  |  28 +-
 14 files changed, 283 insertions(+), 274 deletions(-)

--
2.17.1

author
#17 lore raw
Quoted 8 lines · Yury Norov
Quoted 7 lines · Andy Shevchenko

External Email

A lot of code is using allocation of bitmaps using BITS_PER_LONG() macro and
sizeof(unsigned long) operator. The readability suffers because of this.

Quoted 6 lines
Quoted 3 lines

The series introduces three helpers, i.e. bitmap_alloc(), bitmap_zalloc() and
bitmap_free(), to make it more cleaner.

tools/include/linux/bitmap.h already has bitmap_alloc(),

TBH, I don't give a crap about tools. They invented something that might or might not follow kernel APIs. At the end, it's a user space.

Quoted 5 lines

and it corresponds to bitmap_zalloc() in this patch. It may cause problems in future if people will copy functions that use bitmap_alloc between kernel code and tools. So I think we have to propagate this API to tools and update existing users of bitmap_alloc() in tools.

Propose a patch then. Perhaps I need to inform tools people about new API coming, but that's all what I can do. Existing something in tools/ does not and should not prevent extending / changing kernel internal APIs.

Quoted 2 lines

What about code that calls specific alloc functions, like memblock_virt_alloc() and pcpu_mem_zalloc() in mm/percpu.c,

What about it? It's not in scope of this API for sure. The above mentioned functions have a very limited area of usage.
Your example also a bit complicated, since it's not allocating _just_ a bitmap, but a structure _with_ embedded bitmap.

Quoted 1 line

or devm_kcalloc() in drivers/dma/edma.c?

There is no such file in the tree. You perhaps referred to drivers/dma/ti/edma.c.

Yes, this is a candidate to convert later on if anyone wants to do it (with introducing devm_bitmap_alloc() / devm_bitmap_zalloc() helpers).

Quoted 5 lines

If we are going to
unify bitmap allocations in kernel, we should think about unification of that cases too. Should it be additional flag or optional pointer to the exact allocator in bitmap_{,z}alloc()?

So, I prefer one step at a time. Especially taking into consideration the problems I have to solve now with those simplest helpers I proposed.

Quoted 53 lines

Yury

Quoted 50 lines

Patch 1 is a preparatory to avoid namespace collisions between bitmap API and
MD bitmap. No functional changes intended.

Patch 2 is just orphaned from previous release cycle.

Patch 3 introduces new helpers.

Patches 4 and 5 is just an example how to use new helpers. Locally I have like
dozen of them against different subsystems and drivers.

Ideally it would go through Input subsystem, thus, needs an Ack from MD maintainer(s).

Since v2:
- fix compilation issue in MD bitmap code
- elaborate changes in commit message of patch 5

Since v1:
- added namespace fix patch against MD bitmap API
- moved functions to lib/bitmap.c to avoid circular dependencies
- appended Dmitry's tags

Andy Shevchenko (5):
md: Avoid namespace collision with bitmap API
bitmap: Drop unnecessary 0 check for u32 array operations
bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()
Input: gpio-keys - Switch to bitmap_zalloc()
Input: evdev - Switch to bitmap API

 drivers/input/evdev.c                         |  16 +-
 drivers/input/keyboard/gpio_keys.c            |   8 +-
 drivers/md/dm-raid.c                          |   6 +-
 drivers/md/md-bitmap.c                        | 301 +++++++++----
-----
 drivers/md/md-bitmap.h                        |  46 +--
 drivers/md/md-cluster.c                       |  16 +-
 drivers/md/md.c                               |  44 +--
 .../md/persistent-data/dm-space-map-common.c  |  12 +-
 drivers/md/raid1.c                            |  20 +-
 drivers/md/raid10.c                           |  26 +-
 drivers/md/raid5-cache.c                      |   2 +-
 drivers/md/raid5.c                            |  24 +-
 include/linux/bitmap.h                        |   8 +
 lib/bitmap.c                                  |  28 +-
 14 files changed, 283 insertions(+), 274 deletions(-)

--
2.17.1

Signature · 2 lines
Andy Shevchenko <[email protected]>
Intel Finland Oy