Re: [PATCH v2 1/1] mmc: block: Add write packing control

[merez]

Hi Venkat,

Sorry for the late response. I came back from a long vacation and had many
issues to take care of.
If you still need a rebased version of the packed commands patches, I can
send a rebased version of the write packing control patch once Seungwon
Jeon will send the rebased version of the packing patches.
Please let us know if you ran into conflicts and it is required.

Thanks,
Maya
On Tue, August 28, 2012 10:40 am, S, Venkatraman wrote:
> On Mon, Aug 27, 2012 at 11:58 PM,   wrote:
>>
>> On Fri, July 27, 2012 2:07 am, S, Venkatraman wrote:
>>> On Fri, Jul 27, 2012 at 12:24 AM,   wrote:

 On Thu, July 26, 2012 8:28 am, S, Venkatraman wrote:
> On Tue, Jul 24, 2012 at 2:14 PM,   wrote:
>> On Mon, July 23, 2012 5:22 am, S, Venkatraman wrote:
>>> On Mon, Jul 23, 2012 at 5:13 PM,   wrote:
 On Wed, July 18, 2012 12:26 am, Chris Ball wrote:
> Hi,  [removing Jens and the documentation list, since now we're
>> talking about the MMC side only]
> On Wed, Jul 18 2012, me...@codeaurora.org wrote:
>> Is there anything else that holds this patch from being pushed
>> to
 mmc-next?
> Yes, I'm still uncomfortable with the write packing patchsets for
> a
 couple of reasons, and I suspect that the sum of those reasons
 means
 that
 we should probably plan on holding off merging it until after 3.6.
> Here are the open issues; please correct any misunderstandings:
> With
>> Seungwon's patchset ("Support packed write command"):
> * I still don't have a good set of representative benchmarks
> showing
>   what kind of performance changes come with this patchset.  It
> seems
 like we've had a small amount of testing on one controller/eMMC
 part
 combo
 from Seungwon, and an entirely different test from Maya, and the
>> results
 aren't documented fully anywhere to the level of describing what
 the
>> hardware was, what the test was, and what the results were before
>> and
>> after the patchset.
 Currently, there is only one card vendor that supports packed
 commands.
>> Following are our sequential write (LMDD) test results on 2 of our
>> targets
 (in MB/s):
No packingpacking
 Target 1 (SDR 50MHz) 15   25
 Target 2 (DDR 50MHz) 20   30
> With the reads-during-writes regression:
> * Venkat still has open questions about the nature of the read
>   regression, and thinks we should understand it with blktrace
> before
 trying to fix it.  Maya has a theory about writes overwhelming
 reads,
 but
 Venkat doesn't understand why this would explain the observed
 bandwidth drop.
 The degradation of read due to writes is not a new behavior and
 exists
>> also without the write packing feature (which only increases the
>> degradation). Our investigation of this phenomenon led us to the
>> Conclusion that a new scheduling policy should be used for mobile
>> devices,
 but this is not related to the current discussion of the write
 packing
>> feature.
 The write packing feature increases the degradation of read due to
>> write
 since it allows the MMC to fetch many write requests in a row,
 instead
 of
 fetching only one at a time.  Therefore some of the read requests
 will
>> have to wait for the completion of more write requests before they
>> can
>> be
 issued.
>>>
>>> I am a bit puzzled by this claim. One thing I checked carefully
>>> when
>> reviewing write packing patches from SJeon was that the code didn't
>> plough through a mixed list of reads and writes and selected only
>> writes.
>>> This section of the code in "mmc_blk_prep_packed_list()", from v8
>> patchset..
>>> 
>>> +   if (rq_data_dir(cur) != rq_data_dir(next)) {
>>> +   put_back = 1;
>>> +   break;
>>> +   }
>>> 
>>>
>>> means that once a read is encountered in the middle of write
>>> packing,
>> the packing is stopped at that point and it is executed. Then the
>> next
>> blk_fetch_request should get the next read and continue as before.
>>>
>>> IOW, the ordering of reads and writes is _not_ altered when using
>>> packed
>> commands.
>>> For example if there were 5 write requests, followed by 1 read,
>>> followed by 5 more write requests in the request_queue, the first 5
>> writes will be executed as one "packed command", then the read will
>> be
>> executed, and then the remaining 5 writes will be executed as one
>> "packed command". So the read 

Re: [PATCH v2 1/1] mmc: block: Add write packing control

[merez]

Hi Venkat,

Sorry for the late response. I came back from a long vacation and had many
issues to take care of.
If you still need a rebased version of the packed commands patches, I can
send a rebased version of the write packing control patch once Seungwon
Jeon will send the rebased version of the packing patches.
Please let us know if you ran into conflicts and it is required.

Thanks,
Maya
On Tue, August 28, 2012 10:40 am, S, Venkatraman wrote:
 On Mon, Aug 27, 2012 at 11:58 PM,  me...@codeaurora.org wrote:

 On Fri, July 27, 2012 2:07 am, S, Venkatraman wrote:
 On Fri, Jul 27, 2012 at 12:24 AM,  me...@codeaurora.org wrote:

 On Thu, July 26, 2012 8:28 am, S, Venkatraman wrote:
 On Tue, Jul 24, 2012 at 2:14 PM,  me...@codeaurora.org wrote:
 On Mon, July 23, 2012 5:22 am, S, Venkatraman wrote:
 On Mon, Jul 23, 2012 at 5:13 PM,  me...@codeaurora.org wrote:
 On Wed, July 18, 2012 12:26 am, Chris Ball wrote:
 Hi,  [removing Jens and the documentation list, since now we're
 talking about the MMC side only]
 On Wed, Jul 18 2012, me...@codeaurora.org wrote:
 Is there anything else that holds this patch from being pushed
 to
 mmc-next?
 Yes, I'm still uncomfortable with the write packing patchsets for
 a
 couple of reasons, and I suspect that the sum of those reasons
 means
 that
 we should probably plan on holding off merging it until after 3.6.
 Here are the open issues; please correct any misunderstandings:
 With
 Seungwon's patchset (Support packed write command):
 * I still don't have a good set of representative benchmarks
 showing
   what kind of performance changes come with this patchset.  It
 seems
 like we've had a small amount of testing on one controller/eMMC
 part
 combo
 from Seungwon, and an entirely different test from Maya, and the
 results
 aren't documented fully anywhere to the level of describing what
 the
 hardware was, what the test was, and what the results were before
 and
 after the patchset.
 Currently, there is only one card vendor that supports packed
 commands.
 Following are our sequential write (LMDD) test results on 2 of our
 targets
 (in MB/s):
No packingpacking
 Target 1 (SDR 50MHz) 15   25
 Target 2 (DDR 50MHz) 20   30
 With the reads-during-writes regression:
 * Venkat still has open questions about the nature of the read
   regression, and thinks we should understand it with blktrace
 before
 trying to fix it.  Maya has a theory about writes overwhelming
 reads,
 but
 Venkat doesn't understand why this would explain the observed
 bandwidth drop.
 The degradation of read due to writes is not a new behavior and
 exists
 also without the write packing feature (which only increases the
 degradation). Our investigation of this phenomenon led us to the
 Conclusion that a new scheduling policy should be used for mobile
 devices,
 but this is not related to the current discussion of the write
 packing
 feature.
 The write packing feature increases the degradation of read due to
 write
 since it allows the MMC to fetch many write requests in a row,
 instead
 of
 fetching only one at a time.  Therefore some of the read requests
 will
 have to wait for the completion of more write requests before they
 can
 be
 issued.

 I am a bit puzzled by this claim. One thing I checked carefully
 when
 reviewing write packing patches from SJeon was that the code didn't
 plough through a mixed list of reads and writes and selected only
 writes.
 This section of the code in mmc_blk_prep_packed_list(), from v8
 patchset..
 Quote
 +   if (rq_data_dir(cur) != rq_data_dir(next)) {
 +   put_back = 1;
 +   break;
 +   }
 /Quote

 means that once a read is encountered in the middle of write
 packing,
 the packing is stopped at that point and it is executed. Then the
 next
 blk_fetch_request should get the next read and continue as before.

 IOW, the ordering of reads and writes is _not_ altered when using
 packed
 commands.
 For example if there were 5 write requests, followed by 1 read,
 followed by 5 more write requests in the request_queue, the first 5
 writes will be executed as one packed command, then the read will
 be
 executed, and then the remaining 5 writes will be executed as one
 packed command. So the read does not have to wait any more than it
 waited before (packing feature)

 Let me try to better explain with your example.
 Without packing the MMC layer will fetch 2 write requests and wait
 for
 the
 first write request completion before fetching another write
 request.
 During this time the read request could be inserted into the CFQ and
 since
 it has higher priority than the async write it will be dispatched in
 the
 next fetch. So, the result would be 2 write requests followed by one
 read
 request and the read would have to wait for completion of only 2
 write
 requests.
 With packing, all the 5 write requests will be fetched in a row, and
 then
 the read 

Re: [PATCH v2 1/1] mmc: block: Add write packing control

[S, Venkatraman]

On Mon, Aug 27, 2012 at 11:58 PM,   wrote:
>
> On Fri, July 27, 2012 2:07 am, S, Venkatraman wrote:
>> On Fri, Jul 27, 2012 at 12:24 AM,   wrote:
>>>
>>> On Thu, July 26, 2012 8:28 am, S, Venkatraman wrote:
 On Tue, Jul 24, 2012 at 2:14 PM,   wrote:
> On Mon, July 23, 2012 5:22 am, S, Venkatraman wrote:
>> On Mon, Jul 23, 2012 at 5:13 PM,   wrote:
>>> On Wed, July 18, 2012 12:26 am, Chris Ball wrote:
 Hi,  [removing Jens and the documentation list, since now we're
> talking about the MMC side only]
 On Wed, Jul 18 2012, me...@codeaurora.org wrote:
> Is there anything else that holds this patch from being pushed to
>>> mmc-next?
 Yes, I'm still uncomfortable with the write packing patchsets for a
>>> couple of reasons, and I suspect that the sum of those reasons means
>>> that
>>> we should probably plan on holding off merging it until after 3.6.
 Here are the open issues; please correct any misunderstandings:
 With
> Seungwon's patchset ("Support packed write command"):
 * I still don't have a good set of representative benchmarks
 showing
   what kind of performance changes come with this patchset.  It
 seems
>>> like we've had a small amount of testing on one controller/eMMC part
>>> combo
>>> from Seungwon, and an entirely different test from Maya, and the
> results
>>> aren't documented fully anywhere to the level of describing what the
> hardware was, what the test was, and what the results were before and
> after the patchset.
>>> Currently, there is only one card vendor that supports packed
>>> commands.
> Following are our sequential write (LMDD) test results on 2 of our
> targets
>>> (in MB/s):
>>>No packingpacking
>>> Target 1 (SDR 50MHz) 15   25
>>> Target 2 (DDR 50MHz) 20   30
 With the reads-during-writes regression:
 * Venkat still has open questions about the nature of the read
   regression, and thinks we should understand it with blktrace
 before
>>> trying to fix it.  Maya has a theory about writes overwhelming
>>> reads,
>>> but
>>> Venkat doesn't understand why this would explain the observed
>>> bandwidth drop.
>>> The degradation of read due to writes is not a new behavior and
>>> exists
> also without the write packing feature (which only increases the
> degradation). Our investigation of this phenomenon led us to the
> Conclusion that a new scheduling policy should be used for mobile
> devices,
>>> but this is not related to the current discussion of the write
>>> packing
> feature.
>>> The write packing feature increases the degradation of read due to
> write
>>> since it allows the MMC to fetch many write requests in a row,
>>> instead
>>> of
>>> fetching only one at a time.  Therefore some of the read requests
>>> will
> have to wait for the completion of more write requests before they can
> be
>>> issued.
>>
>> I am a bit puzzled by this claim. One thing I checked carefully when
> reviewing write packing patches from SJeon was that the code didn't
> plough through a mixed list of reads and writes and selected only
> writes.
>> This section of the code in "mmc_blk_prep_packed_list()", from v8
> patchset..
>> 
>> +   if (rq_data_dir(cur) != rq_data_dir(next)) {
>> +   put_back = 1;
>> +   break;
>> +   }
>> 
>>
>> means that once a read is encountered in the middle of write packing,
> the packing is stopped at that point and it is executed. Then the next
> blk_fetch_request should get the next read and continue as before.
>>
>> IOW, the ordering of reads and writes is _not_ altered when using
>> packed
> commands.
>> For example if there were 5 write requests, followed by 1 read,
>> followed by 5 more write requests in the request_queue, the first 5
> writes will be executed as one "packed command", then the read will be
> executed, and then the remaining 5 writes will be executed as one
> "packed command". So the read does not have to wait any more than it
> waited before (packing feature)
>
> Let me try to better explain with your example.
> Without packing the MMC layer will fetch 2 write requests and wait for
> the
> first write request completion before fetching another write request.
> During this time the read request could be inserted into the CFQ and
> since
> it has higher priority than the async write it will be dispatched in
> the
> next fetch. So, the result would be 2 write requests followed by one
> read
> request and the read would have to wait for completion of only 2 write
> 

Re: [PATCH v2 1/1] mmc: block: Add write packing control

[S, Venkatraman]

On Mon, Aug 27, 2012 at 11:58 PM,  me...@codeaurora.org wrote:

 On Fri, July 27, 2012 2:07 am, S, Venkatraman wrote:
 On Fri, Jul 27, 2012 at 12:24 AM,  me...@codeaurora.org wrote:

 On Thu, July 26, 2012 8:28 am, S, Venkatraman wrote:
 On Tue, Jul 24, 2012 at 2:14 PM,  me...@codeaurora.org wrote:
 On Mon, July 23, 2012 5:22 am, S, Venkatraman wrote:
 On Mon, Jul 23, 2012 at 5:13 PM,  me...@codeaurora.org wrote:
 On Wed, July 18, 2012 12:26 am, Chris Ball wrote:
 Hi,  [removing Jens and the documentation list, since now we're
 talking about the MMC side only]
 On Wed, Jul 18 2012, me...@codeaurora.org wrote:
 Is there anything else that holds this patch from being pushed to
 mmc-next?
 Yes, I'm still uncomfortable with the write packing patchsets for a
 couple of reasons, and I suspect that the sum of those reasons means
 that
 we should probably plan on holding off merging it until after 3.6.
 Here are the open issues; please correct any misunderstandings:
 With
 Seungwon's patchset (Support packed write command):
 * I still don't have a good set of representative benchmarks
 showing
   what kind of performance changes come with this patchset.  It
 seems
 like we've had a small amount of testing on one controller/eMMC part
 combo
 from Seungwon, and an entirely different test from Maya, and the
 results
 aren't documented fully anywhere to the level of describing what the
 hardware was, what the test was, and what the results were before and
 after the patchset.
 Currently, there is only one card vendor that supports packed
 commands.
 Following are our sequential write (LMDD) test results on 2 of our
 targets
 (in MB/s):
No packingpacking
 Target 1 (SDR 50MHz) 15   25
 Target 2 (DDR 50MHz) 20   30
 With the reads-during-writes regression:
 * Venkat still has open questions about the nature of the read
   regression, and thinks we should understand it with blktrace
 before
 trying to fix it.  Maya has a theory about writes overwhelming
 reads,
 but
 Venkat doesn't understand why this would explain the observed
 bandwidth drop.
 The degradation of read due to writes is not a new behavior and
 exists
 also without the write packing feature (which only increases the
 degradation). Our investigation of this phenomenon led us to the
 Conclusion that a new scheduling policy should be used for mobile
 devices,
 but this is not related to the current discussion of the write
 packing
 feature.
 The write packing feature increases the degradation of read due to
 write
 since it allows the MMC to fetch many write requests in a row,
 instead
 of
 fetching only one at a time.  Therefore some of the read requests
 will
 have to wait for the completion of more write requests before they can
 be
 issued.

 I am a bit puzzled by this claim. One thing I checked carefully when
 reviewing write packing patches from SJeon was that the code didn't
 plough through a mixed list of reads and writes and selected only
 writes.
 This section of the code in mmc_blk_prep_packed_list(), from v8
 patchset..
 Quote
 +   if (rq_data_dir(cur) != rq_data_dir(next)) {
 +   put_back = 1;
 +   break;
 +   }
 /Quote

 means that once a read is encountered in the middle of write packing,
 the packing is stopped at that point and it is executed. Then the next
 blk_fetch_request should get the next read and continue as before.

 IOW, the ordering of reads and writes is _not_ altered when using
 packed
 commands.
 For example if there were 5 write requests, followed by 1 read,
 followed by 5 more write requests in the request_queue, the first 5
 writes will be executed as one packed command, then the read will be
 executed, and then the remaining 5 writes will be executed as one
 packed command. So the read does not have to wait any more than it
 waited before (packing feature)

 Let me try to better explain with your example.
 Without packing the MMC layer will fetch 2 write requests and wait for
 the
 first write request completion before fetching another write request.
 During this time the read request could be inserted into the CFQ and
 since
 it has higher priority than the async write it will be dispatched in
 the
 next fetch. So, the result would be 2 write requests followed by one
 read
 request and the read would have to wait for completion of only 2 write
 requests.
 With packing, all the 5 write requests will be fetched in a row, and
 then
 the read will arrive and be dispatched in the next fetch. Then the
 read
 will have to wait for the completion of 5 write requests.

 Few more clarifications:
 Due to the plug list mechanism in the block layer the applications can
 aggregate several requests to be inserted into the scheduler before
 waking the MMC queue thread.
 This leads to a situation where there are several write requests in
 the
 CFQ queue when MMC starts to do the fetches.

 If the read was inserted 

Re: [PATCH v2 1/1] mmc: block: Add write packing control

[merez]

On Fri, July 27, 2012 2:07 am, S, Venkatraman wrote:
> On Fri, Jul 27, 2012 at 12:24 AM,   wrote:
>>
>> On Thu, July 26, 2012 8:28 am, S, Venkatraman wrote:
>>> On Tue, Jul 24, 2012 at 2:14 PM,   wrote:
 On Mon, July 23, 2012 5:22 am, S, Venkatraman wrote:
> On Mon, Jul 23, 2012 at 5:13 PM,   wrote:
>> On Wed, July 18, 2012 12:26 am, Chris Ball wrote:
>>> Hi,  [removing Jens and the documentation list, since now we're
 talking about the MMC side only]
>>> On Wed, Jul 18 2012, me...@codeaurora.org wrote:
 Is there anything else that holds this patch from being pushed to
>> mmc-next?
>>> Yes, I'm still uncomfortable with the write packing patchsets for a
>> couple of reasons, and I suspect that the sum of those reasons means
>> that
>> we should probably plan on holding off merging it until after 3.6.
>>> Here are the open issues; please correct any misunderstandings:
>>> With
 Seungwon's patchset ("Support packed write command"):
>>> * I still don't have a good set of representative benchmarks
>>> showing
>>>   what kind of performance changes come with this patchset.  It
>>> seems
>> like we've had a small amount of testing on one controller/eMMC part
>> combo
>> from Seungwon, and an entirely different test from Maya, and the
 results
>> aren't documented fully anywhere to the level of describing what the
 hardware was, what the test was, and what the results were before and
 after the patchset.
>> Currently, there is only one card vendor that supports packed
>> commands.
 Following are our sequential write (LMDD) test results on 2 of our
 targets
>> (in MB/s):
>>No packingpacking
>> Target 1 (SDR 50MHz) 15   25
>> Target 2 (DDR 50MHz) 20   30
>>> With the reads-during-writes regression:
>>> * Venkat still has open questions about the nature of the read
>>>   regression, and thinks we should understand it with blktrace
>>> before
>> trying to fix it.  Maya has a theory about writes overwhelming
>> reads,
>> but
>> Venkat doesn't understand why this would explain the observed
>> bandwidth drop.
>> The degradation of read due to writes is not a new behavior and
>> exists
 also without the write packing feature (which only increases the
 degradation). Our investigation of this phenomenon led us to the
 Conclusion that a new scheduling policy should be used for mobile
 devices,
>> but this is not related to the current discussion of the write
>> packing
 feature.
>> The write packing feature increases the degradation of read due to
 write
>> since it allows the MMC to fetch many write requests in a row,
>> instead
>> of
>> fetching only one at a time.  Therefore some of the read requests
>> will
 have to wait for the completion of more write requests before they can
 be
>> issued.
>
> I am a bit puzzled by this claim. One thing I checked carefully when
 reviewing write packing patches from SJeon was that the code didn't
 plough through a mixed list of reads and writes and selected only
 writes.
> This section of the code in "mmc_blk_prep_packed_list()", from v8
 patchset..
> 
> +   if (rq_data_dir(cur) != rq_data_dir(next)) {
> +   put_back = 1;
> +   break;
> +   }
> 
>
> means that once a read is encountered in the middle of write packing,
 the packing is stopped at that point and it is executed. Then the next
 blk_fetch_request should get the next read and continue as before.
>
> IOW, the ordering of reads and writes is _not_ altered when using
> packed
 commands.
> For example if there were 5 write requests, followed by 1 read,
> followed by 5 more write requests in the request_queue, the first 5
 writes will be executed as one "packed command", then the read will be
 executed, and then the remaining 5 writes will be executed as one
 "packed command". So the read does not have to wait any more than it
 waited before (packing feature)

 Let me try to better explain with your example.
 Without packing the MMC layer will fetch 2 write requests and wait for
 the
 first write request completion before fetching another write request.
 During this time the read request could be inserted into the CFQ and
 since
 it has higher priority than the async write it will be dispatched in
 the
 next fetch. So, the result would be 2 write requests followed by one
 read
 request and the read would have to wait for completion of only 2 write
 requests.
 With packing, all the 5 write requests will be fetched in a row, and
 then
 the read will arrive and be dispatched in the next 

Re: [PATCH v2 1/1] mmc: block: Add write packing control

[merez]

On Fri, July 27, 2012 2:07 am, S, Venkatraman wrote:
 On Fri, Jul 27, 2012 at 12:24 AM,  me...@codeaurora.org wrote:

 On Thu, July 26, 2012 8:28 am, S, Venkatraman wrote:
 On Tue, Jul 24, 2012 at 2:14 PM,  me...@codeaurora.org wrote:
 On Mon, July 23, 2012 5:22 am, S, Venkatraman wrote:
 On Mon, Jul 23, 2012 at 5:13 PM,  me...@codeaurora.org wrote:
 On Wed, July 18, 2012 12:26 am, Chris Ball wrote:
 Hi,  [removing Jens and the documentation list, since now we're
 talking about the MMC side only]
 On Wed, Jul 18 2012, me...@codeaurora.org wrote:
 Is there anything else that holds this patch from being pushed to
 mmc-next?
 Yes, I'm still uncomfortable with the write packing patchsets for a
 couple of reasons, and I suspect that the sum of those reasons means
 that
 we should probably plan on holding off merging it until after 3.6.
 Here are the open issues; please correct any misunderstandings:
 With
 Seungwon's patchset (Support packed write command):
 * I still don't have a good set of representative benchmarks
 showing
   what kind of performance changes come with this patchset.  It
 seems
 like we've had a small amount of testing on one controller/eMMC part
 combo
 from Seungwon, and an entirely different test from Maya, and the
 results
 aren't documented fully anywhere to the level of describing what the
 hardware was, what the test was, and what the results were before and
 after the patchset.
 Currently, there is only one card vendor that supports packed
 commands.
 Following are our sequential write (LMDD) test results on 2 of our
 targets
 (in MB/s):
No packingpacking
 Target 1 (SDR 50MHz) 15   25
 Target 2 (DDR 50MHz) 20   30
 With the reads-during-writes regression:
 * Venkat still has open questions about the nature of the read
   regression, and thinks we should understand it with blktrace
 before
 trying to fix it.  Maya has a theory about writes overwhelming
 reads,
 but
 Venkat doesn't understand why this would explain the observed
 bandwidth drop.
 The degradation of read due to writes is not a new behavior and
 exists
 also without the write packing feature (which only increases the
 degradation). Our investigation of this phenomenon led us to the
 Conclusion that a new scheduling policy should be used for mobile
 devices,
 but this is not related to the current discussion of the write
 packing
 feature.
 The write packing feature increases the degradation of read due to
 write
 since it allows the MMC to fetch many write requests in a row,
 instead
 of
 fetching only one at a time.  Therefore some of the read requests
 will
 have to wait for the completion of more write requests before they can
 be
 issued.

 I am a bit puzzled by this claim. One thing I checked carefully when
 reviewing write packing patches from SJeon was that the code didn't
 plough through a mixed list of reads and writes and selected only
 writes.
 This section of the code in mmc_blk_prep_packed_list(), from v8
 patchset..
 Quote
 +   if (rq_data_dir(cur) != rq_data_dir(next)) {
 +   put_back = 1;
 +   break;
 +   }
 /Quote

 means that once a read is encountered in the middle of write packing,
 the packing is stopped at that point and it is executed. Then the next
 blk_fetch_request should get the next read and continue as before.

 IOW, the ordering of reads and writes is _not_ altered when using
 packed
 commands.
 For example if there were 5 write requests, followed by 1 read,
 followed by 5 more write requests in the request_queue, the first 5
 writes will be executed as one packed command, then the read will be
 executed, and then the remaining 5 writes will be executed as one
 packed command. So the read does not have to wait any more than it
 waited before (packing feature)

 Let me try to better explain with your example.
 Without packing the MMC layer will fetch 2 write requests and wait for
 the
 first write request completion before fetching another write request.
 During this time the read request could be inserted into the CFQ and
 since
 it has higher priority than the async write it will be dispatched in
 the
 next fetch. So, the result would be 2 write requests followed by one
 read
 request and the read would have to wait for completion of only 2 write
 requests.
 With packing, all the 5 write requests will be fetched in a row, and
 then
 the read will arrive and be dispatched in the next fetch. Then the
 read
 will have to wait for the completion of 5 write requests.

 Few more clarifications:
 Due to the plug list mechanism in the block layer the applications can
 aggregate several requests to be inserted into the scheduler before
 waking the MMC queue thread.
 This leads to a situation where there are several write requests in
 the
 CFQ queue when MMC starts to do the fetches.

 If the read was inserted while we are building the packed command then
 I
 agree that we 

Re: [PATCH v2 1/1] mmc: block: Add write packing control

[S, Venkatraman]

On Fri, Jul 27, 2012 at 12:24 AM,   wrote:
>
> On Thu, July 26, 2012 8:28 am, S, Venkatraman wrote:
>> On Tue, Jul 24, 2012 at 2:14 PM,   wrote:
>>> On Mon, July 23, 2012 5:22 am, S, Venkatraman wrote:
 On Mon, Jul 23, 2012 at 5:13 PM,   wrote:
> On Wed, July 18, 2012 12:26 am, Chris Ball wrote:
>> Hi,  [removing Jens and the documentation list, since now we're
>>> talking about the MMC side only]
>> On Wed, Jul 18 2012, me...@codeaurora.org wrote:
>>> Is there anything else that holds this patch from being pushed to
> mmc-next?
>> Yes, I'm still uncomfortable with the write packing patchsets for a
> couple of reasons, and I suspect that the sum of those reasons means
> that
> we should probably plan on holding off merging it until after 3.6.
>> Here are the open issues; please correct any misunderstandings: With
>>> Seungwon's patchset ("Support packed write command"):
>> * I still don't have a good set of representative benchmarks showing
>>   what kind of performance changes come with this patchset.  It seems
> like we've had a small amount of testing on one controller/eMMC part
> combo
> from Seungwon, and an entirely different test from Maya, and the
>>> results
> aren't documented fully anywhere to the level of describing what the
>>> hardware was, what the test was, and what the results were before and
>>> after the patchset.
> Currently, there is only one card vendor that supports packed
> commands.
>>> Following are our sequential write (LMDD) test results on 2 of our
>>> targets
> (in MB/s):
>No packingpacking
> Target 1 (SDR 50MHz) 15   25
> Target 2 (DDR 50MHz) 20   30
>> With the reads-during-writes regression:
>> * Venkat still has open questions about the nature of the read
>>   regression, and thinks we should understand it with blktrace before
> trying to fix it.  Maya has a theory about writes overwhelming reads,
> but
> Venkat doesn't understand why this would explain the observed
> bandwidth drop.
> The degradation of read due to writes is not a new behavior and exists
>>> also without the write packing feature (which only increases the
>>> degradation). Our investigation of this phenomenon led us to the
>>> Conclusion that a new scheduling policy should be used for mobile
>>> devices,
> but this is not related to the current discussion of the write packing
>>> feature.
> The write packing feature increases the degradation of read due to
>>> write
> since it allows the MMC to fetch many write requests in a row, instead
> of
> fetching only one at a time.  Therefore some of the read requests will
>>> have to wait for the completion of more write requests before they can
>>> be
> issued.

 I am a bit puzzled by this claim. One thing I checked carefully when
>>> reviewing write packing patches from SJeon was that the code didn't
>>> plough through a mixed list of reads and writes and selected only
>>> writes.
 This section of the code in "mmc_blk_prep_packed_list()", from v8
>>> patchset..
 
 +   if (rq_data_dir(cur) != rq_data_dir(next)) {
 +   put_back = 1;
 +   break;
 +   }
 

 means that once a read is encountered in the middle of write packing,
>>> the packing is stopped at that point and it is executed. Then the next
>>> blk_fetch_request should get the next read and continue as before.

 IOW, the ordering of reads and writes is _not_ altered when using
 packed
>>> commands.
 For example if there were 5 write requests, followed by 1 read,
 followed by 5 more write requests in the request_queue, the first 5
>>> writes will be executed as one "packed command", then the read will be
>>> executed, and then the remaining 5 writes will be executed as one
>>> "packed command". So the read does not have to wait any more than it
>>> waited before (packing feature)
>>>
>>> Let me try to better explain with your example.
>>> Without packing the MMC layer will fetch 2 write requests and wait for
>>> the
>>> first write request completion before fetching another write request.
>>> During this time the read request could be inserted into the CFQ and
>>> since
>>> it has higher priority than the async write it will be dispatched in the
>>> next fetch. So, the result would be 2 write requests followed by one
>>> read
>>> request and the read would have to wait for completion of only 2 write
>>> requests.
>>> With packing, all the 5 write requests will be fetched in a row, and
>>> then
>>> the read will arrive and be dispatched in the next fetch. Then the read
>>> will have to wait for the completion of 5 write requests.
>>>
>>> Few more clarifications:
>>> Due to the plug list mechanism in the block layer the applications can
>>> "aggregate" several requests to be 

Re: [PATCH v2 1/1] mmc: block: Add write packing control

[S, Venkatraman]

On Fri, Jul 27, 2012 at 12:24 AM,  me...@codeaurora.org wrote:

 On Thu, July 26, 2012 8:28 am, S, Venkatraman wrote:
 On Tue, Jul 24, 2012 at 2:14 PM,  me...@codeaurora.org wrote:
 On Mon, July 23, 2012 5:22 am, S, Venkatraman wrote:
 On Mon, Jul 23, 2012 at 5:13 PM,  me...@codeaurora.org wrote:
 On Wed, July 18, 2012 12:26 am, Chris Ball wrote:
 Hi,  [removing Jens and the documentation list, since now we're
 talking about the MMC side only]
 On Wed, Jul 18 2012, me...@codeaurora.org wrote:
 Is there anything else that holds this patch from being pushed to
 mmc-next?
 Yes, I'm still uncomfortable with the write packing patchsets for a
 couple of reasons, and I suspect that the sum of those reasons means
 that
 we should probably plan on holding off merging it until after 3.6.
 Here are the open issues; please correct any misunderstandings: With
 Seungwon's patchset (Support packed write command):
 * I still don't have a good set of representative benchmarks showing
   what kind of performance changes come with this patchset.  It seems
 like we've had a small amount of testing on one controller/eMMC part
 combo
 from Seungwon, and an entirely different test from Maya, and the
 results
 aren't documented fully anywhere to the level of describing what the
 hardware was, what the test was, and what the results were before and
 after the patchset.
 Currently, there is only one card vendor that supports packed
 commands.
 Following are our sequential write (LMDD) test results on 2 of our
 targets
 (in MB/s):
No packingpacking
 Target 1 (SDR 50MHz) 15   25
 Target 2 (DDR 50MHz) 20   30
 With the reads-during-writes regression:
 * Venkat still has open questions about the nature of the read
   regression, and thinks we should understand it with blktrace before
 trying to fix it.  Maya has a theory about writes overwhelming reads,
 but
 Venkat doesn't understand why this would explain the observed
 bandwidth drop.
 The degradation of read due to writes is not a new behavior and exists
 also without the write packing feature (which only increases the
 degradation). Our investigation of this phenomenon led us to the
 Conclusion that a new scheduling policy should be used for mobile
 devices,
 but this is not related to the current discussion of the write packing
 feature.
 The write packing feature increases the degradation of read due to
 write
 since it allows the MMC to fetch many write requests in a row, instead
 of
 fetching only one at a time.  Therefore some of the read requests will
 have to wait for the completion of more write requests before they can
 be
 issued.

 I am a bit puzzled by this claim. One thing I checked carefully when
 reviewing write packing patches from SJeon was that the code didn't
 plough through a mixed list of reads and writes and selected only
 writes.
 This section of the code in mmc_blk_prep_packed_list(), from v8
 patchset..
 Quote
 +   if (rq_data_dir(cur) != rq_data_dir(next)) {
 +   put_back = 1;
 +   break;
 +   }
 /Quote

 means that once a read is encountered in the middle of write packing,
 the packing is stopped at that point and it is executed. Then the next
 blk_fetch_request should get the next read and continue as before.

 IOW, the ordering of reads and writes is _not_ altered when using
 packed
 commands.
 For example if there were 5 write requests, followed by 1 read,
 followed by 5 more write requests in the request_queue, the first 5
 writes will be executed as one packed command, then the read will be
 executed, and then the remaining 5 writes will be executed as one
 packed command. So the read does not have to wait any more than it
 waited before (packing feature)

 Let me try to better explain with your example.
 Without packing the MMC layer will fetch 2 write requests and wait for
 the
 first write request completion before fetching another write request.
 During this time the read request could be inserted into the CFQ and
 since
 it has higher priority than the async write it will be dispatched in the
 next fetch. So, the result would be 2 write requests followed by one
 read
 request and the read would have to wait for completion of only 2 write
 requests.
 With packing, all the 5 write requests will be fetched in a row, and
 then
 the read will arrive and be dispatched in the next fetch. Then the read
 will have to wait for the completion of 5 write requests.

 Few more clarifications:
 Due to the plug list mechanism in the block layer the applications can
 aggregate several requests to be inserted into the scheduler before
 waking the MMC queue thread.
 This leads to a situation where there are several write requests in the
 CFQ queue when MMC starts to do the fetches.

 If the read was inserted while we are building the packed command then I
 agree that we should have seen less effect on the read performance.
 However, the 

Re: [PATCH v2 1/1] mmc: block: Add write packing control

[merez]

On Thu, July 26, 2012 8:28 am, S, Venkatraman wrote:
> On Tue, Jul 24, 2012 at 2:14 PM,   wrote:
>> On Mon, July 23, 2012 5:22 am, S, Venkatraman wrote:
>>> On Mon, Jul 23, 2012 at 5:13 PM,   wrote:
 On Wed, July 18, 2012 12:26 am, Chris Ball wrote:
> Hi,  [removing Jens and the documentation list, since now we're
>> talking about the MMC side only]
> On Wed, Jul 18 2012, me...@codeaurora.org wrote:
>> Is there anything else that holds this patch from being pushed to
 mmc-next?
> Yes, I'm still uncomfortable with the write packing patchsets for a
 couple of reasons, and I suspect that the sum of those reasons means
 that
 we should probably plan on holding off merging it until after 3.6.
> Here are the open issues; please correct any misunderstandings: With
>> Seungwon's patchset ("Support packed write command"):
> * I still don't have a good set of representative benchmarks showing
>   what kind of performance changes come with this patchset.  It seems
 like we've had a small amount of testing on one controller/eMMC part
 combo
 from Seungwon, and an entirely different test from Maya, and the
>> results
 aren't documented fully anywhere to the level of describing what the
>> hardware was, what the test was, and what the results were before and
>> after the patchset.
 Currently, there is only one card vendor that supports packed
 commands.
>> Following are our sequential write (LMDD) test results on 2 of our
>> targets
 (in MB/s):
No packingpacking
 Target 1 (SDR 50MHz) 15   25
 Target 2 (DDR 50MHz) 20   30
> With the reads-during-writes regression:
> * Venkat still has open questions about the nature of the read
>   regression, and thinks we should understand it with blktrace before
 trying to fix it.  Maya has a theory about writes overwhelming reads,
 but
 Venkat doesn't understand why this would explain the observed
 bandwidth drop.
 The degradation of read due to writes is not a new behavior and exists
>> also without the write packing feature (which only increases the
>> degradation). Our investigation of this phenomenon led us to the
>> Conclusion that a new scheduling policy should be used for mobile
>> devices,
 but this is not related to the current discussion of the write packing
>> feature.
 The write packing feature increases the degradation of read due to
>> write
 since it allows the MMC to fetch many write requests in a row, instead
 of
 fetching only one at a time.  Therefore some of the read requests will
>> have to wait for the completion of more write requests before they can
>> be
 issued.
>>>
>>> I am a bit puzzled by this claim. One thing I checked carefully when
>> reviewing write packing patches from SJeon was that the code didn't
>> plough through a mixed list of reads and writes and selected only
>> writes.
>>> This section of the code in "mmc_blk_prep_packed_list()", from v8
>> patchset..
>>> 
>>> +   if (rq_data_dir(cur) != rq_data_dir(next)) {
>>> +   put_back = 1;
>>> +   break;
>>> +   }
>>> 
>>>
>>> means that once a read is encountered in the middle of write packing,
>> the packing is stopped at that point and it is executed. Then the next
>> blk_fetch_request should get the next read and continue as before.
>>>
>>> IOW, the ordering of reads and writes is _not_ altered when using
>>> packed
>> commands.
>>> For example if there were 5 write requests, followed by 1 read,
>>> followed by 5 more write requests in the request_queue, the first 5
>> writes will be executed as one "packed command", then the read will be
>> executed, and then the remaining 5 writes will be executed as one
>> "packed command". So the read does not have to wait any more than it
>> waited before (packing feature)
>>
>> Let me try to better explain with your example.
>> Without packing the MMC layer will fetch 2 write requests and wait for
>> the
>> first write request completion before fetching another write request.
>> During this time the read request could be inserted into the CFQ and
>> since
>> it has higher priority than the async write it will be dispatched in the
>> next fetch. So, the result would be 2 write requests followed by one
>> read
>> request and the read would have to wait for completion of only 2 write
>> requests.
>> With packing, all the 5 write requests will be fetched in a row, and
>> then
>> the read will arrive and be dispatched in the next fetch. Then the read
>> will have to wait for the completion of 5 write requests.
>>
>> Few more clarifications:
>> Due to the plug list mechanism in the block layer the applications can
>> "aggregate" several requests to be inserted into the scheduler before
>> waking the MMC queue thread.
>> This leads to a situation where there are several write requests in the
>> 

Re: [PATCH v2 1/1] mmc: block: Add write packing control

[S, Venkatraman]

On Tue, Jul 24, 2012 at 2:14 PM,   wrote:
> On Mon, July 23, 2012 5:22 am, S, Venkatraman wrote:
>> On Mon, Jul 23, 2012 at 5:13 PM,   wrote:
>>> On Wed, July 18, 2012 12:26 am, Chris Ball wrote:
 Hi,  [removing Jens and the documentation list, since now we're
> talking about the MMC side only]
 On Wed, Jul 18 2012, me...@codeaurora.org wrote:
> Is there anything else that holds this patch from being pushed to
>>> mmc-next?
 Yes, I'm still uncomfortable with the write packing patchsets for a
>>> couple of reasons, and I suspect that the sum of those reasons means that
>>> we should probably plan on holding off merging it until after 3.6.
 Here are the open issues; please correct any misunderstandings: With
> Seungwon's patchset ("Support packed write command"):
 * I still don't have a good set of representative benchmarks showing
   what kind of performance changes come with this patchset.  It seems
>>> like we've had a small amount of testing on one controller/eMMC part combo
>>> from Seungwon, and an entirely different test from Maya, and the
> results
>>> aren't documented fully anywhere to the level of describing what the
> hardware was, what the test was, and what the results were before and
> after the patchset.
>>> Currently, there is only one card vendor that supports packed commands.
> Following are our sequential write (LMDD) test results on 2 of our
> targets
>>> (in MB/s):
>>>No packingpacking
>>> Target 1 (SDR 50MHz) 15   25
>>> Target 2 (DDR 50MHz) 20   30
 With the reads-during-writes regression:
 * Venkat still has open questions about the nature of the read
   regression, and thinks we should understand it with blktrace before
>>> trying to fix it.  Maya has a theory about writes overwhelming reads, but
>>> Venkat doesn't understand why this would explain the observed
>>> bandwidth drop.
>>> The degradation of read due to writes is not a new behavior and exists
> also without the write packing feature (which only increases the
> degradation). Our investigation of this phenomenon led us to the
> Conclusion that a new scheduling policy should be used for mobile
> devices,
>>> but this is not related to the current discussion of the write packing
> feature.
>>> The write packing feature increases the degradation of read due to
> write
>>> since it allows the MMC to fetch many write requests in a row, instead of
>>> fetching only one at a time.  Therefore some of the read requests will
> have to wait for the completion of more write requests before they can
> be
>>> issued.
>>
>> I am a bit puzzled by this claim. One thing I checked carefully when
> reviewing write packing patches from SJeon was that the code didn't
> plough through a mixed list of reads and writes and selected only
> writes.
>> This section of the code in "mmc_blk_prep_packed_list()", from v8
> patchset..
>> 
>> +   if (rq_data_dir(cur) != rq_data_dir(next)) {
>> +   put_back = 1;
>> +   break;
>> +   }
>> 
>>
>> means that once a read is encountered in the middle of write packing,
> the packing is stopped at that point and it is executed. Then the next
> blk_fetch_request should get the next read and continue as before.
>>
>> IOW, the ordering of reads and writes is _not_ altered when using packed
> commands.
>> For example if there were 5 write requests, followed by 1 read,
>> followed by 5 more write requests in the request_queue, the first 5
> writes will be executed as one "packed command", then the read will be
> executed, and then the remaining 5 writes will be executed as one
> "packed command". So the read does not have to wait any more than it
> waited before (packing feature)
>
> Let me try to better explain with your example.
> Without packing the MMC layer will fetch 2 write requests and wait for the
> first write request completion before fetching another write request.
> During this time the read request could be inserted into the CFQ and since
> it has higher priority than the async write it will be dispatched in the
> next fetch. So, the result would be 2 write requests followed by one read
> request and the read would have to wait for completion of only 2 write
> requests.
> With packing, all the 5 write requests will be fetched in a row, and then
> the read will arrive and be dispatched in the next fetch. Then the read
> will have to wait for the completion of 5 write requests.
>
> Few more clarifications:
> Due to the plug list mechanism in the block layer the applications can
> "aggregate" several requests to be inserted into the scheduler before
> waking the MMC queue thread.
> This leads to a situation where there are several write requests in the
> CFQ queue when MMC starts to do the fetches.
>
> If the read was inserted while we are building the packed command then I
> agree that we should have seen less effect on the read performance.

Re: [PATCH v2 1/1] mmc: block: Add write packing control

[S, Venkatraman]

On Tue, Jul 24, 2012 at 2:14 PM,  me...@codeaurora.org wrote:
 On Mon, July 23, 2012 5:22 am, S, Venkatraman wrote:
 On Mon, Jul 23, 2012 at 5:13 PM,  me...@codeaurora.org wrote:
 On Wed, July 18, 2012 12:26 am, Chris Ball wrote:
 Hi,  [removing Jens and the documentation list, since now we're
 talking about the MMC side only]
 On Wed, Jul 18 2012, me...@codeaurora.org wrote:
 Is there anything else that holds this patch from being pushed to
 mmc-next?
 Yes, I'm still uncomfortable with the write packing patchsets for a
 couple of reasons, and I suspect that the sum of those reasons means that
 we should probably plan on holding off merging it until after 3.6.
 Here are the open issues; please correct any misunderstandings: With
 Seungwon's patchset (Support packed write command):
 * I still don't have a good set of representative benchmarks showing
   what kind of performance changes come with this patchset.  It seems
 like we've had a small amount of testing on one controller/eMMC part combo
 from Seungwon, and an entirely different test from Maya, and the
 results
 aren't documented fully anywhere to the level of describing what the
 hardware was, what the test was, and what the results were before and
 after the patchset.
 Currently, there is only one card vendor that supports packed commands.
 Following are our sequential write (LMDD) test results on 2 of our
 targets
 (in MB/s):
No packingpacking
 Target 1 (SDR 50MHz) 15   25
 Target 2 (DDR 50MHz) 20   30
 With the reads-during-writes regression:
 * Venkat still has open questions about the nature of the read
   regression, and thinks we should understand it with blktrace before
 trying to fix it.  Maya has a theory about writes overwhelming reads, but
 Venkat doesn't understand why this would explain the observed
 bandwidth drop.
 The degradation of read due to writes is not a new behavior and exists
 also without the write packing feature (which only increases the
 degradation). Our investigation of this phenomenon led us to the
 Conclusion that a new scheduling policy should be used for mobile
 devices,
 but this is not related to the current discussion of the write packing
 feature.
 The write packing feature increases the degradation of read due to
 write
 since it allows the MMC to fetch many write requests in a row, instead of
 fetching only one at a time.  Therefore some of the read requests will
 have to wait for the completion of more write requests before they can
 be
 issued.

 I am a bit puzzled by this claim. One thing I checked carefully when
 reviewing write packing patches from SJeon was that the code didn't
 plough through a mixed list of reads and writes and selected only
 writes.
 This section of the code in mmc_blk_prep_packed_list(), from v8
 patchset..
 Quote
 +   if (rq_data_dir(cur) != rq_data_dir(next)) {
 +   put_back = 1;
 +   break;
 +   }
 /Quote

 means that once a read is encountered in the middle of write packing,
 the packing is stopped at that point and it is executed. Then the next
 blk_fetch_request should get the next read and continue as before.

 IOW, the ordering of reads and writes is _not_ altered when using packed
 commands.
 For example if there were 5 write requests, followed by 1 read,
 followed by 5 more write requests in the request_queue, the first 5
 writes will be executed as one packed command, then the read will be
 executed, and then the remaining 5 writes will be executed as one
 packed command. So the read does not have to wait any more than it
 waited before (packing feature)

 Let me try to better explain with your example.
 Without packing the MMC layer will fetch 2 write requests and wait for the
 first write request completion before fetching another write request.
 During this time the read request could be inserted into the CFQ and since
 it has higher priority than the async write it will be dispatched in the
 next fetch. So, the result would be 2 write requests followed by one read
 request and the read would have to wait for completion of only 2 write
 requests.
 With packing, all the 5 write requests will be fetched in a row, and then
 the read will arrive and be dispatched in the next fetch. Then the read
 will have to wait for the completion of 5 write requests.

 Few more clarifications:
 Due to the plug list mechanism in the block layer the applications can
 aggregate several requests to be inserted into the scheduler before
 waking the MMC queue thread.
 This leads to a situation where there are several write requests in the
 CFQ queue when MMC starts to do the fetches.

 If the read was inserted while we are building the packed command then I
 agree that we should have seen less effect on the read performance.
 However, the write packing statistics show that in most of the cases the
 packing stopped due to an empty queue, meaning that the read was 

Re: [PATCH v2 1/1] mmc: block: Add write packing control

[merez]

On Thu, July 26, 2012 8:28 am, S, Venkatraman wrote:
 On Tue, Jul 24, 2012 at 2:14 PM,  me...@codeaurora.org wrote:
 On Mon, July 23, 2012 5:22 am, S, Venkatraman wrote:
 On Mon, Jul 23, 2012 at 5:13 PM,  me...@codeaurora.org wrote:
 On Wed, July 18, 2012 12:26 am, Chris Ball wrote:
 Hi,  [removing Jens and the documentation list, since now we're
 talking about the MMC side only]
 On Wed, Jul 18 2012, me...@codeaurora.org wrote:
 Is there anything else that holds this patch from being pushed to
 mmc-next?
 Yes, I'm still uncomfortable with the write packing patchsets for a
 couple of reasons, and I suspect that the sum of those reasons means
 that
 we should probably plan on holding off merging it until after 3.6.
 Here are the open issues; please correct any misunderstandings: With
 Seungwon's patchset (Support packed write command):
 * I still don't have a good set of representative benchmarks showing
   what kind of performance changes come with this patchset.  It seems
 like we've had a small amount of testing on one controller/eMMC part
 combo
 from Seungwon, and an entirely different test from Maya, and the
 results
 aren't documented fully anywhere to the level of describing what the
 hardware was, what the test was, and what the results were before and
 after the patchset.
 Currently, there is only one card vendor that supports packed
 commands.
 Following are our sequential write (LMDD) test results on 2 of our
 targets
 (in MB/s):
No packingpacking
 Target 1 (SDR 50MHz) 15   25
 Target 2 (DDR 50MHz) 20   30
 With the reads-during-writes regression:
 * Venkat still has open questions about the nature of the read
   regression, and thinks we should understand it with blktrace before
 trying to fix it.  Maya has a theory about writes overwhelming reads,
 but
 Venkat doesn't understand why this would explain the observed
 bandwidth drop.
 The degradation of read due to writes is not a new behavior and exists
 also without the write packing feature (which only increases the
 degradation). Our investigation of this phenomenon led us to the
 Conclusion that a new scheduling policy should be used for mobile
 devices,
 but this is not related to the current discussion of the write packing
 feature.
 The write packing feature increases the degradation of read due to
 write
 since it allows the MMC to fetch many write requests in a row, instead
 of
 fetching only one at a time.  Therefore some of the read requests will
 have to wait for the completion of more write requests before they can
 be
 issued.

 I am a bit puzzled by this claim. One thing I checked carefully when
 reviewing write packing patches from SJeon was that the code didn't
 plough through a mixed list of reads and writes and selected only
 writes.
 This section of the code in mmc_blk_prep_packed_list(), from v8
 patchset..
 Quote
 +   if (rq_data_dir(cur) != rq_data_dir(next)) {
 +   put_back = 1;
 +   break;
 +   }
 /Quote

 means that once a read is encountered in the middle of write packing,
 the packing is stopped at that point and it is executed. Then the next
 blk_fetch_request should get the next read and continue as before.

 IOW, the ordering of reads and writes is _not_ altered when using
 packed
 commands.
 For example if there were 5 write requests, followed by 1 read,
 followed by 5 more write requests in the request_queue, the first 5
 writes will be executed as one packed command, then the read will be
 executed, and then the remaining 5 writes will be executed as one
 packed command. So the read does not have to wait any more than it
 waited before (packing feature)

 Let me try to better explain with your example.
 Without packing the MMC layer will fetch 2 write requests and wait for
 the
 first write request completion before fetching another write request.
 During this time the read request could be inserted into the CFQ and
 since
 it has higher priority than the async write it will be dispatched in the
 next fetch. So, the result would be 2 write requests followed by one
 read
 request and the read would have to wait for completion of only 2 write
 requests.
 With packing, all the 5 write requests will be fetched in a row, and
 then
 the read will arrive and be dispatched in the next fetch. Then the read
 will have to wait for the completion of 5 write requests.

 Few more clarifications:
 Due to the plug list mechanism in the block layer the applications can
 aggregate several requests to be inserted into the scheduler before
 waking the MMC queue thread.
 This leads to a situation where there are several write requests in the
 CFQ queue when MMC starts to do the fetches.

 If the read was inserted while we are building the packed command then I
 agree that we should have seen less effect on the read performance.
 However, the write packing statistics show that in most of the cases the
 

Re: [PATCH v2 1/1] mmc: block: Add write packing control

[merez]

Attached are the trace logs for parallel read and write lmdd operations.

On Tue, July 24, 2012 1:44 am, me...@codeaurora.org wrote:
> On Mon, July 23, 2012 5:22 am, S, Venkatraman wrote:
>> On Mon, Jul 23, 2012 at 5:13 PM,   wrote:
>>> On Wed, July 18, 2012 12:26 am, Chris Ball wrote:
 Hi,  [removing Jens and the documentation list, since now we're
> talking about the MMC side only]
 On Wed, Jul 18 2012, me...@codeaurora.org wrote:
> Is there anything else that holds this patch from being pushed to
>>> mmc-next?
 Yes, I'm still uncomfortable with the write packing patchsets for a
>>> couple of reasons, and I suspect that the sum of those reasons means
>>> that
>>> we should probably plan on holding off merging it until after 3.6.
 Here are the open issues; please correct any misunderstandings: With
> Seungwon's patchset ("Support packed write command"):
 * I still don't have a good set of representative benchmarks showing
   what kind of performance changes come with this patchset.  It seems
>>> like we've had a small amount of testing on one controller/eMMC part
>>> combo
>>> from Seungwon, and an entirely different test from Maya, and the
> results
>>> aren't documented fully anywhere to the level of describing what the
> hardware was, what the test was, and what the results were before and
> after the patchset.
>>> Currently, there is only one card vendor that supports packed commands.
> Following are our sequential write (LMDD) test results on 2 of our
> targets
>>> (in MB/s):
>>>No packingpacking
>>> Target 1 (SDR 50MHz) 15   25
>>> Target 2 (DDR 50MHz) 20   30
 With the reads-during-writes regression:
 * Venkat still has open questions about the nature of the read
   regression, and thinks we should understand it with blktrace before
>>> trying to fix it.  Maya has a theory about writes overwhelming reads,
>>> but
>>> Venkat doesn't understand why this would explain the observed
>>> bandwidth drop.
>>> The degradation of read due to writes is not a new behavior and exists
> also without the write packing feature (which only increases the
> degradation). Our investigation of this phenomenon led us to the
> Conclusion that a new scheduling policy should be used for mobile
> devices,
>>> but this is not related to the current discussion of the write packing
> feature.
>>> The write packing feature increases the degradation of read due to
> write
>>> since it allows the MMC to fetch many write requests in a row, instead
>>> of
>>> fetching only one at a time.  Therefore some of the read requests will
> have to wait for the completion of more write requests before they can
> be
>>> issued.
>>
>> I am a bit puzzled by this claim. One thing I checked carefully when
> reviewing write packing patches from SJeon was that the code didn't
> plough through a mixed list of reads and writes and selected only
> writes.
>> This section of the code in "mmc_blk_prep_packed_list()", from v8
> patchset..
>> 
>> +   if (rq_data_dir(cur) != rq_data_dir(next)) {
>> +   put_back = 1;
>> +   break;
>> +   }
>> 
>>
>> means that once a read is encountered in the middle of write packing,
> the packing is stopped at that point and it is executed. Then the next
> blk_fetch_request should get the next read and continue as before.
>>
>> IOW, the ordering of reads and writes is _not_ altered when using packed
> commands.
>> For example if there were 5 write requests, followed by 1 read,
>> followed by 5 more write requests in the request_queue, the first 5
> writes will be executed as one "packed command", then the read will be
> executed, and then the remaining 5 writes will be executed as one
> "packed command". So the read does not have to wait any more than it
> waited before (packing feature)
>
> Let me try to better explain with your example.
> Without packing the MMC layer will fetch 2 write requests and wait for the
> first write request completion before fetching another write request.
> During this time the read request could be inserted into the CFQ and since
> it has higher priority than the async write it will be dispatched in the
> next fetch. So, the result would be 2 write requests followed by one read
> request and the read would have to wait for completion of only 2 write
> requests.
> With packing, all the 5 write requests will be fetched in a row, and then
> the read will arrive and be dispatched in the next fetch. Then the read
> will have to wait for the completion of 5 write requests.
>
> Few more clarifications:
> Due to the plug list mechanism in the block layer the applications can
> "aggregate" several requests to be inserted into the scheduler before
> waking the MMC queue thread.
> This leads to a situation where there are several write requests in the
> CFQ queue when MMC starts to do the fetches.
>
> If the read was inserted while we 

Re: [PATCH v2 1/1] mmc: block: Add write packing control

[merez]

On Mon, July 23, 2012 5:22 am, S, Venkatraman wrote:
> On Mon, Jul 23, 2012 at 5:13 PM,   wrote:
>> On Wed, July 18, 2012 12:26 am, Chris Ball wrote:
>>> Hi,  [removing Jens and the documentation list, since now we're
talking about the MMC side only]
>>> On Wed, Jul 18 2012, me...@codeaurora.org wrote:
 Is there anything else that holds this patch from being pushed to
>> mmc-next?
>>> Yes, I'm still uncomfortable with the write packing patchsets for a
>> couple of reasons, and I suspect that the sum of those reasons means that
>> we should probably plan on holding off merging it until after 3.6.
>>> Here are the open issues; please correct any misunderstandings: With
Seungwon's patchset ("Support packed write command"):
>>> * I still don't have a good set of representative benchmarks showing
>>>   what kind of performance changes come with this patchset.  It seems
>> like we've had a small amount of testing on one controller/eMMC part combo
>> from Seungwon, and an entirely different test from Maya, and the
results
>> aren't documented fully anywhere to the level of describing what the
hardware was, what the test was, and what the results were before and
after the patchset.
>> Currently, there is only one card vendor that supports packed commands.
Following are our sequential write (LMDD) test results on 2 of our
targets
>> (in MB/s):
>>No packingpacking
>> Target 1 (SDR 50MHz) 15   25
>> Target 2 (DDR 50MHz) 20   30
>>> With the reads-during-writes regression:
>>> * Venkat still has open questions about the nature of the read
>>>   regression, and thinks we should understand it with blktrace before
>> trying to fix it.  Maya has a theory about writes overwhelming reads, but
>> Venkat doesn't understand why this would explain the observed
>> bandwidth drop.
>> The degradation of read due to writes is not a new behavior and exists
also without the write packing feature (which only increases the
degradation). Our investigation of this phenomenon led us to the
Conclusion that a new scheduling policy should be used for mobile
devices,
>> but this is not related to the current discussion of the write packing
feature.
>> The write packing feature increases the degradation of read due to
write
>> since it allows the MMC to fetch many write requests in a row, instead of
>> fetching only one at a time.  Therefore some of the read requests will
have to wait for the completion of more write requests before they can
be
>> issued.
>
> I am a bit puzzled by this claim. One thing I checked carefully when
reviewing write packing patches from SJeon was that the code didn't
plough through a mixed list of reads and writes and selected only
writes.
> This section of the code in "mmc_blk_prep_packed_list()", from v8
patchset..
> 
> +   if (rq_data_dir(cur) != rq_data_dir(next)) {
> +   put_back = 1;
> +   break;
> +   }
> 
>
> means that once a read is encountered in the middle of write packing,
the packing is stopped at that point and it is executed. Then the next
blk_fetch_request should get the next read and continue as before.
>
> IOW, the ordering of reads and writes is _not_ altered when using packed
commands.
> For example if there were 5 write requests, followed by 1 read,
> followed by 5 more write requests in the request_queue, the first 5
writes will be executed as one "packed command", then the read will be
executed, and then the remaining 5 writes will be executed as one
"packed command". So the read does not have to wait any more than it
waited before (packing feature)

Let me try to better explain with your example.
Without packing the MMC layer will fetch 2 write requests and wait for the
first write request completion before fetching another write request.
During this time the read request could be inserted into the CFQ and since
it has higher priority than the async write it will be dispatched in the
next fetch. So, the result would be 2 write requests followed by one read
request and the read would have to wait for completion of only 2 write
requests.
With packing, all the 5 write requests will be fetched in a row, and then
the read will arrive and be dispatched in the next fetch. Then the read
will have to wait for the completion of 5 write requests.

Few more clarifications:
Due to the plug list mechanism in the block layer the applications can
"aggregate" several requests to be inserted into the scheduler before
waking the MMC queue thread.
This leads to a situation where there are several write requests in the
CFQ queue when MMC starts to do the fetches.

If the read was inserted while we are building the packed command then I
agree that we should have seen less effect on the read performance.
However, the write packing statistics show that in most of the cases the
packing stopped due to an empty queue, meaning that the read was inserted
to the CFQ after all the pending 

Re: [PATCH v2 1/1] mmc: block: Add write packing control

[merez]

On Mon, July 23, 2012 5:22 am, S, Venkatraman wrote:
 On Mon, Jul 23, 2012 at 5:13 PM,  me...@codeaurora.org wrote:
 On Wed, July 18, 2012 12:26 am, Chris Ball wrote:
 Hi,  [removing Jens and the documentation list, since now we're
talking about the MMC side only]
 On Wed, Jul 18 2012, me...@codeaurora.org wrote:
 Is there anything else that holds this patch from being pushed to
 mmc-next?
 Yes, I'm still uncomfortable with the write packing patchsets for a
 couple of reasons, and I suspect that the sum of those reasons means that
 we should probably plan on holding off merging it until after 3.6.
 Here are the open issues; please correct any misunderstandings: With
Seungwon's patchset (Support packed write command):
 * I still don't have a good set of representative benchmarks showing
   what kind of performance changes come with this patchset.  It seems
 like we've had a small amount of testing on one controller/eMMC part combo
 from Seungwon, and an entirely different test from Maya, and the
results
 aren't documented fully anywhere to the level of describing what the
hardware was, what the test was, and what the results were before and
after the patchset.
 Currently, there is only one card vendor that supports packed commands.
Following are our sequential write (LMDD) test results on 2 of our
targets
 (in MB/s):
No packingpacking
 Target 1 (SDR 50MHz) 15   25
 Target 2 (DDR 50MHz) 20   30
 With the reads-during-writes regression:
 * Venkat still has open questions about the nature of the read
   regression, and thinks we should understand it with blktrace before
 trying to fix it.  Maya has a theory about writes overwhelming reads, but
 Venkat doesn't understand why this would explain the observed
 bandwidth drop.
 The degradation of read due to writes is not a new behavior and exists
also without the write packing feature (which only increases the
degradation). Our investigation of this phenomenon led us to the
Conclusion that a new scheduling policy should be used for mobile
devices,
 but this is not related to the current discussion of the write packing
feature.
 The write packing feature increases the degradation of read due to
write
 since it allows the MMC to fetch many write requests in a row, instead of
 fetching only one at a time.  Therefore some of the read requests will
have to wait for the completion of more write requests before they can
be
 issued.

 I am a bit puzzled by this claim. One thing I checked carefully when
reviewing write packing patches from SJeon was that the code didn't
plough through a mixed list of reads and writes and selected only
writes.
 This section of the code in mmc_blk_prep_packed_list(), from v8
patchset..
 Quote
 +   if (rq_data_dir(cur) != rq_data_dir(next)) {
 +   put_back = 1;
 +   break;
 +   }
 /Quote

 means that once a read is encountered in the middle of write packing,
the packing is stopped at that point and it is executed. Then the next
blk_fetch_request should get the next read and continue as before.

 IOW, the ordering of reads and writes is _not_ altered when using packed
commands.
 For example if there were 5 write requests, followed by 1 read,
 followed by 5 more write requests in the request_queue, the first 5
writes will be executed as one packed command, then the read will be
executed, and then the remaining 5 writes will be executed as one
packed command. So the read does not have to wait any more than it
waited before (packing feature)

Let me try to better explain with your example.
Without packing the MMC layer will fetch 2 write requests and wait for the
first write request completion before fetching another write request.
During this time the read request could be inserted into the CFQ and since
it has higher priority than the async write it will be dispatched in the
next fetch. So, the result would be 2 write requests followed by one read
request and the read would have to wait for completion of only 2 write
requests.
With packing, all the 5 write requests will be fetched in a row, and then
the read will arrive and be dispatched in the next fetch. Then the read
will have to wait for the completion of 5 write requests.

Few more clarifications:
Due to the plug list mechanism in the block layer the applications can
aggregate several requests to be inserted into the scheduler before
waking the MMC queue thread.
This leads to a situation where there are several write requests in the
CFQ queue when MMC starts to do the fetches.

If the read was inserted while we are building the packed command then I
agree that we should have seen less effect on the read performance.
However, the write packing statistics show that in most of the cases the
packing stopped due to an empty queue, meaning that the read was inserted
to the CFQ after all the pending write requests were fetched and packed.

Following is an example for 

Re: [PATCH v2 1/1] mmc: block: Add write packing control

[merez]

Attached are the trace logs for parallel read and write lmdd operations.

On Tue, July 24, 2012 1:44 am, me...@codeaurora.org wrote:
 On Mon, July 23, 2012 5:22 am, S, Venkatraman wrote:
 On Mon, Jul 23, 2012 at 5:13 PM,  me...@codeaurora.org wrote:
 On Wed, July 18, 2012 12:26 am, Chris Ball wrote:
 Hi,  [removing Jens and the documentation list, since now we're
 talking about the MMC side only]
 On Wed, Jul 18 2012, me...@codeaurora.org wrote:
 Is there anything else that holds this patch from being pushed to
 mmc-next?
 Yes, I'm still uncomfortable with the write packing patchsets for a
 couple of reasons, and I suspect that the sum of those reasons means
 that
 we should probably plan on holding off merging it until after 3.6.
 Here are the open issues; please correct any misunderstandings: With
 Seungwon's patchset (Support packed write command):
 * I still don't have a good set of representative benchmarks showing
   what kind of performance changes come with this patchset.  It seems
 like we've had a small amount of testing on one controller/eMMC part
 combo
 from Seungwon, and an entirely different test from Maya, and the
 results
 aren't documented fully anywhere to the level of describing what the
 hardware was, what the test was, and what the results were before and
 after the patchset.
 Currently, there is only one card vendor that supports packed commands.
 Following are our sequential write (LMDD) test results on 2 of our
 targets
 (in MB/s):
No packingpacking
 Target 1 (SDR 50MHz) 15   25
 Target 2 (DDR 50MHz) 20   30
 With the reads-during-writes regression:
 * Venkat still has open questions about the nature of the read
   regression, and thinks we should understand it with blktrace before
 trying to fix it.  Maya has a theory about writes overwhelming reads,
 but
 Venkat doesn't understand why this would explain the observed
 bandwidth drop.
 The degradation of read due to writes is not a new behavior and exists
 also without the write packing feature (which only increases the
 degradation). Our investigation of this phenomenon led us to the
 Conclusion that a new scheduling policy should be used for mobile
 devices,
 but this is not related to the current discussion of the write packing
 feature.
 The write packing feature increases the degradation of read due to
 write
 since it allows the MMC to fetch many write requests in a row, instead
 of
 fetching only one at a time.  Therefore some of the read requests will
 have to wait for the completion of more write requests before they can
 be
 issued.

 I am a bit puzzled by this claim. One thing I checked carefully when
 reviewing write packing patches from SJeon was that the code didn't
 plough through a mixed list of reads and writes and selected only
 writes.
 This section of the code in mmc_blk_prep_packed_list(), from v8
 patchset..
 Quote
 +   if (rq_data_dir(cur) != rq_data_dir(next)) {
 +   put_back = 1;
 +   break;
 +   }
 /Quote

 means that once a read is encountered in the middle of write packing,
 the packing is stopped at that point and it is executed. Then the next
 blk_fetch_request should get the next read and continue as before.

 IOW, the ordering of reads and writes is _not_ altered when using packed
 commands.
 For example if there were 5 write requests, followed by 1 read,
 followed by 5 more write requests in the request_queue, the first 5
 writes will be executed as one packed command, then the read will be
 executed, and then the remaining 5 writes will be executed as one
 packed command. So the read does not have to wait any more than it
 waited before (packing feature)

 Let me try to better explain with your example.
 Without packing the MMC layer will fetch 2 write requests and wait for the
 first write request completion before fetching another write request.
 During this time the read request could be inserted into the CFQ and since
 it has higher priority than the async write it will be dispatched in the
 next fetch. So, the result would be 2 write requests followed by one read
 request and the read would have to wait for completion of only 2 write
 requests.
 With packing, all the 5 write requests will be fetched in a row, and then
 the read will arrive and be dispatched in the next fetch. Then the read
 will have to wait for the completion of 5 write requests.

 Few more clarifications:
 Due to the plug list mechanism in the block layer the applications can
 aggregate several requests to be inserted into the scheduler before
 waking the MMC queue thread.
 This leads to a situation where there are several write requests in the
 CFQ queue when MMC starts to do the fetches.

 If the read was inserted while we are building the packed command then I
 agree that we should have seen less effect on the read performance.
 However, the write packing statistics show that in most of the 

Re: [PATCH v2 1/1] mmc: block: Add write packing control

[S, Venkatraman]

On Mon, Jul 23, 2012 at 5:13 PM,   wrote:
> On Wed, July 18, 2012 12:26 am, Chris Ball wrote:
>> Hi,  [removing Jens and the documentation list, since now we're
>> talking about the MMC side only]
>>
>> On Wed, Jul 18 2012, me...@codeaurora.org wrote:
>>> Is there anything else that holds this patch from being pushed to
> mmc-next?
>>
>> Yes, I'm still uncomfortable with the write packing patchsets for a
> couple of reasons, and I suspect that the sum of those reasons means that
> we should probably plan on holding off merging it until after 3.6.
>>
>> Here are the open issues; please correct any misunderstandings:
>>
>> With Seungwon's patchset ("Support packed write command"):
>>
>> * I still don't have a good set of representative benchmarks showing
>>   what kind of performance changes come with this patchset.  It seems
> like we've had a small amount of testing on one controller/eMMC part combo
> from Seungwon, and an entirely different test from Maya, and the results
> aren't documented fully anywhere to the level of describing what the
> hardware was, what the test was, and what the results were before and
> after the patchset.
>
> Currently, there is only one card vendor that supports packed commands.
> Following are our sequential write (LMDD) test results on 2 of our targets
> (in MB/s):
>No packingpacking
> Target 1 (SDR 50MHz) 15   25
> Target 2 (DDR 50MHz) 20   30
>
>>
>> With the reads-during-writes regression:
>>
>> * Venkat still has open questions about the nature of the read
>>   regression, and thinks we should understand it with blktrace before
> trying to fix it.  Maya has a theory about writes overwhelming reads, but
> Venkat doesn't understand why this would explain the observed
> bandwidth drop.
>
> The degradation of read due to writes is not a new behavior and exists
> also without the write packing feature (which only increases the
> degradation). Our investigation of this phenomenon led us to the
> Conclusion that a new scheduling policy should be used for mobile devices,
> but this is not related to the current discussion of the write packing
> feature.
>
> The write packing feature increases the degradation of read due to write
> since it allows the MMC to fetch many write requests in a row, instead of
> fetching only one at a time.  Therefore some of the read requests will
> have to wait for the completion of more write requests before they can be
> issued.

I am a bit puzzled by this claim. One thing I checked carefully when
reviewing write packing patches from SJeon was that the code didn't
plough through a mixed list of reads and writes and selected only
writes.
This section of the code in "mmc_blk_prep_packed_list()", from v8 patchset..

+   if (rq_data_dir(cur) != rq_data_dir(next)) {
+   put_back = 1;
+   break;
+   }


means that once a read is encountered in the middle of write packing,
the packing is stopped at that point and it is executed. Then the next
blk_fetch_request should get the next read and continue as before.

IOW, the ordering of reads and writes is _not_ altered when using
packed commands.
For example if there were 5 write requests, followed by 1 read,
followed by 5 more write requests in the request_queue, the first 5
writes will be executed as one "packed command", then the read will be
executed, and then the remaining 5 writes will be executed as one
"packed command". So the read does not have to wait any more than it
waited before (packing feature)

And I requested blktrace to confirm that this is indeed the behaviour.

Your rest of the arguments anyway depend on this assertion, so can you
please clarify this.

>
> To overcome this behavior, the solution would be to stop the write packing
> when a read request is fetched, and this is the algorithm suggested by the
> write packing control.
>
> Let's also keep in mind that lmdd benchmarking doesn't fully reflect the
> real life in which there are not many scenarios that cause massive read
> and write operations. In our user-common-scenarios tests we saw that in
> many cases the write packing decreases the read latency. It can happen in
> cases where the same amount of write requests is fetched with and without
> packing. In such a case the write packing decreases the transfer time of
> the write requests and causes the read request to wait for a shorter time.
>
>>
>> With Maya's patchset ("write packing control"):
>>
>> * Venkat thinks that HPI should be used, and the number-of-requests
>>   metric is too coarse, and it doesn't let you disable packing at the
> right time, and you're essentially implementing a new I/O scheduler inside
> the MMC subsystem without understanding the root cause for why that's
> necessary.
>
> According to our measurements the stop transmission (CMD12) + HPI is a
> heavy operation that may take up to several milliseconds. Therefore, a
> massive 

Re: [PATCH v2 1/1] mmc: block: Add write packing control

[merez]

On Wed, July 18, 2012 12:26 am, Chris Ball wrote:
> Hi,  [removing Jens and the documentation list, since now we're
> talking about the MMC side only]
>
> On Wed, Jul 18 2012, me...@codeaurora.org wrote:
>> Is there anything else that holds this patch from being pushed to
mmc-next?
>
> Yes, I'm still uncomfortable with the write packing patchsets for a
couple of reasons, and I suspect that the sum of those reasons means that
we should probably plan on holding off merging it until after 3.6.
>
> Here are the open issues; please correct any misunderstandings:
>
> With Seungwon's patchset ("Support packed write command"):
>
> * I still don't have a good set of representative benchmarks showing
>   what kind of performance changes come with this patchset.  It seems
like we've had a small amount of testing on one controller/eMMC part combo
from Seungwon, and an entirely different test from Maya, and the results
aren't documented fully anywhere to the level of describing what the
hardware was, what the test was, and what the results were before and
after the patchset.

Currently, there is only one card vendor that supports packed commands.
Following are our sequential write (LMDD) test results on 2 of our targets
(in MB/s):
   No packingpacking
Target 1 (SDR 50MHz) 15   25
Target 2 (DDR 50MHz) 20   30

>
> With the reads-during-writes regression:
>
> * Venkat still has open questions about the nature of the read
>   regression, and thinks we should understand it with blktrace before
trying to fix it.  Maya has a theory about writes overwhelming reads, but
Venkat doesn't understand why this would explain the observed
bandwidth drop.

The degradation of read due to writes is not a new behavior and exists
also without the write packing feature (which only increases the
degradation). Our investigation of this phenomenon led us to the
Conclusion that a new scheduling policy should be used for mobile devices,
but this is not related to the current discussion of the write packing
feature.

The write packing feature increases the degradation of read due to write
since it allows the MMC to fetch many write requests in a row, instead of
fetching only one at a time.  Therefore some of the read requests will
have to wait for the completion of more write requests before they can be
issued.

To overcome this behavior, the solution would be to stop the write packing
when a read request is fetched, and this is the algorithm suggested by the
write packing control.

Let's also keep in mind that lmdd benchmarking doesn't fully reflect the
real life in which there are not many scenarios that cause massive read
and write operations. In our user-common-scenarios tests we saw that in
many cases the write packing decreases the read latency. It can happen in
cases where the same amount of write requests is fetched with and without
packing. In such a case the write packing decreases the transfer time of
the write requests and causes the read request to wait for a shorter time.

>
> With Maya's patchset ("write packing control"):
>
> * Venkat thinks that HPI should be used, and the number-of-requests
>   metric is too coarse, and it doesn't let you disable packing at the
right time, and you're essentially implementing a new I/O scheduler inside
the MMC subsystem without understanding the root cause for why that's
necessary.

According to our measurements the stop transmission (CMD12) + HPI is a
heavy operation that may take up to several milliseconds. Therefore, a
massive usage of HPI can cause a degradation of performance.
In addition, it doesn’t provide a complete solution for read during write
since it doesn’t solve the problem of “what to do with the interrupted
write request remainder?”.  That is, a common interrupting read request
will usually be followed by another one. If we just continue to write the
interrupted write request remainder we will probably get another HPI due
to the second read request, so eventually we may end up with lots of HPIs
and write retries. A complete solution will be: stop the current write,
change packing mode to non-packing, serve the read request, push back the
write remainders to the block I/O scheduler and let him schedule them
again probably after the read burst ends (this requires block layer
support of course).

Regarding the packing control, there seem to be a confusion since the
number-of-requests is the trigger for *enabling* the packing (after it was
disabled), while a single read request disable packing. Therefore, the
packing is stopped at the right time.

The packing control doesn't add any scheduling policy to the MMC layer.
The write packing feature is the one changing the scheduling policy by
fetching many write requests in a row without a delay that allows read
requests to come in the middle.
By disabling the write packing, the write packing control returns the old
scheduling policy. It causes the MMC to fetch the requests one 

Re: [PATCH v2 1/1] mmc: block: Add write packing control

[merez]

On Wed, July 18, 2012 12:26 am, Chris Ball wrote:
 Hi,  [removing Jens and the documentation list, since now we're
 talking about the MMC side only]

 On Wed, Jul 18 2012, me...@codeaurora.org wrote:
 Is there anything else that holds this patch from being pushed to
mmc-next?

 Yes, I'm still uncomfortable with the write packing patchsets for a
couple of reasons, and I suspect that the sum of those reasons means that
we should probably plan on holding off merging it until after 3.6.

 Here are the open issues; please correct any misunderstandings:

 With Seungwon's patchset (Support packed write command):

 * I still don't have a good set of representative benchmarks showing
   what kind of performance changes come with this patchset.  It seems
like we've had a small amount of testing on one controller/eMMC part combo
from Seungwon, and an entirely different test from Maya, and the results
aren't documented fully anywhere to the level of describing what the
hardware was, what the test was, and what the results were before and
after the patchset.

Currently, there is only one card vendor that supports packed commands.
Following are our sequential write (LMDD) test results on 2 of our targets
(in MB/s):
   No packingpacking
Target 1 (SDR 50MHz) 15   25
Target 2 (DDR 50MHz) 20   30


 With the reads-during-writes regression:

 * Venkat still has open questions about the nature of the read
   regression, and thinks we should understand it with blktrace before
trying to fix it.  Maya has a theory about writes overwhelming reads, but
Venkat doesn't understand why this would explain the observed
bandwidth drop.

The degradation of read due to writes is not a new behavior and exists
also without the write packing feature (which only increases the
degradation). Our investigation of this phenomenon led us to the
Conclusion that a new scheduling policy should be used for mobile devices,
but this is not related to the current discussion of the write packing
feature.

The write packing feature increases the degradation of read due to write
since it allows the MMC to fetch many write requests in a row, instead of
fetching only one at a time.  Therefore some of the read requests will
have to wait for the completion of more write requests before they can be
issued.

To overcome this behavior, the solution would be to stop the write packing
when a read request is fetched, and this is the algorithm suggested by the
write packing control.

Let's also keep in mind that lmdd benchmarking doesn't fully reflect the
real life in which there are not many scenarios that cause massive read
and write operations. In our user-common-scenarios tests we saw that in
many cases the write packing decreases the read latency. It can happen in
cases where the same amount of write requests is fetched with and without
packing. In such a case the write packing decreases the transfer time of
the write requests and causes the read request to wait for a shorter time.


 With Maya's patchset (write packing control):

 * Venkat thinks that HPI should be used, and the number-of-requests
   metric is too coarse, and it doesn't let you disable packing at the
right time, and you're essentially implementing a new I/O scheduler inside
the MMC subsystem without understanding the root cause for why that's
necessary.

According to our measurements the stop transmission (CMD12) + HPI is a
heavy operation that may take up to several milliseconds. Therefore, a
massive usage of HPI can cause a degradation of performance.
In addition, it doesn’t provide a complete solution for read during write
since it doesn’t solve the problem of “what to do with the interrupted
write request remainder?”.  That is, a common interrupting read request
will usually be followed by another one. If we just continue to write the
interrupted write request remainder we will probably get another HPI due
to the second read request, so eventually we may end up with lots of HPIs
and write retries. A complete solution will be: stop the current write,
change packing mode to non-packing, serve the read request, push back the
write remainders to the block I/O scheduler and let him schedule them
again probably after the read burst ends (this requires block layer
support of course).

Regarding the packing control, there seem to be a confusion since the
number-of-requests is the trigger for *enabling* the packing (after it was
disabled), while a single read request disable packing. Therefore, the
packing is stopped at the right time.

The packing control doesn't add any scheduling policy to the MMC layer.
The write packing feature is the one changing the scheduling policy by
fetching many write requests in a row without a delay that allows read
requests to come in the middle.
By disabling the write packing, the write packing control returns the old
scheduling policy. It causes the MMC to fetch the requests one by one,
thus read requests 

Re: [PATCH v2 1/1] mmc: block: Add write packing control

[S, Venkatraman]

On Mon, Jul 23, 2012 at 5:13 PM,  me...@codeaurora.org wrote:
 On Wed, July 18, 2012 12:26 am, Chris Ball wrote:
 Hi,  [removing Jens and the documentation list, since now we're
 talking about the MMC side only]

 On Wed, Jul 18 2012, me...@codeaurora.org wrote:
 Is there anything else that holds this patch from being pushed to
 mmc-next?

 Yes, I'm still uncomfortable with the write packing patchsets for a
 couple of reasons, and I suspect that the sum of those reasons means that
 we should probably plan on holding off merging it until after 3.6.

 Here are the open issues; please correct any misunderstandings:

 With Seungwon's patchset (Support packed write command):

 * I still don't have a good set of representative benchmarks showing
   what kind of performance changes come with this patchset.  It seems
 like we've had a small amount of testing on one controller/eMMC part combo
 from Seungwon, and an entirely different test from Maya, and the results
 aren't documented fully anywhere to the level of describing what the
 hardware was, what the test was, and what the results were before and
 after the patchset.

 Currently, there is only one card vendor that supports packed commands.
 Following are our sequential write (LMDD) test results on 2 of our targets
 (in MB/s):
No packingpacking
 Target 1 (SDR 50MHz) 15   25
 Target 2 (DDR 50MHz) 20   30


 With the reads-during-writes regression:

 * Venkat still has open questions about the nature of the read
   regression, and thinks we should understand it with blktrace before
 trying to fix it.  Maya has a theory about writes overwhelming reads, but
 Venkat doesn't understand why this would explain the observed
 bandwidth drop.

 The degradation of read due to writes is not a new behavior and exists
 also without the write packing feature (which only increases the
 degradation). Our investigation of this phenomenon led us to the
 Conclusion that a new scheduling policy should be used for mobile devices,
 but this is not related to the current discussion of the write packing
 feature.

 The write packing feature increases the degradation of read due to write
 since it allows the MMC to fetch many write requests in a row, instead of
 fetching only one at a time.  Therefore some of the read requests will
 have to wait for the completion of more write requests before they can be
 issued.

I am a bit puzzled by this claim. One thing I checked carefully when
reviewing write packing patches from SJeon was that the code didn't
plough through a mixed list of reads and writes and selected only
writes.
This section of the code in mmc_blk_prep_packed_list(), from v8 patchset..
Quote
+   if (rq_data_dir(cur) != rq_data_dir(next)) {
+   put_back = 1;
+   break;
+   }
/Quote

means that once a read is encountered in the middle of write packing,
the packing is stopped at that point and it is executed. Then the next
blk_fetch_request should get the next read and continue as before.

IOW, the ordering of reads and writes is _not_ altered when using
packed commands.
For example if there were 5 write requests, followed by 1 read,
followed by 5 more write requests in the request_queue, the first 5
writes will be executed as one packed command, then the read will be
executed, and then the remaining 5 writes will be executed as one
packed command. So the read does not have to wait any more than it
waited before (packing feature)

And I requested blktrace to confirm that this is indeed the behaviour.

Your rest of the arguments anyway depend on this assertion, so can you
please clarify this.


 To overcome this behavior, the solution would be to stop the write packing
 when a read request is fetched, and this is the algorithm suggested by the
 write packing control.

 Let's also keep in mind that lmdd benchmarking doesn't fully reflect the
 real life in which there are not many scenarios that cause massive read
 and write operations. In our user-common-scenarios tests we saw that in
 many cases the write packing decreases the read latency. It can happen in
 cases where the same amount of write requests is fetched with and without
 packing. In such a case the write packing decreases the transfer time of
 the write requests and causes the read request to wait for a shorter time.


 With Maya's patchset (write packing control):

 * Venkat thinks that HPI should be used, and the number-of-requests
   metric is too coarse, and it doesn't let you disable packing at the
 right time, and you're essentially implementing a new I/O scheduler inside
 the MMC subsystem without understanding the root cause for why that's
 necessary.

 According to our measurements the stop transmission (CMD12) + HPI is a
 heavy operation that may take up to several milliseconds. Therefore, a
 massive usage of HPI can cause a degradation of performance.
 In addition, it doesn’t 

Re: [PATCH v2 1/1] mmc: block: Add write packing control

[Muthu Kumar]

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


Yeah... I guess it's upto Jens.


>
> - Chris.
> --
> Chris Ball  
> One Laptop Per Child
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/1] mmc: block: Add write packing control

[Chris Ball]

Hi,  [removing Jens and the documentation list, since now we're
talking about the MMC side only]

On Wed, Jul 18 2012, me...@codeaurora.org wrote:
> Is there anything else that holds this patch from being pushed to mmc-next?

Yes, I'm still uncomfortable with the write packing patchsets for a
couple of reasons, and I suspect that the sum of those reasons means
that we should probably plan on holding off merging it until after 3.6.

Here are the open issues; please correct any misunderstandings:

With Seungwon's patchset ("Support packed write command"):

* I still don't have a good set of representative benchmarks showing
  what kind of performance changes come with this patchset.  It seems
  like we've had a small amount of testing on one controller/eMMC part
  combo from Seungwon, and an entirely different test from Maya, and the
  results aren't documented fully anywhere to the level of describing
  what the hardware was, what the test was, and what the results were
  before and after the patchset.

With the reads-during-writes regression:

* Venkat still has open questions about the nature of the read
  regression, and thinks we should understand it with blktrace before
  trying to fix it.  Maya has a theory about writes overwhelming reads,
  but Venkat doesn't understand why this would explain the observed
  bandwidth drop.

With Maya's patchset ("write packing control"):

* Venkat thinks that HPI should be used, and the number-of-requests
  metric is too coarse, and it doesn't let you disable packing at the
  right time, and you're essentially implementing a new I/O scheduler
  inside the MMC subsystem without understanding the root cause for
  why that's necessary.

My sense is that there's no way we can solve all of these to
satisfaction in the next week (which is when the merge window will
open), but that by waiting a cycle we might come up with some good
answers.

What do other people think?  If you're excited about these patchsets,
now would be a fine time to come forward with your benchmarking results
and to help understand the reads-during-writes regression.

Thanks!

- Chris.
-- 
Chris Ball  
One Laptop Per Child
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/1] mmc: block: Add write packing control

[merez]

Hi Chris,

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

Thanks,
Maya
On Tue, July 17, 2012 3:50 pm, Chris Ball wrote:
> Hi Muthu,
>
> On Mon, Jul 16 2012, Muthu Kumar wrote:
>> On Sun, Jul 15, 2012 at 7:46 PM, Chris Ball  wrote:
>>> Hi,
>>>
>>> On Sun, Jul 15 2012, Muthu Kumar wrote:
> 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.
>>
>> 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  
> One Laptop Per Child
>


-- 
Sent by consultant of 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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/1] mmc: block: Add write packing control

[merez]

Hi Chris,

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

Thanks,
Maya
On Tue, July 17, 2012 3:50 pm, Chris Ball wrote:
 Hi Muthu,

 On Mon, Jul 16 2012, Muthu Kumar wrote:
 On Sun, Jul 15, 2012 at 7:46 PM, Chris Ball c...@laptop.org wrote:
 Hi,

 On Sun, Jul 15 2012, Muthu Kumar wrote:
 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.

 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   c...@laptop.org   http://printf.net/
 One Laptop Per Child



-- 
Sent by consultant of 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-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/1] mmc: block: Add write packing control

[Chris Ball]

Hi,  [removing Jens and the documentation list, since now we're
talking about the MMC side only]

On Wed, Jul 18 2012, me...@codeaurora.org wrote:
 Is there anything else that holds this patch from being pushed to mmc-next?

Yes, I'm still uncomfortable with the write packing patchsets for a
couple of reasons, and I suspect that the sum of those reasons means
that we should probably plan on holding off merging it until after 3.6.

Here are the open issues; please correct any misunderstandings:

With Seungwon's patchset (Support packed write command):

* I still don't have a good set of representative benchmarks showing
  what kind of performance changes come with this patchset.  It seems
  like we've had a small amount of testing on one controller/eMMC part
  combo from Seungwon, and an entirely different test from Maya, and the
  results aren't documented fully anywhere to the level of describing
  what the hardware was, what the test was, and what the results were
  before and after the patchset.

With the reads-during-writes regression:

* Venkat still has open questions about the nature of the read
  regression, and thinks we should understand it with blktrace before
  trying to fix it.  Maya has a theory about writes overwhelming reads,
  but Venkat doesn't understand why this would explain the observed
  bandwidth drop.

With Maya's patchset (write packing control):

* Venkat thinks that HPI should be used, and the number-of-requests
  metric is too coarse, and it doesn't let you disable packing at the
  right time, and you're essentially implementing a new I/O scheduler
  inside the MMC subsystem without understanding the root cause for
  why that's necessary.

My sense is that there's no way we can solve all of these to
satisfaction in the next week (which is when the merge window will
open), but that by waiting a cycle we might come up with some good
answers.

What do other people think?  If you're excited about these patchsets,
now would be a fine time to come forward with your benchmarking results
and to help understand the reads-during-writes regression.

Thanks!

- Chris.
-- 
Chris Ball   c...@laptop.org   http://printf.net/
One Laptop Per Child
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/1] mmc: block: Add write packing control

[Muthu Kumar]

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


Yeah... I guess it's upto Jens.



 - Chris.
 --
 Chris Ball   c...@laptop.org   http://printf.net/
 One Laptop Per Child
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/1] mmc: block: Add write packing control

[Chris Ball]

Hi Muthu,

On Mon, Jul 16 2012, Muthu Kumar wrote:
> On Sun, Jul 15, 2012 at 7:46 PM, Chris Ball  wrote:
>> Hi,
>>
>> On Sun, Jul 15 2012, Muthu Kumar wrote:
 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.
>
> 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  
One Laptop Per Child
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/1] mmc: block: Add write packing control

[Chris Ball]

Hi Muthu,

On Mon, Jul 16 2012, Muthu Kumar wrote:
 On Sun, Jul 15, 2012 at 7:46 PM, Chris Ball c...@laptop.org wrote:
 Hi,

 On Sun, Jul 15 2012, Muthu Kumar wrote:
 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.

 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   c...@laptop.org   http://printf.net/
One Laptop Per Child
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/1] mmc: block: Add write packing control

[S, Venkatraman]

On Mon, Jul 16, 2012 at 8:16 AM, Chris Ball  wrote:
> Hi,
>
> On Sun, Jul 15 2012, Muthu Kumar wrote:
>>> 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.
>

It's better to normalise this eventually. It would be better if we create a
duplicate sysfs entry within MMC, which is identical to the current
block layer attribute. Then schedule the block layer attribute to be
removed by, say, 3.9. [Add it to Documentation/feature-removal-schedule.txt]

Since it is a MMC specific attribute, generic tools wouldn't depend on it.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/1] mmc: block: Add write packing control

[Muthu Kumar]

On Sun, Jul 15, 2012 at 7:46 PM, Chris Ball  wrote:
> Hi,
>
> On Sun, Jul 15 2012, Muthu Kumar wrote:
>>> 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.

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

Jens,
What do you think?


Regards,
Muthu




>
> Thanks,
>
> - Chris.
> --
> Chris Ball  
> One Laptop Per Child
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/1] mmc: block: Add write packing control

[Muthu Kumar]

On Sun, Jul 15, 2012 at 7:46 PM, Chris Ball c...@laptop.org wrote:
 Hi,

 On Sun, Jul 15 2012, Muthu Kumar wrote:
 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.

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

Jens,
What do you think?


Regards,
Muthu





 Thanks,

 - Chris.
 --
 Chris Ball   c...@laptop.org   http://printf.net/
 One Laptop Per Child
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/1] mmc: block: Add write packing control

[S, Venkatraman]

On Mon, Jul 16, 2012 at 8:16 AM, Chris Ball c...@laptop.org wrote:
 Hi,

 On Sun, Jul 15 2012, Muthu Kumar wrote:
 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.


It's better to normalise this eventually. It would be better if we create a
duplicate sysfs entry within MMC, which is identical to the current
block layer attribute. Then schedule the block layer attribute to be
removed by, say, 3.9. [Add it to Documentation/feature-removal-schedule.txt]

Since it is a MMC specific attribute, generic tools wouldn't depend on it.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/1] mmc: block: Add write packing control

[Chris Ball]

Hi,

On Sun, Jul 15 2012, Muthu Kumar wrote:
>> 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  
One Laptop Per Child
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/1] mmc: block: Add write packing control

[Muthu Kumar]

Chris,

On Sat, Jul 14, 2012 at 12:12 PM, Chris Ball  wrote:
> Hi,
>
> On Wed, Jun 13 2012, Muthu Kumar wrote:
>> On Wed, Jun 13, 2012 at 12:52 PM,   wrote:
>>>
>>> On Mon, June 11, 2012 5:28 pm, Muthu Kumar wrote:
 On Mon, Jun 11, 2012 at 2:19 PM, Muthu Kumar  wrote:
> On Fri, Jun 1, 2012 at 11:55 AM, Maya Erez  wrote:
>> trigger
>> the packing can be configured via sysfs by writing the required value
>> to:
>> /sys/block//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?

 Just to be clear, please create a directory, say mmc, under
 /sys/block// 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  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.
>

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

Regards,
Muthu


> Thanks,
>
> - Chris.
> --
> Chris Ball  
> One Laptop Per Child
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/1] mmc: block: Add write packing control

[Muthu Kumar]

Chris,

On Sat, Jul 14, 2012 at 12:12 PM, Chris Ball c...@laptop.org wrote:
 Hi,

 On Wed, Jun 13 2012, Muthu Kumar wrote:
 On Wed, Jun 13, 2012 at 12:52 PM,  me...@codeaurora.org wrote:

 On Mon, June 11, 2012 5:28 pm, Muthu Kumar wrote:
 On Mon, Jun 11, 2012 at 2:19 PM, Muthu Kumar muthu.l...@gmail.com wrote:
 On Fri, Jun 1, 2012 at 11:55 AM, Maya Erez me...@codeaurora.org wrote:
 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?

 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?

 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.

Regards,
Muthu


 Thanks,

 - Chris.
 --
 Chris Ball   c...@laptop.org   http://printf.net/
 One Laptop Per Child
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/1] mmc: block: Add write packing control

[Chris Ball]

Hi,

On Sun, Jul 15 2012, Muthu Kumar wrote:
 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   c...@laptop.org   http://printf.net/
One Laptop Per Child
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/1] mmc: block: Add write packing control

[Chris Ball]

Hi,

On Wed, Jun 13 2012, Muthu Kumar wrote:
> On Wed, Jun 13, 2012 at 12:52 PM,   wrote:
>>
>> On Mon, June 11, 2012 5:28 pm, Muthu Kumar wrote:
>>> On Mon, Jun 11, 2012 at 2:19 PM, Muthu Kumar  wrote:
 On Fri, Jun 1, 2012 at 11:55 AM, Maya Erez  wrote:
> trigger
> the packing can be configured via sysfs by writing the required value
> to:
> /sys/block//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?
>>>
>>> Just to be clear, please create a directory, say mmc, under
>>> /sys/block// 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  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  
One Laptop Per Child
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/1] mmc: block: Add write packing control

[Chris Ball]

Hi,

On Wed, Jun 13 2012, Muthu Kumar wrote:
 On Wed, Jun 13, 2012 at 12:52 PM,  me...@codeaurora.org wrote:

 On Mon, June 11, 2012 5:28 pm, Muthu Kumar wrote:
 On Mon, Jun 11, 2012 at 2:19 PM, Muthu Kumar muthu.l...@gmail.com wrote:
 On Fri, Jun 1, 2012 at 11:55 AM, Maya Erez me...@codeaurora.org wrote:
 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?

 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?

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   c...@laptop.org   http://printf.net/
One Laptop Per Child
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/