[PATCH] mmc: core: remove issue_fn indirect function call

[PATCH] mmc: core: remove issue_fn indirect function call

[Grant Grundler]

struct mmc_queue defines issue_fn as an indirect function call.
issue_fn field only gets set to mmc_blk_issue_rq and only gets
invoked immediately after calling blk_fetch_request().
Don't bother with indirect function call - it's pointless and just
obfuscates the code.

Signed-off-by: Grant Grundler <grundler@chromium.org>
---
 drivers/mmc/card/block.c | 1 -
 drivers/mmc/card/queue.c | 2 +-
 drivers/mmc/card/queue.h | 1 -
 3 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 1a3163f..b2cdd10 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -2072,7 +2072,6 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card,
 	if (ret)
 		goto err_putdisk;
 
-	md->queue.issue_fn = mmc_blk_issue_rq;
 	md->queue.data = md;
 
 	md->disk->major	= MMC_BLOCK_MAJOR;
diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
index fa9632e..6f9adc5 100644
--- a/drivers/mmc/card/queue.c
+++ b/drivers/mmc/card/queue.c
@@ -67,7 +67,7 @@ static int mmc_queue_thread(void *d)
 		if (req || mq->mqrq_prev->req) {
 			set_current_state(TASK_RUNNING);
 			cmd_flags = req ? req->cmd_flags : 0;
-			mq->issue_fn(mq, req);
+			mmc_blk_issue_rq(mq, req);
 			if (mq->flags & MMC_QUEUE_NEW_REQUEST) {
 				mq->flags &= ~MMC_QUEUE_NEW_REQUEST;
 				continue; /* fetch again */
diff --git a/drivers/mmc/card/queue.h b/drivers/mmc/card/queue.h
index 5752d50..35380015 100644
--- a/drivers/mmc/card/queue.h
+++ b/drivers/mmc/card/queue.h
@@ -51,7 +51,6 @@ struct mmc_queue {
 #define MMC_QUEUE_SUSPENDED	(1 << 0)
 #define MMC_QUEUE_NEW_REQUEST	(1 << 1)
 
-	int			(*issue_fn)(struct mmc_queue *, struct request *);
 	void			*data;
 	struct request_queue	*queue;
 	struct mmc_queue_req	mqrq[2];
-- 
1.8.4

Re: [PATCH] mmc: core: remove issue_fn indirect function call

[Ulf Hansson]

On 19 September 2013 19:20, Grant Grundler <grundler@chromium.org> wrote:
> struct mmc_queue defines issue_fn as an indirect function call.
> issue_fn field only gets set to mmc_blk_issue_rq and only gets
> invoked immediately after calling blk_fetch_request().
> Don't bother with indirect function call - it's pointless and just
> obfuscates the code.
>
> Signed-off-by: Grant Grundler <grundler@chromium.org>

Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

> ---
>  drivers/mmc/card/block.c | 1 -
>  drivers/mmc/card/queue.c | 2 +-
>  drivers/mmc/card/queue.h | 1 -
>  3 files changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index 1a3163f..b2cdd10 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -2072,7 +2072,6 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card,
>         if (ret)
>                 goto err_putdisk;
>
> -       md->queue.issue_fn = mmc_blk_issue_rq;
>         md->queue.data = md;
>
>         md->disk->major = MMC_BLOCK_MAJOR;
> diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
> index fa9632e..6f9adc5 100644
> --- a/drivers/mmc/card/queue.c
> +++ b/drivers/mmc/card/queue.c
> @@ -67,7 +67,7 @@ static int mmc_queue_thread(void *d)
>                 if (req || mq->mqrq_prev->req) {
>                         set_current_state(TASK_RUNNING);
>                         cmd_flags = req ? req->cmd_flags : 0;
> -                       mq->issue_fn(mq, req);
> +                       mmc_blk_issue_rq(mq, req);
>                         if (mq->flags & MMC_QUEUE_NEW_REQUEST) {
>                                 mq->flags &= ~MMC_QUEUE_NEW_REQUEST;
>                                 continue; /* fetch again */
> diff --git a/drivers/mmc/card/queue.h b/drivers/mmc/card/queue.h
> index 5752d50..35380015 100644
> --- a/drivers/mmc/card/queue.h
> +++ b/drivers/mmc/card/queue.h
> @@ -51,7 +51,6 @@ struct mmc_queue {
>  #define MMC_QUEUE_SUSPENDED    (1 << 0)
>  #define MMC_QUEUE_NEW_REQUEST  (1 << 1)
>
> -       int                     (*issue_fn)(struct mmc_queue *, struct request *);
>         void                    *data;
>         struct request_queue    *queue;
>         struct mmc_queue_req    mqrq[2];
> --
> 1.8.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@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] mmc: core: remove issue_fn indirect function call

[Chris Ball]

Hi,

On Fri, Sep 20 2013, Ulf Hansson wrote:
> On 19 September 2013 19:20, Grant Grundler <grundler@chromium.org> wrote:
>> struct mmc_queue defines issue_fn as an indirect function call.
>> issue_fn field only gets set to mmc_blk_issue_rq and only gets
>> invoked immediately after calling blk_fetch_request().
>> Don't bother with indirect function call - it's pointless and just
>> obfuscates the code.
>>
>> Signed-off-by: Grant Grundler <grundler@chromium.org>
>
> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

Thanks, pushed to mmc-next for 3.13.

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>

Re: [PATCH] mmc: core: remove issue_fn indirect function call

[Chris Ball]

Hi,

On Wed, Sep 25 2013, Chris Ball wrote:
> Hi,
>
> On Fri, Sep 20 2013, Ulf Hansson wrote:
>> On 19 September 2013 19:20, Grant Grundler <grundler@chromium.org> wrote:
>>> struct mmc_queue defines issue_fn as an indirect function call.
>>> issue_fn field only gets set to mmc_blk_issue_rq and only gets
>>> invoked immediately after calling blk_fetch_request().
>>> Don't bother with indirect function call - it's pointless and just
>>> obfuscates the code.
>>>
>>> Signed-off-by: Grant Grundler <grundler@chromium.org>
>>
>> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
>
> Thanks, pushed to mmc-next for 3.13.

Have dropped this, it's breaking my build:

/home/cjb/git/mmc/drivers/mmc/card/block.c:1955:12: warning: ‘mmc_blk_issue_rq’ defined but not used [-Wunused-function]
/home/cjb/git/mmc/drivers/mmc/card/queue.c: In function ‘mmc_queue_thread’:
/home/cjb/git/mmc/drivers/mmc/card/queue.c:70:4: error: implicit declaration of function ‘mmc_blk_issue_rq’ [-Werror=implicit-function-declaration]

Grant, please could you take a look and resubmit?

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>

Re: [PATCH] mmc: core: remove issue_fn indirect function call

[Grant Grundler]

On Wed, Sep 25, 2013 at 7:37 PM, Chris Ball <cjb@laptop.org> wrote:
> Hi,
>
> On Wed, Sep 25 2013, Chris Ball wrote:
>> Hi,
>>
>> On Fri, Sep 20 2013, Ulf Hansson wrote:
>>> On 19 September 2013 19:20, Grant Grundler <grundler@chromium.org> wrote:
>>>> struct mmc_queue defines issue_fn as an indirect function call.
>>>> issue_fn field only gets set to mmc_blk_issue_rq and only gets
>>>> invoked immediately after calling blk_fetch_request().
>>>> Don't bother with indirect function call - it's pointless and just
>>>> obfuscates the code.
>>>>
>>>> Signed-off-by: Grant Grundler <grundler@chromium.org>
>>>
>>> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
>>
>> Thanks, pushed to mmc-next for 3.13.
>
> Have dropped this, it's breaking my build:
>
> /home/cjb/git/mmc/drivers/mmc/card/block.c:1955:12: warning: ‘mmc_blk_issue_rq’ defined but not used [-Wunused-function]

The function is declared static. :(  Let me respin to remove the
static and add a function prototype to a header file. I just got lucky
when I built this in an earlier tree.

sorry about that...

grant

> /home/cjb/git/mmc/drivers/mmc/card/queue.c: In function ‘mmc_queue_thread’:
> /home/cjb/git/mmc/drivers/mmc/card/queue.c:70:4: error: implicit declaration of function ‘mmc_blk_issue_rq’ [-Werror=implicit-function-declaration]


>
> Grant, please could you take a look and resubmit?
>
> - Chris.
> --
> Chris Ball   <cjb@laptop.org>   <http://printf.net/>

Re: [PATCH] mmc: core: remove issue_fn indirect function call

[Grant Grundler]

On Thu, Sep 26, 2013 at 2:56 PM, Grant Grundler <grundler@chromium.org> wrote:
> On Wed, Sep 25, 2013 at 7:37 PM, Chris Ball <cjb@laptop.org> wrote:
>> Hi,
>>
>> On Wed, Sep 25 2013, Chris Ball wrote:
>>> Hi,
>>>
>>> On Fri, Sep 20 2013, Ulf Hansson wrote:
>>>> On 19 September 2013 19:20, Grant Grundler <grundler@chromium.org> wrote:
>>>>> struct mmc_queue defines issue_fn as an indirect function call.
>>>>> issue_fn field only gets set to mmc_blk_issue_rq and only gets
>>>>> invoked immediately after calling blk_fetch_request().
>>>>> Don't bother with indirect function call - it's pointless and just
>>>>> obfuscates the code.
>>>>>
>>>>> Signed-off-by: Grant Grundler <grundler@chromium.org>
>>>>
>>>> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>
>>> Thanks, pushed to mmc-next for 3.13.
>>
>> Have dropped this, it's breaking my build:
>>
>> /home/cjb/git/mmc/drivers/mmc/card/block.c:1955:12: warning: ‘mmc_blk_issue_rq’ defined but not used [-Wunused-function]
>
> The function is declared static. :(  Let me respin to remove the
> static and add a function prototype to a header file.

block.o and queue.o are linked together into one .ko all the time:
obj-$(CONFIG_MMC_BLOCK)         += mmc_block.o
mmc_block-objs                  := block.o queue.o

Two ways to handle this: I can
1) add a local function prototype of mmc_blk_issue_rq() to queue.c
2) move mmc_init_queue() and mmc_queue_thread() from queue.c to block.c

(2) actually makes sense since both functions are block IO specific.

Thoughts? Preference? Other ideas?

thanks,
grant

ps. It's more obvious now that the return value from
mmc_blk_issue_rq() is getting ignored. *sigh*

Re: [PATCH] mmc: core: remove issue_fn indirect function call

[Ulf Hansson]

On 27 September 2013 00:16, Grant Grundler <grundler@chromium.org> wrote:
> On Thu, Sep 26, 2013 at 2:56 PM, Grant Grundler <grundler@chromium.org> wrote:
>> On Wed, Sep 25, 2013 at 7:37 PM, Chris Ball <cjb@laptop.org> wrote:
>>> Hi,
>>>
>>> On Wed, Sep 25 2013, Chris Ball wrote:
>>>> Hi,
>>>>
>>>> On Fri, Sep 20 2013, Ulf Hansson wrote:
>>>>> On 19 September 2013 19:20, Grant Grundler <grundler@chromium.org> wrote:
>>>>>> struct mmc_queue defines issue_fn as an indirect function call.
>>>>>> issue_fn field only gets set to mmc_blk_issue_rq and only gets
>>>>>> invoked immediately after calling blk_fetch_request().
>>>>>> Don't bother with indirect function call - it's pointless and just
>>>>>> obfuscates the code.
>>>>>>
>>>>>> Signed-off-by: Grant Grundler <grundler@chromium.org>
>>>>>
>>>>> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>>
>>>> Thanks, pushed to mmc-next for 3.13.
>>>
>>> Have dropped this, it's breaking my build:
>>>
>>> /home/cjb/git/mmc/drivers/mmc/card/block.c:1955:12: warning: ‘mmc_blk_issue_rq’ defined but not used [-Wunused-function]
>>
>> The function is declared static. :(  Let me respin to remove the
>> static and add a function prototype to a header file.
>
> block.o and queue.o are linked together into one .ko all the time:
> obj-$(CONFIG_MMC_BLOCK)         += mmc_block.o
> mmc_block-objs                  := block.o queue.o
>
> Two ways to handle this: I can
> 1) add a local function prototype of mmc_blk_issue_rq() to queue.c
> 2) move mmc_init_queue() and mmc_queue_thread() from queue.c to block.c
>
> (2) actually makes sense since both functions are block IO specific.
>
> Thoughts? Preference? Other ideas?

Hi Grant,

Generally I am in favour of cleaning up messy code. But in this case
it now seems a bit overworked.

How about actually leaving it as is?

Kind regards
Ulf Hansson

>
> thanks,
> grant
>
> ps. It's more obvious now that the return value from
> mmc_blk_issue_rq() is getting ignored. *sigh*

Re: [PATCH] mmc: core: remove issue_fn indirect function call

[Grant Grundler]

[-- Attachment #1: Type: text/plain, Size: 1542 bytes --]

On Fri, Sep 27, 2013 at 2:04 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
...
>> Two ways to handle this: I can
>> 1) add a local function prototype of mmc_blk_issue_rq() to queue.c
>> 2) move mmc_init_queue() and mmc_queue_thread() from queue.c to block.c
>>
>> (2) actually makes sense since both functions are block IO specific.
>>
>> Thoughts? Preference? Other ideas?
>
> Hi Grant,
>
> Generally I am in favour of cleaning up messy code. But in this case
> it now seems a bit overworked.
>
> How about actually leaving it as is?

Hi Ulf,
Sure - I can leave it.

I'll point out that mmcqd uses about 1/2 a CPU core when busy driving
eMMC devices behind dw_mmc interface. That's pretty bad. I haven't
worked out exact "service demand" (avg CPU cycles to service one IO)
but that is the right metric to be looking at when evaluating design
changes.

My feeling is there too many layers in this subsystem. I count 6
layers of data structures when starting with struct mmc_blk_data. I've
attached my drawing (scanned pdf). "boxes" are host memory footprint.
Arrows are pointers to other host memory.

I thought the block layer provides sufficient IO request queueing and
can't justify why this code needs to manage it's own array (of two)
struct mmc_queue_req. Given how much faster CPUs have always been than
block storage, the difference in latency (nano or microseconds at
best) doesn't seem worth managing our own local queues - especially
since I'm convinced this code has SMP bugs that don't exist in the
block layer.

cheers,
grant

[-- Attachment #2: 20130927141112165.pdf --]
[-- Type: application/pdf, Size: 202478 bytes --]