On 21 November 2016 at 13:44, Adrian Hunter <adrian.hunter@xxxxxxxxx> wrote:
> On 21/11/16 13:11, Arnd Bergmann wrote:
>> On Monday, November 21, 2016 11:08:57 AM CET Linus Walleij wrote:
>>> I've had it with this code now.
>>>
>>> The packed command support is a complex hurdle in the MMC/SD block
>>> layer, around 500+ lines of code which was introduced in 2013 in
>>> commits
>>>
>>> ce39f9d17c14 ("mmc: support packed write command for eMMC4.5 devices")
>>> abd9ac144947 ("mmc: add packed command feature of eMMC4.5")
>>>
>>> ...and since then it has been rotting. The original author of the
>>> code has disappeared from the community and the mail address is
>>> bouncing.
>>>
>>> For the code to be exercised the host must flag that it supports
>>> packed commands, so in mmc_blk_prep_packed_list() which is called for
>>> every single request, the following construction appears:
>>>
>>> u8 max_packed_rw = 0;
>>>
>>> if ((rq_data_dir(cur) == WRITE) &&
>>> mmc_host_packed_wr(card->host))
>>> max_packed_rw = card->ext_csd.max_packed_writes;
>>>
>>> if (max_packed_rw == 0)
>>> goto no_packed;
>>>
>>> This has the following logical deductions:
>>>
>>> - Only WRITE commands can really be packed, so the solution is
>>> only half-done: we support packed WRITE but not packed READ.
>>> The packed command support has not been finalized by supporting
>>> reads in three years!
>>
>> Packed reads don't make a lot of sense, there is very little
>> for an MMC to optimize in reads that it can't already do without
>> the packing. For writes, packing makes could be an important
>> performance optimization, if the eMMC supports it.
>>
>> I've added Luca Porzio and Alex Lemberg to Cc. I think they
>> are subscribed to the list already, but it would be good to
>> get some estimate from them too about how common the packed
>> write support is on existing hardware from their respective
>> employers before we kill it off.
>>
>> If none of Samsung/Micron/Sandisk are currently shipping
>> devices that support eMMC-4.5 packed commands but don't
>> support the eMMC-5.1 command queuing (which IIRC is a more
>> general way to achieve the same), we probably don't need to
>> worry about it too much.
>>
>>> - mmc_host_packed_wr() is just a static inline that checks
>>> host->caps2 & MMC_CAP2_PACKED_WR. The problem with this is
>>> that NO upstream host sets this capability flag! No driver
>>> in the kernel is using it, and we can't test it. Packed
>>> command may be supported in out-of-tree code, but I doubt
>>> it. I doubt that the code is even working anymore due to
>>> other refactorings in the MMC block layer, who would
>>> notice if patches affecting it broke packed commands?
>>> No one.
>>
>> This is a very good indication that it's not really used.
>> It would be useful to also check out the Android AOSP
>> kernel tree to see if it's in there, if anything important
>> supports packed commands, it's probably in there.
>>
>>> - There is no Device Tree binding or code to mark a host as
>>> supporting packed read or write commands, just this flag
>>> in caps2, so for sure there are not any DT systems using
>>> it either.
>>>
>>> It has other problems as well: mmc_blk_prep_packed_list() is
>>> speculatively picking requests out of the request queue with
>>> blk_fetch_request() making the MMC/SD stack harder to convert
>>> to the multiqueue block layer. By this we get rid of an
>>> obstacle.
>
> SDHCIv4 has a feature (ADMA3) which is slightly similar to packed
> commands but it does not require card support.
>
> Why is it a problem for blk-mq to allow some extra requests to
> pick from for packing?
>
In blk-mq, requests don't get picked from the queue, but instead those
gets "pushed" to the block device driver.
First, to support packing, it seems like we would need to specify a
queue-depth > 1, more or less lie to blk-mq layer about the device's
capability. Okay, we can do that. But..
I also believe, the implementation would become really complex, as you
would need to "hold" the first write request, while waiting for a
second to arrive. Then for how long shall you hold it? And then what
if you are unlucky so the next is read request, thus you can't pack
them. The solution will be suboptimal, won't it?
Perhaps Linus can add something, or confirm?
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
