mmc: block: Add write packing control

The write packing control will ensure that read requests latency is not increased due to long write packed commands.

The trigger for enabling the write packing is managing to pack several write requests. The number of potential packed requests that will trigger the packing can be configured via sysfs by writing the required value to:
/sys/block/<block_dev_name>/num_wr_reqs_to_start_packing.
The trigger for disabling the write packing is fetching a read request.

---
 Documentation/mmc/mmc-dev-attrs.txt |   17 ++++++
 drivers/mmc/card/block.c            |  100 ++++++++++++++++++++++++++++++++++-
 drivers/mmc/card/queue.c            |    8 +++
 drivers/mmc/card/queue.h            |    3 +
 include/linux/mmc/host.h            |    1 +
 5 files changed, 128 insertions(+), 1 deletions(-)

Documentation/mmc/mmc-dev-attrs.txt +17 -0

index 22ae844..08f7312 100644
@@ -8,6 +8,23 @@ The following attributes are read/write.
 
 	force_ro		Enforce read-only access even if write protect switch is off.
 
+	num_wr_reqs_to_start_packing 	This attribute is used to determine
+	the trigger for activating the write packing, in case the write
+	packing control feature is enabled.
+
+	When the MMC manages to reach a point where num_wr_reqs_to_start_packing
+	write requests could be packed, it enables the write packing feature.
+	This allows us to start the write packing only when it is beneficial
+	and has minimum affect on the read latency.
+
+	The number of potential packed requests that will trigger the packing
+	can be configured via sysfs by writing the required value to:
+	/sys/block/<block_dev_name>/num_wr_reqs_to_start_packing.
+
+	The default value of num_wr_reqs_to_start_packing was determined by
+	running parallel lmdd write and lmdd read operations and calculating
+	the max number of packed writes requests.
+
 SD and MMC Device Attributes
 ============================
 

drivers/mmc/card/block.c +62 -0

index 2785fd4..ef192fb 100644
@@ -114,6 +114,7 @@ struct mmc_blk_data {
 	struct device_attribute force_ro;
 	struct device_attribute power_ro_lock;
 	int	area_type;
+	struct device_attribute num_wr_reqs_to_start_packing;
 };
 
 static DEFINE_MUTEX(open_lock);

@@ -281,6 +282,38 @@ out:
 	return ret;
 }
 
+static ssize_t
+num_wr_reqs_to_start_packing_show(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct mmc_blk_data *md = mmc_blk_get(dev_to_disk(dev));
+	int num_wr_reqs_to_start_packing;
+	int ret;
+
+	num_wr_reqs_to_start_packing = md->queue.num_wr_reqs_to_start_packing;
+
+	ret = snprintf(buf, PAGE_SIZE, "%d\n", num_wr_reqs_to_start_packing);
+
+	mmc_blk_put(md);
+	return ret;
+}
+
+static ssize_t
+num_wr_reqs_to_start_packing_store(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	int value;
+	struct mmc_blk_data *md = mmc_blk_get(dev_to_disk(dev));
+
+	sscanf(buf, "%d", &value);
+	if (value >= 0)
+		md->queue.num_wr_reqs_to_start_packing = value;
+
+	mmc_blk_put(md);
+	return count;
+}
+
 static int mmc_blk_open(struct block_device *bdev, fmode_t mode)
 {
 	struct mmc_blk_data *md = mmc_blk_get(bdev->bd_disk);

@@ -1313,6 +1346,48 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
 	mmc_queue_bounce_pre(mqrq);
 }
 
+static void mmc_blk_write_packing_control(struct mmc_queue *mq,
+					  struct request *req)
+{
+	struct mmc_host *host = mq->card->host;
+	int data_dir;
+
+	if (!(host->caps2 & MMC_CAP2_PACKED_WR))
+		return;
+
+	/*
+	 * In case the packing control is not supported by the host, it should
+	 * not have an effect on the write packing. Therefore we have to enable
+	 * the write packing
+	 */
+	if (!(host->caps2 & MMC_CAP2_PACKED_WR_CONTROL)) {
+		mq->wr_packing_enabled = true;
+		return;
+	}
+
+	if (!req || (req && (req->cmd_flags & REQ_FLUSH))) {
+		if (mq->num_of_potential_packed_wr_reqs >
+				mq->num_wr_reqs_to_start_packing)
+			mq->wr_packing_enabled = true;
+		return;
+	}
+
+	data_dir = rq_data_dir(req);
+
+	if (data_dir == READ) {

drivers/mmc/card/block.c +22 -1

+
 	if ((rq_data_dir(cur) == WRITE) &&
 			(card->host->caps2 & MMC_CAP2_PACKED_WR))
 		max_packed_rw = card->ext_csd.max_packed_writes;

@@ -1396,6 +1474,8 @@ static u8 mmc_blk_prep_packed_list(struct mmc_queue *mq, struct request *req)
 			break;
 		}
 
+		if (rq_data_dir(next) == WRITE)
+			mq->num_of_potential_packed_wr_reqs++;
 		list_add_tail(&next->queuelist, &mq->mqrq_cur->packed_list);
 		cur = next;
 		reqs++;

@@ -1780,7 +1860,9 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
 		goto out;
 	}
 
-	if (req && req->cmd_flags & REQ_DISCARD) {
+	mmc_blk_write_packing_control(mq, req);
+
+	if (req && req->cmd_flags & REQ_DISCARD) {
 		/* complete ongoing async transfer before issuing discard */
 		if (card->host->areq)
 			mmc_blk_issue_rw_rq(mq, NULL);

@@ -2010,6 +2092,8 @@ static void mmc_blk_remove_req(struct mmc_blk_data *md)
 
 	if (md) {
 		card = md->queue.card;
+		device_remove_file(disk_to_dev(md->disk),
+				   &md->num_wr_reqs_to_start_packing);
 		if (md->disk->flags & GENHD_FL_UP) {
 			device_remove_file(disk_to_dev(md->disk), &md->force_ro);
 			if ((md->area_type & MMC_BLK_DATA_AREA_BOOT) &&

@@ -2076,6 +2160,20 @@ static int mmc_add_disk(struct mmc_blk_data *md)
 		if (ret)
 			goto power_ro_lock_fail;
 	}
+
+	md->num_wr_reqs_to_start_packing.show =
+		num_wr_reqs_to_start_packing_show;
+	md->num_wr_reqs_to_start_packing.store =
+		num_wr_reqs_to_start_packing_store;
+	sysfs_attr_init(&md->num_wr_reqs_to_start_packing.attr);
+	md->num_wr_reqs_to_start_packing.attr.name =
+		"num_wr_reqs_to_start_packing";
+	md->num_wr_reqs_to_start_packing.attr.mode = S_IRUGO | S_IWUSR;
+	ret = device_create_file(disk_to_dev(md->disk),
+				 &md->num_wr_reqs_to_start_packing);
+	if (ret)
+		goto power_ro_lock_fail;
+
 	return ret;
 
 power_ro_lock_fail:

drivers/mmc/card/queue.c +8 -0

index 165d85a..79ef91b 100644
@@ -25,6 +25,13 @@
 #define MMC_QUEUE_SUSPENDED	(1 << 0)
 
 /*
+ * Based on benchmark tests the default num of requests to trigger the write
+ * packing was determined, to keep the read latency as low as possible and
+ * manage to keep the high write throughput.
+ */
+#define DEFAULT_NUM_REQS_TO_START_PACK 17
+
+/*
  * Prepare a MMC request. This just filters out odd stuff.
  */
 static int mmc_prep_request(struct request_queue *q, struct request *req)

@@ -181,6 +188,7 @@ int mmc_init_queue(struct mmc_queue *mq, struct mmc_card *card,
 	mq->mqrq_cur = mqrq_cur;
 	mq->mqrq_prev = mqrq_prev;
 	mq->queue->queuedata = mq;
+	mq->num_wr_reqs_to_start_packing = DEFAULT_NUM_REQS_TO_START_PACK;
 
 	blk_queue_prep_rq(mq->queue, mmc_prep_request);
 	queue_flag_set_unlocked(QUEUE_FLAG_NONROT, mq->queue);

drivers/mmc/card/queue.h +3 -0

index d761bf1..6c29e0e 100644
@@ -44,6 +44,9 @@ struct mmc_queue {
 	struct mmc_queue_req	mqrq[2];
 	struct mmc_queue_req	*mqrq_cur;
 	struct mmc_queue_req	*mqrq_prev;
+	bool			wr_packing_enabled;
+	int			num_of_potential_packed_wr_reqs;
+	int			num_wr_reqs_to_start_packing;
 };
 
 extern int mmc_init_queue(struct mmc_queue *, struct mmc_card *, spinlock_t *,

include/linux/mmc/host.h +1 -1

index 9d0d946..0eb6c7b 100644
@@ -242,6 +242,7 @@ struct mmc_host {
 #define MMC_CAP2_PACKED_WR	(1 << 11)	/* Allow packed write */
 #define MMC_CAP2_PACKED_CMD	(MMC_CAP2_PACKED_RD | \
 				 MMC_CAP2_PACKED_WR) /* Allow packed commands */
+#define MMC_CAP2_PACKED_WR_CONTROL (1 << 12) /* Allow write packing control */
 
 	mmc_pm_flag_t		pm_caps;	/* supported pm features */
 	unsigned int        power_notify_type;
--

1.7.3.3
--
Sent by a consultant of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum. --
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi,

How can we check the effect?
Do you have any result?

Please check the several comment below.

Quoted 143 lines · Maya Erez

The write packing control will ensure that read requests latency is not increased due to long write packed commands.

The trigger for enabling the write packing is managing to pack several write requests. The number of potential packed requests that will trigger
the packing can be configured via sysfs by writing the required value to:
/sys/block/<block_dev_name>/num_wr_reqs_to_start_packing. The trigger for disabling the write packing is fetching a read request.

---
 Documentation/mmc/mmc-dev-attrs.txt |   17 ++++++
 drivers/mmc/card/block.c            |  100
++++++++++++++++++++++++++++++++++-
 drivers/mmc/card/queue.c            |    8 +++
 drivers/mmc/card/queue.h            |    3 +
 include/linux/mmc/host.h            |    1 +
 5 files changed, 128 insertions(+), 1 deletions(-)

Documentation/mmc/mmc-dev-attrs.txt +0 -0

index 22ae844..08f7312 100644
@@ -8,6 +8,23 @@ The following attributes are read/write.
 
 	force_ro		Enforce read-only access even if write protect switch is

off.

Documentation/mmc/mmc-dev-attrs.txt +5 -0

+	num_wr_reqs_to_start_packing 	This attribute is used to determine
+	the trigger for activating the write packing, in case the write
+	packing control feature is enabled.
+
+	When the MMC manages to reach a point where

num_wr_reqs_to_start_packing

Documentation/mmc/mmc-dev-attrs.txt +12 -0

+	write requests could be packed, it enables the write packing feature.
+	This allows us to start the write packing only when it is beneficial
+	and has minimum affect on the read latency.
+
+	The number of potential packed requests that will trigger the packing
+	can be configured via sysfs by writing the required value to:
+	/sys/block/<block_dev_name>/num_wr_reqs_to_start_packing.
+
+	The default value of num_wr_reqs_to_start_packing was determined by
+	running parallel lmdd write and lmdd read operations and calculating
+	the max number of packed writes requests.
+
 SD and MMC Device Attributes
 ============================

drivers/mmc/card/block.c +45 -0

index 2785fd4..ef192fb 100644
@@ -114,6 +114,7 @@ struct mmc_blk_data {
 	struct device_attribute force_ro;
 	struct device_attribute power_ro_lock;
 	int	area_type;
+	struct device_attribute num_wr_reqs_to_start_packing;
 };
 
 static DEFINE_MUTEX(open_lock);

@@ -281,6 +282,38 @@ out:
 	return ret;
 }
 
+static ssize_t
+num_wr_reqs_to_start_packing_show(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct mmc_blk_data *md = mmc_blk_get(dev_to_disk(dev));
+	int num_wr_reqs_to_start_packing;
+	int ret;
+
+	num_wr_reqs_to_start_packing = md->queue.num_wr_reqs_to_start_packing;
+
+	ret = snprintf(buf, PAGE_SIZE, "%d\n", num_wr_reqs_to_start_packing);
+
+	mmc_blk_put(md);
+	return ret;
+}
+
+static ssize_t
+num_wr_reqs_to_start_packing_store(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	int value;
+	struct mmc_blk_data *md = mmc_blk_get(dev_to_disk(dev));
+
+	sscanf(buf, "%d", &value);
+	if (value >= 0)
+		md->queue.num_wr_reqs_to_start_packing = value;
+
+	mmc_blk_put(md);
+	return count;
+}
+
 static int mmc_blk_open(struct block_device *bdev, fmode_t mode)
 {
 	struct mmc_blk_data *md = mmc_blk_get(bdev->bd_disk);

@@ -1313,6 +1346,48 @@ static void mmc_blk_rw_rq_prep(struct
 mmc_queue_req *mqrq,
 	mmc_queue_bounce_pre(mqrq);
 }
 
+static void mmc_blk_write_packing_control(struct mmc_queue *mq,
+					  struct request *req)
+{
+	struct mmc_host *host = mq->card->host;
+	int data_dir;
+
+	if (!(host->caps2 & MMC_CAP2_PACKED_WR))
+		return;
+
+	/*
+	 * In case the packing control is not supported by the host, it should
+	 * not have an effect on the write packing. Therefore we have to

enable

drivers/mmc/card/block.c +23 -0

+	 * the write packing
+	 */
+	if (!(host->caps2 & MMC_CAP2_PACKED_WR_CONTROL)) {
+		mq->wr_packing_enabled = true;
+		return;
+	}
+
+	if (!req || (req && (req->cmd_flags & REQ_FLUSH))) {
+		if (mq->num_of_potential_packed_wr_reqs >
+				mq->num_wr_reqs_to_start_packing)
+			mq->wr_packing_enabled = true;
+		return;
+	}
+
+	data_dir = rq_data_dir(req);
+
+	if (data_dir == READ) {
+		mq->num_of_potential_packed_wr_reqs = 0;
+		mq->wr_packing_enabled = false;
+		return;
+	} else if (data_dir == WRITE) {
+		mq->num_of_potential_packed_wr_reqs++;
+	}

drivers/mmc/card/block.c static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq, · line 1371

Quoted 4 lines

drivers/mmc/card/block.c +4 -0

+
+	if (mq->num_of_potential_packed_wr_reqs >
+			mq->num_wr_reqs_to_start_packing)
+		mq->wr_packing_enabled = true;

Write Packing is available only if continuing write requests are over num_wr_reqs_to_start_packing?
That means individual request(1...17) will be issued with non-packing. Could you explain your policy more?

Quoted 12 lines

drivers/mmc/card/block.c +3 -0

+
+}
+
 static u8 mmc_blk_prep_packed_list(struct mmc_queue *mq, struct request

*req)
{

drivers/mmc/card/block.c +0 -0

 	struct request_queue *q = mq->queue;

@@ -1332,6 +1407,9 @@ static u8 mmc_blk_prep_packed_list(struct
 mmc_queue *mq, struct request *req)
 			!card->ext_csd.packed_event_en)
 		goto no_packed;
 

drivers/mmc/card/block.c static u8 mmc_blk_prep_packed_list(struct mmc_queue *mq, struct request *req) · line 1411

Quoted 2 lines

drivers/mmc/card/block.c +2 -0

+	if (!mq->wr_packing_enabled)
+		goto no_packed;

If wr_packing_enabled is set to true, several write requests can be packed.
We don't need to consider read request since packed write?

Thanks,
Seungwon Jeon

Quoted 5 lines

Hi,

How can we check the effect?
Do you have any result?

We ran parallel lmdd read and write operations and found out that the write packing causes the read throughput to drop from 24MB/s to 12MB/s. The write packing control managed to increase the read throughput back to the original value.
We also examined "real life" scenarios, such as performing a big push operation in parallel to launching several applications. We measured the read latency and found out that with the write packing control the worst case of the read latency was smaller.

Quoted 146 lines

Please check the several comment below.

Quoted 143 lines · Maya Erez

The write packing control will ensure that read requests latency is not increased due to long write packed commands.

The trigger for enabling the write packing is managing to pack several write requests. The number of potential packed requests that will trigger
the packing can be configured via sysfs by writing the required value to:
/sys/block/<block_dev_name>/num_wr_reqs_to_start_packing. The trigger for disabling the write packing is fetching a read request.

---
 Documentation/mmc/mmc-dev-attrs.txt |   17 ++++++
 drivers/mmc/card/block.c            |  100
++++++++++++++++++++++++++++++++++-
 drivers/mmc/card/queue.c            |    8 +++
 drivers/mmc/card/queue.h            |    3 +
 include/linux/mmc/host.h            |    1 +
 5 files changed, 128 insertions(+), 1 deletions(-)

Documentation/mmc/mmc-dev-attrs.txt +0 -0

index 22ae844..08f7312 100644
@@ -8,6 +8,23 @@ The following attributes are read/write.
 
 	force_ro		Enforce read-only access even if write protect switch is

off.

Documentation/mmc/mmc-dev-attrs.txt +5 -0

+	num_wr_reqs_to_start_packing 	This attribute is used to determine
+	the trigger for activating the write packing, in case the write
+	packing control feature is enabled.
+
+	When the MMC manages to reach a point where

num_wr_reqs_to_start_packing

Documentation/mmc/mmc-dev-attrs.txt +12 -0

+	write requests could be packed, it enables the write packing feature.
+	This allows us to start the write packing only when it is beneficial
+	and has minimum affect on the read latency.
+
+	The number of potential packed requests that will trigger the packing
+	can be configured via sysfs by writing the required value to:
+	/sys/block/<block_dev_name>/num_wr_reqs_to_start_packing.
+
+	The default value of num_wr_reqs_to_start_packing was determined by
+	running parallel lmdd write and lmdd read operations and calculating
+	the max number of packed writes requests.
+
 SD and MMC Device Attributes
 ============================

drivers/mmc/card/block.c +45 -0

index 2785fd4..ef192fb 100644
@@ -114,6 +114,7 @@ struct mmc_blk_data {
 	struct device_attribute force_ro;
 	struct device_attribute power_ro_lock;
 	int	area_type;
+	struct device_attribute num_wr_reqs_to_start_packing;
 };
 
 static DEFINE_MUTEX(open_lock);

@@ -281,6 +282,38 @@ out:
 	return ret;
 }
 
+static ssize_t
+num_wr_reqs_to_start_packing_show(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct mmc_blk_data *md = mmc_blk_get(dev_to_disk(dev));
+	int num_wr_reqs_to_start_packing;
+	int ret;
+
+	num_wr_reqs_to_start_packing = md->queue.num_wr_reqs_to_start_packing;
+
+	ret = snprintf(buf, PAGE_SIZE, "%d\n", num_wr_reqs_to_start_packing);
+
+	mmc_blk_put(md);
+	return ret;
+}
+
+static ssize_t
+num_wr_reqs_to_start_packing_store(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	int value;
+	struct mmc_blk_data *md = mmc_blk_get(dev_to_disk(dev));
+
+	sscanf(buf, "%d", &value);
+	if (value >= 0)
+		md->queue.num_wr_reqs_to_start_packing = value;
+
+	mmc_blk_put(md);
+	return count;
+}
+
 static int mmc_blk_open(struct block_device *bdev, fmode_t mode)
 {
 	struct mmc_blk_data *md = mmc_blk_get(bdev->bd_disk);

@@ -1313,6 +1346,48 @@ static void mmc_blk_rw_rq_prep(struct
 mmc_queue_req *mqrq,
 	mmc_queue_bounce_pre(mqrq);
 }
 
+static void mmc_blk_write_packing_control(struct mmc_queue *mq,
+					  struct request *req)
+{
+	struct mmc_host *host = mq->card->host;
+	int data_dir;
+
+	if (!(host->caps2 & MMC_CAP2_PACKED_WR))
+		return;
+
+	/*
+	 * In case the packing control is not supported by the host, it should
+	 * not have an effect on the write packing. Therefore we have to

enable

drivers/mmc/card/block.c +23 -0

+	 * the write packing
+	 */
+	if (!(host->caps2 & MMC_CAP2_PACKED_WR_CONTROL)) {
+		mq->wr_packing_enabled = true;
+		return;
+	}
+
+	if (!req || (req && (req->cmd_flags & REQ_FLUSH))) {
+		if (mq->num_of_potential_packed_wr_reqs >
+				mq->num_wr_reqs_to_start_packing)
+			mq->wr_packing_enabled = true;
+		return;
+	}
+
+	data_dir = rq_data_dir(req);
+
+	if (data_dir == READ) {
+		mq->num_of_potential_packed_wr_reqs = 0;
+		mq->wr_packing_enabled = false;
+		return;
+	} else if (data_dir == WRITE) {
+		mq->num_of_potential_packed_wr_reqs++;
+	}

Quoted 8 lines

drivers/mmc/card/block.c static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq, · line 1371

Quoted 4 lines

drivers/mmc/card/block.c +4 -0

+
+	if (mq->num_of_potential_packed_wr_reqs >
+			mq->num_wr_reqs_to_start_packing)
+		mq->wr_packing_enabled = true;

Write Packing is available only if continuing write requests are over num_wr_reqs_to_start_packing?
That means individual request(1...17) will be issued with non-packing. Could you explain your policy more?

We try to identify the case where there is parallel read and write operations. In our experiments we found out that the number of write requests between read requests in parallel read and write operations doesn't exceed 17 requests. Therefore, we can assume that fetching more than 17 write requests without hitting a read request can indicate that there is no read activity.

You are right that this affects the write throughput a bit but the goal of this algorithm is to make sure the read throughput and latency are not decreased due to write. If this is not the desired result, this algorithm can be disabled.

Quoted 12 lines

Quoted 12 lines

drivers/mmc/card/block.c +3 -0

+
+}
+
 static u8 mmc_blk_prep_packed_list(struct mmc_queue *mq, struct request

*req)
{

drivers/mmc/card/block.c +0 -0

 	struct request_queue *q = mq->queue;

@@ -1332,6 +1407,9 @@ static u8 mmc_blk_prep_packed_list(struct
 mmc_queue *mq, struct request *req)
 			!card->ext_csd.packed_event_en)
 		goto no_packed;
 

Quoted 5 lines

drivers/mmc/card/block.c static u8 mmc_blk_prep_packed_list(struct mmc_queue *mq, struct request *req) · line 1411

Quoted 2 lines

drivers/mmc/card/block.c +2 -0

+	if (!mq->wr_packing_enabled)
+		goto no_packed;

If wr_packing_enabled is set to true, several write requests can be packed.
We don't need to consider read request since packed write?

I'm not sure I understand the question. We check if there was a read request in the mmc_blk_write_packing_control, and in such a case set mq->wr_packing_enabled to false.
If I didn't answer the question, please explain it again.

Quoted 4 lines

Thanks,
Seungwon Jeon

Thanks,
Maya Erez
--
Sent by a consultant of the Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 178 lines · Maya Erez

Quoted 5 lines

Hi,

How can we check the effect?
Do you have any result?

We ran parallel lmdd read and write operations and found out that the write packing causes the read throughput to drop from 24MB/s to 12MB/s. The write packing control managed to increase the read throughput back to
the original value.
We also examined "real life" scenarios, such as performing a big push operation in parallel to launching several applications. We measured the read latency and found out that with the write packing control the worst case of the read latency was smaller.

Quoted 7 lines

Please check the several comment below.

Quoted 4 lines · Maya Erez

The write packing control will ensure that read requests latency is not increased due to long write packed commands.

The trigger for enabling the write packing is managing to pack

several

Quoted 6 lines

Quoted 6 lines

write requests. The number of potential packed requests that will trigger
the packing can be configured via sysfs by writing the required value to:
/sys/block/<block_dev_name>/num_wr_reqs_to_start_packing. The trigger for disabling the write packing is fetching a read

request.

Quoted 20 lines

Quoted 20 lines

---
 Documentation/mmc/mmc-dev-attrs.txt |   17 ++++++
 drivers/mmc/card/block.c            |  100
++++++++++++++++++++++++++++++++++-
 drivers/mmc/card/queue.c            |    8 +++
 drivers/mmc/card/queue.h            |    3 +
 include/linux/mmc/host.h            |    1 +
 5 files changed, 128 insertions(+), 1 deletions(-)

Documentation/mmc/mmc-dev-attrs.txt +0 -0

index 22ae844..08f7312 100644
@@ -8,6 +8,23 @@ The following attributes are read/write.
 
 	force_ro		Enforce read-only access even if write protect switch is

off.

Quoted 7 lines

Documentation/mmc/mmc-dev-attrs.txt

Quoted 7 lines

Documentation/mmc/mmc-dev-attrs.txt +5 -0

+	num_wr_reqs_to_start_packing 	This attribute is used to determine
+	the trigger for activating the write packing, in case the write
+	packing control feature is enabled.
+
+	When the MMC manages to reach a point where

num_wr_reqs_to_start_packing

Documentation/mmc/mmc-dev-attrs.txt +1 -0

+	write requests could be packed, it enables the write packing

feature.

Quoted 1 line

Documentation/mmc/mmc-dev-attrs.txt

Quoted 1 line

Documentation/mmc/mmc-dev-attrs.txt +1 -0

+	This allows us to start the write packing only when it is

beneficial

Quoted 3 lines

Documentation/mmc/mmc-dev-attrs.txt The following attributes are read/write.

Quoted 3 lines

Documentation/mmc/mmc-dev-attrs.txt +3 -0

+	and has minimum affect on the read latency.
+
+	The number of potential packed requests that will trigger the

packing

Quoted 5 lines

Documentation/mmc/mmc-dev-attrs.txt The following attributes are read/write.

Quoted 5 lines

Documentation/mmc/mmc-dev-attrs.txt +5 -0

+	can be configured via sysfs by writing the required value to:
+	/sys/block/<block_dev_name>/num_wr_reqs_to_start_packing.
+
+	The default value of num_wr_reqs_to_start_packing was determined by
+	running parallel lmdd write and lmdd read operations and

calculating

Quoted 21 lines

Quoted 21 lines

Documentation/mmc/mmc-dev-attrs.txt +2 -0

+	the max number of packed writes requests.
+
 SD and MMC Device Attributes
 ============================

drivers/mmc/card/block.c +1 -0

index 2785fd4..ef192fb 100644
@@ -114,6 +114,7 @@ struct mmc_blk_data {
 	struct device_attribute force_ro;
 	struct device_attribute power_ro_lock;
 	int	area_type;
+	struct device_attribute num_wr_reqs_to_start_packing;
 };
 
 static DEFINE_MUTEX(open_lock);

@@ -281,6 +282,38 @@ out:
 	return ret;
 }
 

Quoted 9 lines

drivers/mmc/card/block.c out:

Quoted 9 lines

drivers/mmc/card/block.c +9 -0

+static ssize_t
+num_wr_reqs_to_start_packing_show(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct mmc_blk_data *md = mmc_blk_get(dev_to_disk(dev));
+	int num_wr_reqs_to_start_packing;
+	int ret;
+
+	num_wr_reqs_to_start_packing =

md->queue.num_wr_reqs_to_start_packing;

Quoted 2 lines

drivers/mmc/card/block.c out: · line 295

Quoted 2 lines

drivers/mmc/card/block.c +2 -0

+
+	ret = snprintf(buf, PAGE_SIZE, "%d\n",

num_wr_reqs_to_start_packing);

Quoted 29 lines

Quoted 29 lines

drivers/mmc/card/block.c +21 -0

+
+	mmc_blk_put(md);
+	return ret;
+}
+
+static ssize_t
+num_wr_reqs_to_start_packing_store(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	int value;
+	struct mmc_blk_data *md = mmc_blk_get(dev_to_disk(dev));
+
+	sscanf(buf, "%d", &value);
+	if (value >= 0)
+		md->queue.num_wr_reqs_to_start_packing = value;
+
+	mmc_blk_put(md);
+	return count;
+}
+
 static int mmc_blk_open(struct block_device *bdev, fmode_t mode)
 {
 	struct mmc_blk_data *md = mmc_blk_get(bdev->bd_disk);

@@ -1313,6 +1346,48 @@ static void mmc_blk_rw_rq_prep(struct
 mmc_queue_req *mqrq,
 	mmc_queue_bounce_pre(mqrq);
 }
 

Quoted 11 lines

drivers/mmc/card/block.c static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,

Quoted 11 lines

drivers/mmc/card/block.c +11 -0

+static void mmc_blk_write_packing_control(struct mmc_queue *mq,
+					  struct request *req)
+{
+	struct mmc_host *host = mq->card->host;
+	int data_dir;
+
+	if (!(host->caps2 & MMC_CAP2_PACKED_WR))
+		return;
+
+	/*
+	 * In case the packing control is not supported by the host, it

should

Quoted 33 lines

Quoted 19 lines

drivers/mmc/card/block.c +1 -0

+	 * not have an effect on the write packing. Therefore we have to

enable

drivers/mmc/card/block.c +17 -0

+	 * the write packing
+	 */
+	if (!(host->caps2 & MMC_CAP2_PACKED_WR_CONTROL)) {
+		mq->wr_packing_enabled = true;
+		return;
+	}
+
+	if (!req || (req && (req->cmd_flags & REQ_FLUSH))) {
+		if (mq->num_of_potential_packed_wr_reqs >
+				mq->num_wr_reqs_to_start_packing)
+			mq->wr_packing_enabled = true;
+		return;
+	}
+
+	data_dir = rq_data_dir(req);
+
+	if (data_dir == READ) {

drivers/mmc/card/block.c static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq, · line 1387

Quoted 10 lines

drivers/mmc/card/block.c +10 -0

+		mq->num_of_potential_packed_wr_reqs = 0;
+		mq->wr_packing_enabled = false;
+		return;
+	} else if (data_dir == WRITE) {
+		mq->num_of_potential_packed_wr_reqs++;
+	}
+
+	if (mq->num_of_potential_packed_wr_reqs >
+			mq->num_wr_reqs_to_start_packing)
+		mq->wr_packing_enabled = true;

Write Packing is available only if continuing write requests are over num_wr_reqs_to_start_packing?
That means individual request(1...17) will be issued with non-packing. Could you explain your policy more?

Quoted 6 lines

We try to identify the case where there is parallel read and write operations. In our experiments we found out that the number of write requests between read requests in parallel read and write operations doesn't exceed 17 requests. Therefore, we can assume that fetching more than 17 write requests without hitting a read request can indicate that there is no read activity.

We can apply this experiment regardless I/O scheduler? Which I/O scheduler was used with this experiment?

Quoted 25 lines

You are right that this affects the write throughput a bit but the goal of
this algorithm is to make sure the read throughput and latency are not decreased due to write. If this is not the desired result, this algorithm
can be disabled.

Quoted 4 lines

drivers/mmc/card/block.c out:

Quoted 4 lines

drivers/mmc/card/block.c +3 -0

+
+}
+
 static u8 mmc_blk_prep_packed_list(struct mmc_queue *mq, struct

request

Quoted 13 lines

drivers/mmc/card/block.c static u8 mmc_blk_prep_packed_list(struct · line 1412

Quoted 10 lines

*req)
{

drivers/mmc/card/block.c +2 -0

 	struct request_queue *q = mq->queue;

@@ -1332,6 +1407,9 @@ static u8 mmc_blk_prep_packed_list(struct
 mmc_queue *mq, struct request *req)
 			!card->ext_csd.packed_event_en)
 		goto no_packed;
 
+	if (!mq->wr_packing_enabled)
+		goto no_packed;

If wr_packing_enabled is set to true, several write requests can be packed.
We don't need to consider read request since packed write?

Quoted 4 lines

I'm not sure I understand the question. We check if there was a read request in the mmc_blk_write_packing_control, and in such a case set mq->wr_packing_enabled to false.
If I didn't answer the question, please explain it again.

Packed write can be possible after exceeding 17 requests.
Is it assured that read request doesn't follow immediately after packed write?
I wonder this case.

Thanks,
Seungwon Jeon.

Quoted 15 lines

Quoted 4 lines

Thanks,
Seungwon Jeon

Thanks,
Maya Erez
--
Sent by a consultant of the Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 180 lines

Quoted 178 lines · Maya Erez

Quoted 5 lines

Hi,

How can we check the effect?
Do you have any result?

We ran parallel lmdd read and write operations and found out that the write packing causes the read throughput to drop from 24MB/s to 12MB/s. The write packing control managed to increase the read throughput back to
the original value.
We also examined "real life" scenarios, such as performing a big push operation in parallel to launching several applications. We measured the read latency and found out that with the write packing control the worst case of the read latency was smaller.

Quoted 7 lines

Please check the several comment below.

Quoted 4 lines · Maya Erez

The write packing control will ensure that read requests latency is not increased due to long write packed commands.

The trigger for enabling the write packing is managing to pack

several

Quoted 6 lines

Quoted 6 lines

write requests. The number of potential packed requests that will trigger
the packing can be configured via sysfs by writing the required value to:
/sys/block/<block_dev_name>/num_wr_reqs_to_start_packing. The trigger for disabling the write packing is fetching a read

request.

Quoted 20 lines

Quoted 20 lines

---
 Documentation/mmc/mmc-dev-attrs.txt |   17 ++++++ drivers/mmc/card/block.c            |  100
++++++++++++++++++++++++++++++++++-

 drivers/mmc/card/queue.c            |    8 +++
 drivers/mmc/card/queue.h            |    3 +
 include/linux/mmc/host.h            |    1 +
 5 files changed, 128 insertions(+), 1 deletions(-)

Documentation/mmc/mmc-dev-attrs.txt +0 -0

index 22ae844..08f7312 100644
@@ -8,6 +8,23 @@ The following attributes are read/write.
 

  force_ro                Enforce read-only access even if write protect switch is off.

Quoted 7 lines

Quoted 7 lines

+ num_wr_reqs_to_start_packing    This attribute is used to determine
+ the trigger for activating the write packing, in case the write
+ packing control feature is enabled.
+
+ When the MMC manages to reach a point where num_wr_reqs_to_start_packing
+ write requests could be packed, it enables the write packing

feature.

Quoted 1 line

Quoted 1 line

+ This allows us to start the write packing only when it is

beneficial

Quoted 3 lines

Quoted 3 lines

+ and has minimum affect on the read latency.
+
+ The number of potential packed requests that will trigger the

packing

Quoted 5 lines

Quoted 5 lines

+ can be configured via sysfs by writing the required value to:
+ /sys/block/<block_dev_name>/num_wr_reqs_to_start_packing. +
+ The default value of num_wr_reqs_to_start_packing was determined by
+ running parallel lmdd write and lmdd read operations and

calculating

Quoted 21 lines

Quoted 21 lines

+ the max number of packed writes requests.
+
 SD and MMC Device Attributes
 ============================

drivers/mmc/card/block.c +1 -0

index 2785fd4..ef192fb 100644
@@ -114,6 +114,7 @@ struct mmc_blk_data {
   struct device_attribute force_ro;
   struct device_attribute power_ro_lock;
   int     area_type;
+ struct device_attribute num_wr_reqs_to_start_packing;
  };
 
  static DEFINE_MUTEX(open_lock);

@@ -281,6 +282,38 @@ out:
   return ret;
  }
 

Quoted 9 lines

drivers/mmc/card/block.c out:

Quoted 9 lines

drivers/mmc/card/block.c +9 -0

+static ssize_t
+num_wr_reqs_to_start_packing_show(struct device *dev,
+                           struct device_attribute *attr, char *buf)
+{
+ struct mmc_blk_data *md = mmc_blk_get(dev_to_disk(dev));
+ int num_wr_reqs_to_start_packing;
+ int ret;
+
+ num_wr_reqs_to_start_packing =

md->queue.num_wr_reqs_to_start_packing;

Quoted 2 lines

drivers/mmc/card/block.c out: · line 295

Quoted 2 lines

drivers/mmc/card/block.c +2 -0

+
+ ret = snprintf(buf, PAGE_SIZE, "%d\n",

num_wr_reqs_to_start_packing);

Quoted 29 lines

Quoted 29 lines

drivers/mmc/card/block.c +21 -0

+
+ mmc_blk_put(md);
+ return ret;
+}
+
+static ssize_t
+num_wr_reqs_to_start_packing_store(struct device *dev,
+                          struct device_attribute *attr,
+                          const char *buf, size_t count)
+{
+ int value;
+ struct mmc_blk_data *md = mmc_blk_get(dev_to_disk(dev));
+
+ sscanf(buf, "%d", &value);
+ if (value >= 0)
+         md->queue.num_wr_reqs_to_start_packing = value;
+
+ mmc_blk_put(md);
+ return count;
+}
+
  static int mmc_blk_open(struct block_device *bdev, fmode_t mode)
  {
   struct mmc_blk_data *md = mmc_blk_get(bdev->bd_disk);

@@ -1313,6 +1346,48 @@ static void mmc_blk_rw_rq_prep(struct
 mmc_queue_req *mqrq,
   mmc_queue_bounce_pre(mqrq);
  }
 

Quoted 11 lines

drivers/mmc/card/block.c static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,

Quoted 11 lines

drivers/mmc/card/block.c +11 -0

+static void mmc_blk_write_packing_control(struct mmc_queue *mq,
+                                   struct request *req)
+{
+ struct mmc_host *host = mq->card->host;
+ int data_dir;
+
+ if (!(host->caps2 & MMC_CAP2_PACKED_WR))
+         return;
+
+ /*
+  * In case the packing control is not supported by the host, it

should

Quoted 33 lines

Quoted 19 lines

drivers/mmc/card/block.c +1 -0

+  * not have an effect on the write packing. Therefore we have to

enable
+  * the write packing
+  */
+ if (!(host->caps2 & MMC_CAP2_PACKED_WR_CONTROL)) {
+         mq->wr_packing_enabled = true;
+         return;
+ }

drivers/mmc/card/block.c +11 -0

+
+ if (!req || (req && (req->cmd_flags & REQ_FLUSH))) {
+         if (mq->num_of_potential_packed_wr_reqs >
+                         mq->num_wr_reqs_to_start_packing)
+                 mq->wr_packing_enabled = true;
+         return;
+ }
+
+ data_dir = rq_data_dir(req);
+
+ if (data_dir == READ) {

drivers/mmc/card/block.c static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq, · line 1387

Quoted 10 lines

drivers/mmc/card/block.c +10 -0

+         mq->num_of_potential_packed_wr_reqs = 0;
+         mq->wr_packing_enabled = false;
+         return;
+ } else if (data_dir == WRITE) {
+         mq->num_of_potential_packed_wr_reqs++;
+ }
+
+ if (mq->num_of_potential_packed_wr_reqs >
+                 mq->num_wr_reqs_to_start_packing)
+         mq->wr_packing_enabled = true;

Write Packing is available only if continuing write requests are over num_wr_reqs_to_start_packing?
That means individual request(1...17) will be issued with non-packing. Could you explain your policy more?

Quoted 8 lines

Quoted 6 lines

We try to identify the case where there is parallel read and write operations. In our experiments we found out that the number of write requests between read requests in parallel read and write operations doesn't exceed 17 requests. Therefore, we can assume that fetching more than 17 write requests without hitting a read request can indicate that there is no read activity.

We can apply this experiment regardless I/O scheduler? Which I/O scheduler was used with this experiment?

The experiment was performed with the CFQ scheduler. Since the deadline uses a batch of 16 requests it should also fit the deadline scheduler. In case another value is required, this value can be changed via sysfs.

Quoted 25 lines

Quoted 25 lines

You are right that this affects the write throughput a bit but the goal of
this algorithm is to make sure the read throughput and latency are not decreased due to write. If this is not the desired result, this algorithm
can be disabled.

Quoted 4 lines

Quoted 4 lines

drivers/mmc/card/block.c +3 -0

+
+}
+

 static u8 mmc_blk_prep_packed_list(struct mmc_queue *mq, struct

request

Quoted 13 lines

drivers/mmc/card/block.c static u8 mmc_blk_prep_packed_list(struct · line 1412

Quoted 10 lines

*req)
 {
  struct request_queue *q = mq->queue;

drivers/mmc/card/block.c +2 -0

@@ -1332,6 +1407,9 @@ static u8 mmc_blk_prep_packed_list(struct
 mmc_queue *mq, struct request *req)
                   !card->ext_csd.packed_event_en)
           goto no_packed;
 
+ if (!mq->wr_packing_enabled)
+         goto no_packed;

If wr_packing_enabled is set to true, several write requests can be packed.
We don't need to consider read request since packed write?

Quoted 8 lines

Quoted 4 lines

I'm not sure I understand the question. We check if there was a read request in the mmc_blk_write_packing_control, and in such a case set mq->wr_packing_enabled to false.
If I didn't answer the question, please explain it again.

Packed write can be possible after exceeding 17 requests.
Is it assured that read request doesn't follow immediately after packed write?
I wonder this case.

Currently in such a case we will send the packed command followed by the read request. The latency of this read request will be high due to waiting for the completion of the packed write. However, since we will disable the write packing, the latency of the following read requests will be low. We are working on a solution where the read request will bypass the write requests in such a case. This change requires modification of the scheduler in order to re-insert the write requests to the scheduler.

Quoted 1 line

Quoted 237 lines

Quoted 198 lines

Quoted 188 lines · Maya Erez

Quoted 5 lines

Hi,

How can we check the effect?
Do you have any result?

We ran parallel lmdd read and write operations and found out that the write packing causes the read throughput to drop from 24MB/s to 12MB/s.
The write packing control managed to increase the read throughput back to
the original value.
We also examined "real life" scenarios, such as performing a big push operation in parallel to launching several applications. We measured the
read latency and found out that with the write packing control the worst
case of the read latency was smaller.

Quoted 7 lines

Please check the several comment below.

Quoted 4 lines · Maya Erez

The write packing control will ensure that read requests latency is not increased due to long write packed commands.

The trigger for enabling the write packing is managing to pack

several

Quoted 3 lines

Quoted 3 lines

write requests. The number of potential packed requests that will trigger
the packing can be configured via sysfs by writing the required

value

Quoted 3 lines

Quoted 3 lines

to:
/sys/block/<block_dev_name>/num_wr_reqs_to_start_packing. The trigger for disabling the write packing is fetching a read

request.

Quoted 10 lines

Quoted 10 lines

---
 Documentation/mmc/mmc-dev-attrs.txt |   17 ++++++ drivers/mmc/card/block.c            |  100
++++++++++++++++++++++++++++++++++-

 drivers/mmc/card/queue.c            |    8 +++
 drivers/mmc/card/queue.h            |    3 +
 include/linux/mmc/host.h            |    1 +
 5 files changed, 128 insertions(+), 1 deletions(-)

Quoted 8 lines

Quoted 8 lines

Documentation/mmc/mmc-dev-attrs.txt +0 -0

index 22ae844..08f7312 100644
@@ -8,6 +8,23 @@ The following attributes are read/write.
 

  force_ro                Enforce read-only access even if write

protect switch is

Quoted 3 lines

Quoted 3 lines

off.

+ num_wr_reqs_to_start_packing    This attribute is used to

determine

Quoted 6 lines

Quoted 6 lines

+ the trigger for activating the write packing, in case the write
+ packing control feature is enabled.
+
+ When the MMC manages to reach a point where num_wr_reqs_to_start_packing
+ write requests could be packed, it enables the write packing

feature.

Quoted 1 line

Quoted 1 line

+ This allows us to start the write packing only when it is

beneficial

Quoted 3 lines

Quoted 3 lines

+ and has minimum affect on the read latency.
+
+ The number of potential packed requests that will trigger the

packing

Quoted 4 lines

Quoted 4 lines

+ can be configured via sysfs by writing the required value to:
+ /sys/block/<block_dev_name>/num_wr_reqs_to_start_packing. +
+ The default value of num_wr_reqs_to_start_packing was determined

by

Quoted 1 line

Quoted 1 line

+ running parallel lmdd write and lmdd read operations and

calculating

Quoted 16 lines

Quoted 16 lines

+ the max number of packed writes requests.
+
 SD and MMC Device Attributes
 ============================

drivers/mmc/card/block.c +1 -0

index 2785fd4..ef192fb 100644
@@ -114,6 +114,7 @@ struct mmc_blk_data {
   struct device_attribute force_ro;
   struct device_attribute power_ro_lock;
   int     area_type;
+ struct device_attribute num_wr_reqs_to_start_packing;
  };
 

Quoted 8 lines

drivers/mmc/card/block.c out: · line 287

Quoted 8 lines

drivers/mmc/card/block.c +3 -0

  static DEFINE_MUTEX(open_lock);

@@ -281,6 +282,38 @@ out:
   return ret;
  }
 
+static ssize_t
+num_wr_reqs_to_start_packing_show(struct device *dev,
+                           struct device_attribute *attr, char

*buf)

Quoted 6 lines

drivers/mmc/card/block.c out:

Quoted 6 lines

drivers/mmc/card/block.c +6 -0

+{
+ struct mmc_blk_data *md = mmc_blk_get(dev_to_disk(dev));
+ int num_wr_reqs_to_start_packing;
+ int ret;
+
+ num_wr_reqs_to_start_packing =

md->queue.num_wr_reqs_to_start_packing;

Quoted 2 lines

drivers/mmc/card/block.c out: · line 295

Quoted 2 lines

drivers/mmc/card/block.c +2 -0

+
+ ret = snprintf(buf, PAGE_SIZE, "%d\n",

num_wr_reqs_to_start_packing);

Quoted 29 lines

Quoted 29 lines

drivers/mmc/card/block.c +21 -0

+
+ mmc_blk_put(md);
+ return ret;
+}
+
+static ssize_t
+num_wr_reqs_to_start_packing_store(struct device *dev,
+                          struct device_attribute *attr,
+                          const char *buf, size_t count)
+{
+ int value;
+ struct mmc_blk_data *md = mmc_blk_get(dev_to_disk(dev));
+
+ sscanf(buf, "%d", &value);
+ if (value >= 0)
+         md->queue.num_wr_reqs_to_start_packing = value;
+
+ mmc_blk_put(md);
+ return count;
+}
+
  static int mmc_blk_open(struct block_device *bdev, fmode_t mode)
  {
   struct mmc_blk_data *md = mmc_blk_get(bdev->bd_disk);

@@ -1313,6 +1346,48 @@ static void mmc_blk_rw_rq_prep(struct
 mmc_queue_req *mqrq,
   mmc_queue_bounce_pre(mqrq);
  }
 

Quoted 11 lines

drivers/mmc/card/block.c static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,

Quoted 11 lines

drivers/mmc/card/block.c +11 -0

+static void mmc_blk_write_packing_control(struct mmc_queue *mq,
+                                   struct request *req)
+{
+ struct mmc_host *host = mq->card->host;
+ int data_dir;
+
+ if (!(host->caps2 & MMC_CAP2_PACKED_WR))
+         return;
+
+ /*
+  * In case the packing control is not supported by the host, it

should

Quoted 22 lines

Quoted 22 lines

drivers/mmc/card/block.c +1 -0

+  * not have an effect on the write packing. Therefore we have to

enable
+  * the write packing
+  */
+ if (!(host->caps2 & MMC_CAP2_PACKED_WR_CONTROL)) {
+         mq->wr_packing_enabled = true;
+         return;
+ }

drivers/mmc/card/block.c +14 -0

+
+ if (!req || (req && (req->cmd_flags & REQ_FLUSH))) {
+         if (mq->num_of_potential_packed_wr_reqs >
+                         mq->num_wr_reqs_to_start_packing)
+                 mq->wr_packing_enabled = true;
+         return;
+ }
+
+ data_dir = rq_data_dir(req);
+
+ if (data_dir == READ) {
+         mq->num_of_potential_packed_wr_reqs = 0;
+         mq->wr_packing_enabled = false;
+         return;

Quoted 8 lines

Quoted 7 lines

+ } else if (data_dir == WRITE) {
+         mq->num_of_potential_packed_wr_reqs++;
+ }
+
+ if (mq->num_of_potential_packed_wr_reqs >
+                 mq->num_wr_reqs_to_start_packing)
+         mq->wr_packing_enabled = true;

Write Packing is available only if continuing write requests are

over

Quoted 2 lines

num_wr_reqs_to_start_packing?
That means individual request(1...17) will be issued with

non-packing.

Quoted 1 line

Could you explain your policy more?

Quoted 8 lines

We try to identify the case where there is parallel read and write operations. In our experiments we found out that the number of write requests between read requests in parallel read and write operations doesn't exceed 17 requests. Therefore, we can assume that fetching more
than 17 write requests without hitting a read request can indicate that
there is no read activity.

Quoted 2 lines

We can apply this experiment regardless I/O scheduler? Which I/O scheduler was used with this experiment?

The experiment was performed with the CFQ scheduler. Since the deadline uses a batch of 16 requests it should also fit the deadline scheduler. In case another value is required, this value can be changed via sysfs.

Quoted 26 lines

Quoted 26 lines

You are right that this affects the write throughput a bit but the goal
of
this algorithm is to make sure the read throughput and latency are not decreased due to write. If this is not the desired result, this algorithm
can be disabled.

Quoted 4 lines

Quoted 4 lines

drivers/mmc/card/block.c +3 -0

+
+}
+

 static u8 mmc_blk_prep_packed_list(struct mmc_queue *mq, struct

request

Quoted 13 lines

drivers/mmc/card/block.c static u8 mmc_blk_prep_packed_list(struct · line 1412

Quoted 10 lines

*req)
 {
  struct request_queue *q = mq->queue;

drivers/mmc/card/block.c +2 -0

@@ -1332,6 +1407,9 @@ static u8 mmc_blk_prep_packed_list(struct
 mmc_queue *mq, struct request *req)
                   !card->ext_csd.packed_event_en)
           goto no_packed;
 
+ if (!mq->wr_packing_enabled)
+         goto no_packed;

If wr_packing_enabled is set to true, several write requests can be packed.
We don't need to consider read request since packed write?

Quoted 8 lines

Quoted 4 lines

I'm not sure I understand the question. We check if there was a read request in the mmc_blk_write_packing_control, and in such a case set mq->wr_packing_enabled to false.
If I didn't answer the question, please explain it again.

Packed write can be possible after exceeding 17 requests.
Is it assured that read request doesn't follow immediately after packed write?
I wonder this case.

Quoted 11 lines

Currently in such a case we will send the packed command followed by the read request. The latency of this read request will be high due to waiting
for the completion of the packed write. However, since we will disable the
write packing, the latency of the following read requests will be low. We are working on a solution where the read request will bypass the write
requests in such a case. This change requires modification of the scheduler in order to re-insert the write requests to the scheduler.

Quoted 1 line

Thats the precise reason for using foreground HPI (shameless plug :-)) I understand the intent of write packing control, but using the number of requests
as a metric is too coarse. Some writes could be for only one sector (512B) and others
could be in 512KB or more, giving a 1000x variance.

Foreground HPI solves this problem by interrupting only on a wait threshold.

Another aspect is that if a packed write is in progress, and you have a read request,
you will most likely disable packing for the _next_ write, not the ongoing one, right ?
That's too late an intervention IMHO.

Quoted 258 lines · On Mon, Jun 11, 2012 at 7:25 PM,  <[email protected]> wrote:

Quoted 237 lines

Quoted 198 lines

Quoted 188 lines · Maya Erez

Quoted 5 lines

Hi,

How can we check the effect?
Do you have any result?

We ran parallel lmdd read and write operations and found out that the write packing causes the read throughput to drop from 24MB/s to 12MB/s.
The write packing control managed to increase the read throughput back to
the original value.
We also examined "real life" scenarios, such as performing a big push operation in parallel to launching several applications. We measured the
read latency and found out that with the write packing control the worst
case of the read latency was smaller.

Quoted 7 lines

Please check the several comment below.

Quoted 4 lines · Maya Erez

The write packing control will ensure that read requests latency is not increased due to long write packed commands.

The trigger for enabling the write packing is managing to pack

several

Quoted 3 lines

Quoted 3 lines

write requests. The number of potential packed requests that will trigger
the packing can be configured via sysfs by writing the required

value

Quoted 3 lines

Quoted 3 lines

to:
/sys/block/<block_dev_name>/num_wr_reqs_to_start_packing. The trigger for disabling the write packing is fetching a read

request.

Quoted 10 lines

Quoted 10 lines

---
 Documentation/mmc/mmc-dev-attrs.txt |   17 ++++++ drivers/mmc/card/block.c            |  100
++++++++++++++++++++++++++++++++++-

 drivers/mmc/card/queue.c            |    8 +++
 drivers/mmc/card/queue.h            |    3 +
 include/linux/mmc/host.h            |    1 +
 5 files changed, 128 insertions(+), 1 deletions(-)

Quoted 8 lines

Quoted 8 lines

Documentation/mmc/mmc-dev-attrs.txt +0 -0

index 22ae844..08f7312 100644
@@ -8,6 +8,23 @@ The following attributes are read/write.
 

  force_ro                Enforce read-only access even if write

protect switch is

Quoted 3 lines

Quoted 3 lines

off.

+ num_wr_reqs_to_start_packing    This attribute is used to

determine

Quoted 6 lines

Quoted 6 lines

+ the trigger for activating the write packing, in case the write
+ packing control feature is enabled.
+
+ When the MMC manages to reach a point where num_wr_reqs_to_start_packing
+ write requests could be packed, it enables the write packing

feature.

Quoted 1 line

Quoted 1 line

+ This allows us to start the write packing only when it is

beneficial

Quoted 3 lines

Quoted 3 lines

+ and has minimum affect on the read latency.
+
+ The number of potential packed requests that will trigger the

packing

Quoted 4 lines

Quoted 4 lines

+ can be configured via sysfs by writing the required value to:
+ /sys/block/<block_dev_name>/num_wr_reqs_to_start_packing. +
+ The default value of num_wr_reqs_to_start_packing was determined

by

Quoted 1 line

Quoted 1 line

+ running parallel lmdd write and lmdd read operations and

calculating

Quoted 16 lines

Quoted 16 lines

+ the max number of packed writes requests.
+
 SD and MMC Device Attributes
 ============================

drivers/mmc/card/block.c +1 -0

index 2785fd4..ef192fb 100644
@@ -114,6 +114,7 @@ struct mmc_blk_data {
   struct device_attribute force_ro;
   struct device_attribute power_ro_lock;
   int     area_type;
+ struct device_attribute num_wr_reqs_to_start_packing;
  };
 

Quoted 8 lines

drivers/mmc/card/block.c out: · line 287

Quoted 8 lines

drivers/mmc/card/block.c +3 -0

  static DEFINE_MUTEX(open_lock);

@@ -281,6 +282,38 @@ out:
   return ret;
  }
 
+static ssize_t
+num_wr_reqs_to_start_packing_show(struct device *dev,
+                           struct device_attribute *attr, char

*buf)

Quoted 6 lines

drivers/mmc/card/block.c out: · line 293

Quoted 6 lines

drivers/mmc/card/block.c +6 -0

+{
+ struct mmc_blk_data *md = mmc_blk_get(dev_to_disk(dev));
+ int num_wr_reqs_to_start_packing;
+ int ret;
+
+ num_wr_reqs_to_start_packing =

md->queue.num_wr_reqs_to_start_packing;

Quoted 2 lines

drivers/mmc/card/block.c out: · line 295

Quoted 2 lines

drivers/mmc/card/block.c +2 -0

+
+ ret = snprintf(buf, PAGE_SIZE, "%d\n",

num_wr_reqs_to_start_packing);

Quoted 29 lines

Quoted 29 lines

drivers/mmc/card/block.c +21 -0

+
+ mmc_blk_put(md);
+ return ret;
+}
+
+static ssize_t
+num_wr_reqs_to_start_packing_store(struct device *dev,
+                          struct device_attribute *attr,
+                          const char *buf, size_t count)
+{
+ int value;
+ struct mmc_blk_data *md = mmc_blk_get(dev_to_disk(dev));
+
+ sscanf(buf, "%d", &value);
+ if (value >= 0)
+         md->queue.num_wr_reqs_to_start_packing = value;
+
+ mmc_blk_put(md);
+ return count;
+}
+
  static int mmc_blk_open(struct block_device *bdev, fmode_t mode)
  {
   struct mmc_blk_data *md = mmc_blk_get(bdev->bd_disk);

@@ -1313,6 +1346,48 @@ static void mmc_blk_rw_rq_prep(struct
 mmc_queue_req *mqrq,
   mmc_queue_bounce_pre(mqrq);
  }
 

Quoted 11 lines

drivers/mmc/card/block.c static void mmc_blk_rw_rq_prep(struct · line 1360

Quoted 11 lines

drivers/mmc/card/block.c +11 -0

+static void mmc_blk_write_packing_control(struct mmc_queue *mq,
+                                   struct request *req)
+{
+ struct mmc_host *host = mq->card->host;
+ int data_dir;
+
+ if (!(host->caps2 & MMC_CAP2_PACKED_WR))
+         return;
+
+ /*
+  * In case the packing control is not supported by the host, it

should

Quoted 22 lines

Quoted 22 lines

drivers/mmc/card/block.c +1 -0

+  * not have an effect on the write packing. Therefore we have to

enable
+  * the write packing
+  */
+ if (!(host->caps2 & MMC_CAP2_PACKED_WR_CONTROL)) {
+         mq->wr_packing_enabled = true;
+         return;
+ }

drivers/mmc/card/block.c +14 -0

+
+ if (!req || (req && (req->cmd_flags & REQ_FLUSH))) {
+         if (mq->num_of_potential_packed_wr_reqs >
+                         mq->num_wr_reqs_to_start_packing)
+                 mq->wr_packing_enabled = true;
+         return;
+ }
+
+ data_dir = rq_data_dir(req);
+
+ if (data_dir == READ) {
+         mq->num_of_potential_packed_wr_reqs = 0;
+         mq->wr_packing_enabled = false;
+         return;

Quoted 8 lines

Quoted 7 lines

+ } else if (data_dir == WRITE) {
+         mq->num_of_potential_packed_wr_reqs++;
+ }
+
+ if (mq->num_of_potential_packed_wr_reqs >
+                 mq->num_wr_reqs_to_start_packing)
+         mq->wr_packing_enabled = true;

Write Packing is available only if continuing write requests are

over

Quoted 2 lines

num_wr_reqs_to_start_packing?
That means individual request(1...17) will be issued with

non-packing.

Quoted 1 line

Could you explain your policy more?

Quoted 8 lines

We try to identify the case where there is parallel read and write operations. In our experiments we found out that the number of write requests between read requests in parallel read and write operations doesn't exceed 17 requests. Therefore, we can assume that fetching more
than 17 write requests without hitting a read request can indicate that
there is no read activity.

Quoted 2 lines

We can apply this experiment regardless I/O scheduler? Which I/O scheduler was used with this experiment?

The experiment was performed with the CFQ scheduler. Since the deadline uses a batch of 16 requests it should also fit the deadline scheduler. In case another value is required, this value can be changed via sysfs.

Quoted 26 lines

Quoted 26 lines

You are right that this affects the write throughput a bit but the goal
of
this algorithm is to make sure the read throughput and latency are not decreased due to write. If this is not the desired result, this algorithm
can be disabled.

Quoted 4 lines

Quoted 4 lines

drivers/mmc/card/block.c +3 -0

+
+}
+

 static u8 mmc_blk_prep_packed_list(struct mmc_queue *mq, struct

request

Quoted 13 lines

drivers/mmc/card/block.c static u8 mmc_blk_prep_packed_list(struct · line 1412

Quoted 10 lines

*req)
 {
  struct request_queue *q = mq->queue;

drivers/mmc/card/block.c +2 -0

@@ -1332,6 +1407,9 @@ static u8 mmc_blk_prep_packed_list(struct
 mmc_queue *mq, struct request *req)
                   !card->ext_csd.packed_event_en)
           goto no_packed;
 
+ if (!mq->wr_packing_enabled)
+         goto no_packed;

If wr_packing_enabled is set to true, several write requests can be packed.
We don't need to consider read request since packed write?

Quoted 8 lines

Quoted 4 lines

I'm not sure I understand the question. We check if there was a read request in the mmc_blk_write_packing_control, and in such a case set mq->wr_packing_enabled to false.
If I didn't answer the question, please explain it again.

Packed write can be possible after exceeding 17 requests.
Is it assured that read request doesn't follow immediately after packed write?
I wonder this case.

Quoted 11 lines

Currently in such a case we will send the packed command followed by the read request. The latency of this read request will be high due to waiting
for the completion of the packed write. However, since we will disable the
write packing, the latency of the following read requests will be low. We are working on a solution where the read request will bypass the write
requests in such a case. This change requires modification of the scheduler in order to re-insert the write requests to the scheduler.

Quoted 1 line

Thats the precise reason for using foreground HPI (shameless plug :-)) I understand the intent of write packing control, but using the number of requests
as a metric is too coarse. Some writes could be for only one sector (512B) and others
could be in 512KB or more, giving a 1000x variance.

Quoted 9 lines

Foreground HPI solves this problem by interrupting only on a wait threshold.

Another aspect is that if a packed write is in progress, and you have a read request,
you will most likely disable packing for the _next_ write, not the ongoing one, right ?
That's too late an intervention IMHO.

If a write request is in progress and a read is fetched we pln to use HPI to stop it and re-insert the remider of the write packed command back to the scheduler for a later dispatch.

Regarding the packing control trigger, we also tried using a trigger of an amount of write bytes between read. However, the number of potential packed requests seemed like the reasonable trigger since we would like to activate the packing only when it will be beneficial, regardless of the write requests sizes.

Quoted 5 lines

Hi,

How can we check the effect?
Do you have any result?

We ran parallel lmdd read and write operations and found out that the write packing causes the read throughput to drop from 24MB/s to 12MB/s.

The write packing control managed to increase the read throughput back to the original value.
We also examined "real life" scenarios, such as performing a big push operation in parallel to launching several applications. We measured the read latency and found out that with the write packing control the worst case of the read latency was smaller.

Quoted 14 lines

Please check the several comment below.

Quoted 11 lines · Maya Erez

The write packing control will ensure that read requests latency is not increased due to long write packed commands.

The trigger for enabling the write packing is managing to pack several write requests. The number of potential packed requests that will trigger
the packing can be configured via sysfs by writing the required value to:
/sys/block/<block_dev_name>/num_wr_reqs_to_start_packing. The trigger for disabling the write packing is fetching a read request.

Quoted 7 lines

Quoted 5 lines

Hi,

How can we check the effect?
Do you have any result?

We ran parallel lmdd read and write operations and found out that the write packing causes the read throughput to drop from 24MB/s to 12MB/s.

Whoa! That's a big drop.
BTW, is there a problem with throughput or latency, or both ? If these numbers are over long duration (>5 seconds), then where are the cycles going?
It would be nice to see some blktrace figures for the issue, and then fix it,
rather than apply a band aid like the write-packing-control on top..

Quoted 16 lines

The write packing control managed to increase the read throughput back to
the original value.
We also examined "real life" scenarios, such as performing a big push operation in parallel to launching several applications. We measured the read latency and found out that with the write packing control the worst case of the read latency was smaller.

Quoted 6 lines

Please check the several comment below.

Quoted 3 lines · Maya Erez

The write packing control will ensure that read requests latency is not increased due to long write packed commands.

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

The trigger for enabling the write packing is managing to pack several write requests. The number of potential packed requests that will trigger
the packing can be configured via sysfs by writing the required value to:
/sys/block/<block_dev_name>/num_wr_reqs_to_start_packing. The trigger for disabling the write packing is fetching a read request.

Quoted 10 lines · On Sat, Jun 9, 2012 at 8:16 PM,  <[email protected]> wrote:

Quoted 7 lines

Quoted 5 lines

Hi,

How can we check the effect?
Do you have any result?

We ran parallel lmdd read and write operations and found out that the write packing causes the read throughput to drop from 24MB/s to 12MB/s.

Quoted 7 lines

Whoa! That's a big drop.
BTW, is there a problem with throughput or latency, or both ? If these numbers are over long duration (>5 seconds), then where are the cycles going?
It would be nice to see some blktrace figures for the issue, and then fix it,
rather than apply a band aid like the write-packing-control on top..

I believe this is because the write packing changes the dispatching policy of the scheduler. Without write packing only 2 write requests were fetched, giving the read requests a chance to be inserted into the scheduler while we wait for the completion of the first write request.

Then when the next fetch was performed the read request would be the chosen one. When write packing is enabled we keep fetching all the write requests that are queued (assuming there are no read requests inserted yet) and when the read is inserted and fetched is has to wait for the completion of a bigger amount of write requests.

Quoted 16 lines

Quoted 16 lines

The write packing control managed to increase the read throughput back to
the original value.
We also examined "real life" scenarios, such as performing a big push operation in parallel to launching several applications. We measured the read latency and found out that with the write packing control the worst case of the read latency was smaller.

Quoted 6 lines

Please check the several comment below.

Quoted 3 lines · Maya Erez

The write packing control will ensure that read requests latency is not increased due to long write packed commands.

Quoted 10 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

The trigger for enabling the write packing is managing to pack several write requests. The number of potential packed requests that will trigger
the packing can be configured via sysfs by writing the required value to:
/sys/block/<block_dev_name>/num_wr_reqs_to_start_packing. The trigger for disabling the write packing is fetching a read request.

Thanks,
Maya Erez
Consultant for Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

The write packing control will ensure that read requests latency is not increased due to long write packed commands.

The trigger for enabling the write packing is managing to pack several write requests. The number of potential packed requests that will trigger the packing can be configured via sysfs by writing the required value to:
/sys/block/<block_dev_name>/num_wr_reqs_to_start_packing.
The trigger for disabling the write packing is fetching a read request.

Quoted 9 lines · Maya Erez

The write packing control will ensure that read requests latency is not increased due to long write packed commands.

The trigger for enabling the write packing is managing to pack several write requests. The number of potential packed requests that will trigger the packing can be configured via sysfs by writing the required value to:
/sys/block/<block_dev_name>/num_wr_reqs_to_start_packing.
The trigger for disabling the write packing is fetching a read request.

If it is applicable only to MMC why do we have this sysfs attr for all block devices?

Quoted 13 lines · Muthu Kumar

Quoted 11 lines · Maya Erez

The write packing control will ensure that read requests latency is not increased due to long write packed commands.

The trigger for enabling the write packing is managing to pack several write requests. The number of potential packed requests that will trigger
the packing can be configured via sysfs by writing the required value to:
/sys/block/<block_dev_name>/num_wr_reqs_to_start_packing. The trigger for disabling the write packing is fetching a read request.

Quoted 2 lines

If it is applicable only to MMC why do we have this sysfs attr for all block devices?

Just to be clear, please create a directory, say mmc, under /sys/block/<dev>/ and create the attr inside that.

You can refer to dm (dm-sysfs.c) for sample implementation.

Regards,
Muthu

Quoted 13 lines · Muthu Kumar

Quoted 11 lines · Maya Erez

The write packing control will ensure that read requests latency is not increased due to long write packed commands.

The trigger for enabling the write packing is managing to pack several write requests. The number of potential packed requests that will trigger
the packing can be configured via sysfs by writing the required value to:
/sys/block/<block_dev_name>/num_wr_reqs_to_start_packing. The trigger for disabling the write packing is fetching a read request.

Quoted 2 lines

If it is applicable only to MMC why do we have this sysfs attr for all block devices?

Just to be clear, please create a directory, say mmc, under /sys/block/<dev>/ and create the attr inside that.

You can refer to dm (dm-sysfs.c) for sample implementation.

Regards,
Muthu

Quoted 12 lines · Muthu Kumar

Quoted 10 lines · Muthu Kumar

Quoted 6 lines · Maya Erez

trigger
the packing can be configured via sysfs by writing the required value to:
/sys/block/<block_dev_name>/num_wr_reqs_to_start_packing. The trigger for disabling the write packing is fetching a read request.

If it is applicable only to MMC why do we have this sysfs attr for all block devices?

Quoted 4 lines

Just to be clear, please create a directory, say mmc, under /sys/block/<dev>/ and create the attr inside that.

You can refer to dm (dm-sysfs.c) for sample implementation.

I understand why you think it would be best to distinguish the MMC specific attribute from the general block devices attributes. However, since this attribute is created only for the MMC block device, other block devices won't be aware of it.

Therefore, it doesn't
necessarily require a separation to a different folder. Currently there is another MMC specific attribute (force_ro) which is also created in the root directory. I think it would be better to also create the num_wr_reqs_to_start_packing in the same folder as force_ro and not make it an exceptional attribute in its location and the code that handles it.

I would appreciate your opinion on that.

Thanks,
Maya
--
Sent by consultant of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Quoted 17 lines · On Wed, Jun 13, 2012 at 12:52 PM, <[email protected]> wrote:

Quoted 15 lines · Muthu Kumar

Quoted 12 lines · Muthu Kumar

Quoted 7 lines · Maya Erez

trigger
the packing can be configured via sysfs by writing the required value to:
/sys/block/<block_dev_name>/num_wr_reqs_to_start_packing. The trigger for disabling the write packing is fetching a read request.

If it is applicable only to MMC why do we have this sysfs attr for all block devices?

Quoted 8 lines

Quoted 4 lines

Just to be clear, please create a directory, say mmc, under /sys/block/<dev>/ and create the attr inside that.

You can refer to dm (dm-sysfs.c) for sample implementation.

I understand why you think it would be best to distinguish the MMC specific attribute from the general block devices attributes. However, since this attribute is created only for the MMC block device, other block devices won't be aware of it.

I understand its created by the MMC code so will not be there for other block devices. But having the device specific attributes inside one <device> directory is better/cleaner. And since we are already following that model for other devices, why not follow that for MMC also?

Quoted 9 lines

Therefore, it doesn't
necessarily require a separation to a different folder. Currently there is another MMC specific attribute (force_ro) which is also
created in the root directory. I think it would be better to also create the num_wr_reqs_to_start_packing in the same folder as force_ro and not make it an exceptional attribute in its location and the code that handles
it.

Then time to move that as well to "mmc" directory.

Regards,
Muthu

Quoted 10 lines

I would appreciate your opinion on that.

Thanks,
Maya
--
Sent by consultant of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Quoted 14 lines · On Wed, Jun 13, 2012 at 12:52 PM, <[email protected]> wrote:

Quoted 12 lines · Muthu Kumar

Quoted 10 lines · Muthu Kumar

Quoted 6 lines · Maya Erez

trigger
the packing can be configured via sysfs by writing the required value to:
/sys/block/<block_dev_name>/num_wr_reqs_to_start_packing. The trigger for disabling the write packing is fetching a read request.

If it is applicable only to MMC why do we have this sysfs attr for all block devices?

Quoted 8 lines

Quoted 4 lines

Just to be clear, please create a directory, say mmc, under /sys/block/<dev>/ and create the attr inside that.

You can refer to dm (dm-sysfs.c) for sample implementation.

I understand why you think it would be best to distinguish the MMC specific attribute from the general block devices attributes. However, since this attribute is created only for the MMC block device, other block devices won't be aware of it.

I understand its created by the MMC code so will not be there for other block devices. But having the device specific attributes inside one <device> directory is better/cleaner. And since we are already following that model for other devices, why not follow that for MMC also?

Hi,

Quoted 24 lines · Muthu Kumar

Quoted 14 lines · On Wed, Jun 13, 2012 at 12:52 PM, <[email protected]> wrote:

Quoted 12 lines · Muthu Kumar

Quoted 10 lines · Muthu Kumar

Quoted 6 lines · Maya Erez

trigger
the packing can be configured via sysfs by writing the required value to:
/sys/block/<block_dev_name>/num_wr_reqs_to_start_packing. The trigger for disabling the write packing is fetching a read request.

If it is applicable only to MMC why do we have this sysfs attr for all block devices?

Quoted 8 lines

Quoted 4 lines

Just to be clear, please create a directory, say mmc, under /sys/block/<dev>/ and create the attr inside that.

You can refer to dm (dm-sysfs.c) for sample implementation.

I understand why you think it would be best to distinguish the MMC specific attribute from the general block devices attributes. However, since this attribute is created only for the MMC block device, other block devices won't be aware of it.

Quoted 5 lines

I understand its created by the MMC code so will not be there for other block devices. But having the device specific attributes inside one <device> directory is better/cleaner. And since we are already following that model for other devices, why not follow that for MMC also?

I've already replied to a later version of the patch, but just to get this comment in at the appropriate point of the discussion as well:

Even though it would result in a cleaner sysfs, I don't want to do this now because it will break userspace scripts that are depending on the current locations of these attributes.

Thanks,

- Chris.
--
Chris Ball <[email protected]> <http://printf.net/>
One Laptop Per Child

Quoted 6 lines

I've already replied to a later version of the patch, but just to get this comment in at the appropriate point of the discussion as well:

Even though it would result in a cleaner sysfs, I don't want to do this now because it will break userspace scripts that are depending on the current locations of these attributes.

Maya is adding a new sysfs attribute with that patch. So, there should not be any user space stuff that depends on it.

Hi,

Quoted 9 lines · Muthu Kumar

Quoted 6 lines

I've already replied to a later version of the patch, but just to get this comment in at the appropriate point of the discussion as well:

Even though it would result in a cleaner sysfs, I don't want to do this now because it will break userspace scripts that are depending on the current locations of these attributes.

Maya is adding a new sysfs attribute with that patch. So, there should not be any user space stuff that depends on it.

In the later patchset, Maya's "[PATCH v4 1/2] mmc: card: Move MMC specific attributes to mmc sub-directory" moves the existing attributes into the mmc/ directory.

It's that move that I'm objecting to, rather than the creation of a new directory -- although since we're going to leave the current attributes where they are, it might not make sense to add the new directory.

We'd be creating two places that people have to look for mmc-related attributes, which is arguably less clean than having one place to look even though it's mixed in with the other block device attributes.

Thanks,

- Chris.
--
Chris Ball <[email protected]> <http://printf.net/>
One Laptop Per Child

Quoted 21 lines · Chris Ball

Hi,

Quoted 9 lines · Muthu Kumar

Quoted 6 lines

I've already replied to a later version of the patch, but just to get this comment in at the appropriate point of the discussion as well:

Even though it would result in a cleaner sysfs, I don't want to do this now because it will break userspace scripts that are depending on the current locations of these attributes.

Maya is adding a new sysfs attribute with that patch. So, there should not be any user space stuff that depends on it.

In the later patchset, Maya's "[PATCH v4 1/2] mmc: card: Move MMC specific attributes to mmc sub-directory" moves the existing attributes into the mmc/ directory.

It's that move that I'm objecting to, rather than the creation of a new directory -- although since we're going to leave the current attributes where they are, it might not make sense to add the new directory.

Quoted 3 lines

We'd be creating two places that people have to look for mmc-related attributes, which is arguably less clean than having one place to look even though it's mixed in with the other block device attributes.

So, what is the plan for fixing the user land tools and cleaning this up?

Hi Muthu,

Quoted 22 lines · Muthu Kumar

Quoted 21 lines · Chris Ball

Hi,

Quoted 9 lines · Muthu Kumar

Quoted 6 lines

I've already replied to a later version of the patch, but just to get this comment in at the appropriate point of the discussion as well:

Even though it would result in a cleaner sysfs, I don't want to do this now because it will break userspace scripts that are depending on the current locations of these attributes.

Maya is adding a new sysfs attribute with that patch. So, there should not be any user space stuff that depends on it.

In the later patchset, Maya's "[PATCH v4 1/2] mmc: card: Move MMC specific attributes to mmc sub-directory" moves the existing attributes into the mmc/ directory.

It's that move that I'm objecting to, rather than the creation of a new directory -- although since we're going to leave the current attributes where they are, it might not make sense to add the new directory.

Quoted 6 lines

Quoted 3 lines

We'd be creating two places that people have to look for mmc-related attributes, which is arguably less clean than having one place to look even though it's mixed in with the other block device attributes.

So, what is the plan for fixing the user land tools and cleaning this up?

At the moment I don't have any plan to do that, because the cure (potentially breaking userland scripts that are writing to some read/write attributes, by breaking ABI to move everything into a new directory) seems worse than the disease (having some attributes in a directory that isn't the ideal one).

I'd be willing to explore something like Venkat's idea if the block layer maintainers insist, though.

- Chris.
--
Chris Ball <[email protected]> <http://printf.net/>
One Laptop Per Child

Is there anything else that holds this patch from being pushed to mmc-next?

I'd be willing to explore something like Venkat's idea if the block layer maintainers insist, though.

- Chris.
--
Chris Ball <[email protected]> <http://printf.net/>
One Laptop Per Child

Hi,

Quoted 9 lines · Muthu Kumar

Quoted 6 lines

I've already replied to a later version of the patch, but just to get this comment in at the appropriate point of the discussion as well:

Even though it would result in a cleaner sysfs, I don't want to do this now because it will break userspace scripts that are depending on the current locations of these attributes.

Maya is adding a new sysfs attribute with that patch. So, there should not be any user space stuff that depends on it.

In the later patchset, Maya's "[PATCH v4 1/2] mmc: card: Move MMC specific attributes to mmc sub-directory" moves the existing attributes into the mmc/ directory.

It's that move that I'm objecting to, rather than the creation of a new directory -- although since we're going to leave the current attributes where they are, it might not make sense to add the new directory.

We'd be creating two places that people have to look for mmc-related attributes, which is arguably less clean than having one place to look even though it's mixed in with the other block device attributes.