Date:   Wed, 15 Nov 2017 15:07:56 +0200
From:   Adrian Hunter <adrian.hunter@...el.com>
To:     Ulf Hansson <ulf.hansson@...aro.org>,
        Linus Walleij <linus.walleij@...aro.org>
Cc:     linux-mmc <linux-mmc@...r.kernel.org>,
        linux-block <linux-block@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Bough Chen <haibo.chen@....com>,
        Alex Lemberg <alex.lemberg@...disk.com>,
        Mateusz Nowak <mateusz.nowak@...el.com>,
        Yuliy Izrailov <Yuliy.Izrailov@...disk.com>,
        Jaehoon Chung <jh80.chung@...sung.com>,
        Dong Aisheng <dongas86@...il.com>,
        Das Asutosh <asutoshd@...eaurora.org>,
        Zhangfei Gao <zhangfei.gao@...il.com>,
        Sahitya Tummala <stummala@...eaurora.org>,
        Harjani Ritesh <riteshh@...eaurora.org>,
        Venu Byravarasu <vbyravarasu@...dia.com>,
        Shawn Lin <shawn.lin@...k-chips.com>,
        Christoph Hellwig <hch@....de>
Subject: Re: [PATCH V13 03/10] mmc: block: Add blk-mq support

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.

Powered by blists - more mailing lists