linux-kernel - Re: [PATCH] mmc:mmc-hsq:use fifo to dispatch mmc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CA+Da2qxP2gvUc2=n5xW7_YEcgevGpDhqbcVFWVbF0c6DqXDXHA@mail.gmail.com>
Date:   Fri, 18 Nov 2022 19:43:19 +0800
From:   Wenchao Chen <wenchao.chen666@...il.com>
To:     Michael Wu <michael@...winnertech.com>
Cc:     ulf.hansson@...aro.org, wenchao.chen@...soc.com,
        linux-mmc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mmc:mmc-hsq:use fifo to dispatch mmc_request

On Fri, Nov 18, 2022 at 1:52 PM Michael Wu <michael@...winnertech.com> wrote:
>
> Current next_tag selection will cause a large delay in some requests and
> destroy the scheduling results of the block scheduling layer. Because the
> issued mrq tags cannot ensure that each time is sequential, especially when
> the IO load is heavy. In the fio performance test, we found that 4k random
> read data was sent to mmc_hsq to start calling request_atomic It takes
> nearly 200ms to process the request, while mmc_hsq has processed thousands
> of other requests. So we use fifo here to ensure the first in, first out
> feature of the request and avoid adding additional delay to the request.
>

Hi Michael
Is the test device an eMMC?
Could you share the fio test command if you want?
Can you provide more logs?

> Signed-off-by: Michael Wu <michael@...winnertech.com>
> ---
>  drivers/mmc/host/mmc_hsq.c | 40 ++++++++++++++------------------------
>  drivers/mmc/host/mmc_hsq.h |  5 +++++
>  2 files changed, 20 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/mmc/host/mmc_hsq.c b/drivers/mmc/host/mmc_hsq.c
> index 9d35453e7371..d2a1a96ed5bd 100644
> --- a/drivers/mmc/host/mmc_hsq.c
> +++ b/drivers/mmc/host/mmc_hsq.c
> @@ -13,9 +13,6 @@
>
>  #include "mmc_hsq.h"
>
> -#define HSQ_NUM_SLOTS  64
> -#define HSQ_INVALID_TAG        HSQ_NUM_SLOTS
> -
>  static void mmc_hsq_retry_handler(struct work_struct *work)
>  {
>         struct mmc_hsq *hsq = container_of(work, struct mmc_hsq, retry_work);
> @@ -73,7 +70,6 @@ static void mmc_hsq_pump_requests(struct mmc_hsq *hsq)
>
>  static void mmc_hsq_update_next_tag(struct mmc_hsq *hsq, int remains)
>  {
> -       struct hsq_slot *slot;
>         int tag;
>
>         /*
> @@ -82,29 +78,12 @@ static void mmc_hsq_update_next_tag(struct mmc_hsq *hsq, int remains)
>          */
>         if (!remains) {
>                 hsq->next_tag = HSQ_INVALID_TAG;
> +               hsq->tag_tail = HSQ_INVALID_TAG;
>                 return;
>         }
>
> -       /*
> -        * Increasing the next tag and check if the corresponding request is
> -        * available, if yes, then we found a candidate request.
> -        */
> -       if (++hsq->next_tag != HSQ_INVALID_TAG) {
> -               slot = &hsq->slot[hsq->next_tag];
> -               if (slot->mrq)
> -                       return;
> -       }
> -
> -       /* Othersie we should iterate all slots to find a available tag. */
> -       for (tag = 0; tag < HSQ_NUM_SLOTS; tag++) {
> -               slot = &hsq->slot[tag];
> -               if (slot->mrq)
> -                       break;
> -       }
> -
> -       if (tag == HSQ_NUM_SLOTS)
> -               tag = HSQ_INVALID_TAG;
> -
> +       tag = hsq->tag_slot[hsq->next_tag];
> +       hsq->tag_slot[hsq->next_tag] = HSQ_INVALID_TAG;
>         hsq->next_tag = tag;
>  }
>
> @@ -233,8 +212,14 @@ static int mmc_hsq_request(struct mmc_host *mmc, struct mmc_request *mrq)
>          * Set the next tag as current request tag if no available
>          * next tag.
>          */
> -       if (hsq->next_tag == HSQ_INVALID_TAG)
> +       if (hsq->next_tag == HSQ_INVALID_TAG) {
>                 hsq->next_tag = tag;
> +               hsq->tag_tail = tag;
> +               hsq->tag_slot[hsq->tag_tail] = HSQ_INVALID_TAG;
> +       } else {
> +               hsq->tag_slot[hsq->tag_tail] = tag;
> +               hsq->tag_tail = tag;
> +       }
>
>         hsq->qcnt++;
>
> @@ -339,8 +324,10 @@ static const struct mmc_cqe_ops mmc_hsq_ops = {
>
>  int mmc_hsq_init(struct mmc_hsq *hsq, struct mmc_host *mmc)
>  {
> +       int i;
>         hsq->num_slots = HSQ_NUM_SLOTS;
>         hsq->next_tag = HSQ_INVALID_TAG;
> +       hsq->tag_tail = HSQ_INVALID_TAG;
>
>         hsq->slot = devm_kcalloc(mmc_dev(mmc), hsq->num_slots,
>                                  sizeof(struct hsq_slot), GFP_KERNEL);
> @@ -351,6 +338,9 @@ int mmc_hsq_init(struct mmc_hsq *hsq, struct mmc_host *mmc)
>         hsq->mmc->cqe_private = hsq;
>         mmc->cqe_ops = &mmc_hsq_ops;
>
> +       for (i = 0; i < HSQ_NUM_SLOTS; i++)
> +               hsq->tag_slot[i] = HSQ_INVALID_TAG;
> +
>         INIT_WORK(&hsq->retry_work, mmc_hsq_retry_handler);
>         spin_lock_init(&hsq->lock);
>         init_waitqueue_head(&hsq->wait_queue);
> diff --git a/drivers/mmc/host/mmc_hsq.h b/drivers/mmc/host/mmc_hsq.h
> index ffdd9cd172c3..a783366285a9 100644
> --- a/drivers/mmc/host/mmc_hsq.h
> +++ b/drivers/mmc/host/mmc_hsq.h
> @@ -2,6 +2,9 @@
>  #ifndef LINUX_MMC_HSQ_H
>  #define LINUX_MMC_HSQ_H
>
> +#define HSQ_NUM_SLOTS  64
> +#define HSQ_INVALID_TAG        HSQ_NUM_SLOTS
> +
>  struct hsq_slot {
>         struct mmc_request *mrq;
>  };
> @@ -17,6 +20,8 @@ struct mmc_hsq {
>         int next_tag;
>         int num_slots;
>         int qcnt;
> +       int tag_tail;
> +       int tag_slot[HSQ_NUM_SLOTS];
>
>         bool enabled;
>         bool waiting_for_idle;
> --
> 2.29.0
>