On 15/11/17 12:55, Ulf Hansson wrote:
> Linus, Adrian,
>
> Apologize for sidetracking the discussion, just wanted to add some
> minor comments.
>
> [...]
>
>>
>>>> But what I think is nice in doing it around
>>>> each request is that since mmc_put_card() calls mmc_release_host()
>>>> contains this:
>>>>
>>>> if (--host->claim_cnt) { (...)
>>>>
>>>> So there is a counter inside the claim/release host functions
>>>> and now there is another counter keeping track if the in-flight
>>>> requests as well and it gives me the feeling we are maintaining
>>>> two counters when we only need one.
>>>
>>> The gets / puts also get runtime pm for the card. It is a lot a messing
>>> around for the sake of a quick check for the number of requests inflight -
>>> which is needed anyway for CQE.
>
> Actually the get / puts for runtime PM of the card is already done
> taking the host->claim_cnt into account.
We do pm_runtime_get_sync(&card->dev) always i.e.
void mmc_get_card(struct mmc_card *card, struct mmc_ctx *ctx)
{
pm_runtime_get_sync(&card->dev);
__mmc_claim_host(card->host, ctx, NULL);
}
>
> In other words, the additional in-flight counter does not provide an
> additional improvement in this regards.
>
> For that reason, perhaps the in-flight counter should be added in the
> CQE patch instead, because it seems like it is there it really
> belongs?
>
> [...]
>
>>
>> OK I guess I'm more forging ahead with such things. But we could
>> at least enable it by default so whoever checks out and builds
>> and tests linux-next with their MMC/SD controllers will then be
>> testing this for us in the next kernel cycle.
>>
>>>> But I think I would prefer that if a big slew of new code is
>>>> introduced it needs to be put to wide use and any bugs
>>>> smoked out during the -rc phase, and we are now
>>>> hiding it behind a Kconfig option so it's unlikely to see testing
>>>> until that option is turned on, and that is not good.
>>>
>>> And if you find a big problem in rc7, then what do you do? At least with
>>> the config option, the revert is trivial.
>>
>> I see your point. I guess it is up to Ulf how he feels about
>> trust wrt the code. If a problem appears at -rc7 it's indeed
>> nice if we can leave the code in-tree and work on it.
>
> Well, it's not an easy decision, simply because it moves the code in
> an even worse situation maintenance wise. So, if you guys just
> suddenly have to move on to do something else, then it becomes my
> problem to work out. :-)
>
> However, as I trust both of you, and that you seems to agree on the
> path forward, I am fine keeping the old code for while.
>
> Although, please make sure the Kconfig option starts out by being
> enabled by default, so we can get some test coverage of the mq path.
Ok
>
> Of course, then we need to work on the card removal problem asap, and
> hopefully we manage to fix them. If not, or other strange errors
> happens, we would need to change the default value to 'n' of the
> Kconfig.
>
> [...]
>
>>>> Hm! What you are saying sounds correct, and we really need to
>>>> consider the multi-CPU aspects of this, maybe not now. I am
>>>> happy as long as we have equal performance as before and
>>>> maintainable code.
>>>
>>> Well I saw 3% drop in sequential read performance, improving to 1% when an
>>> unbound workqueue was used.
>
> Can you please make this improvement as a standalone patch on top of
> the mq patch?
Unfortunately it was just a hack to the blk-mq core - the block layer does
not have an unbound workqueue. I have not had time to consider a proper
fix. It will have to wait, but I agree 3% is not enough to delay going forward.
>
> I think that would be good, because it points out some generic
> problems with mq, which we then, at least short term, tries to address
> locally in MMC.
>
> [...]
>
>>
>>>> If you just make a series also deleteing the old block layer
>>>> I will test it so it doesn't break anything and then you can
>>>> probably expect a big Acked-by on the whole shebang.
>>>
>>> I will add patches for that - let's see what happens.
>
> Yes, please. However, I will as stated, be withholding that change for
> a while, until we fixed any showstoppers in the mq path.
