linux-kernel - Re: [PATCH v1 1/1] nvme: complete directly for hctx with only one ctx mapping

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] [day] [month] [year] [list]

Message-ID: <f14b7a6d463c0e569e310e480e67e773a7679d2e.camel@mediatek.com>
Date:   Wed, 31 May 2023 01:32:42 +0000
From:   Ed Tsai (蔡宗軒) <Ed.Tsai@...iatek.com>
To:     "kbusch@...nel.org" <kbusch@...nel.org>,
        Powen Kao (高伯文) <Powen.Kao@...iatek.com>
CC:     Peter Wang (王信友) 
        <peter.wang@...iatek.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-mediatek@...ts.infradead.org" 
        <linux-mediatek@...ts.infradead.org>,
        CC Chou (周志杰) <cc.chou@...iatek.com>,
        Eddie Huang (黃智傑) 
        <eddie.huang@...iatek.com>,
        Alice Chao (趙珮均) 
        <Alice.Chao@...iatek.com>, "sagi@...mberg.me" <sagi@...mberg.me>,
        wsd_upstream <wsd_upstream@...iatek.com>,
        "hch@....de" <hch@....de>,
        "linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
        "axboe@...nel.dk" <axboe@...nel.dk>,
        Chun-Hung Wu (巫駿宏) 
        <Chun-hung.Wu@...iatek.com>,
        "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
        Naomi Chu (朱詠田) <Naomi.Chu@...iatek.com>,
        "linux-nvme@...ts.infradead.org" <linux-nvme@...ts.infradead.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        Stanley Chu (朱原陞) 
        <stanley.chu@...iatek.com>,
        "matthias.bgg@...il.com" <matthias.bgg@...il.com>,
        "angelogioacchino.delregno@...labora.com" 
        <angelogioacchino.delregno@...labora.com>
Subject: Re: [PATCH v1 1/1] nvme: complete directly for hctx with only one ctx
 mapping

On Tue, 2023-05-30 at 11:45 -0600, Keith Busch wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On Tue, May 30, 2023 at 10:41:19AM +0800, Po-Wen Kao wrote:
> > ---
> >  block/blk-mq.c           | 8 +++-----
> >  drivers/nvme/host/nvme.h | 4 ++++
> >  2 files changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/block/blk-mq.c b/block/blk-mq.c
> > index 1749f5890606..b60c78f5ad46 100644
> > --- a/block/blk-mq.c
> > +++ b/block/blk-mq.c
> > @@ -1181,12 +1181,10 @@ bool blk_mq_complete_request_remote(struct
> request *rq)
> >  WRITE_ONCE(rq->state, MQ_RQ_COMPLETE);
> >  
> >  /*
> > - * For request which hctx has only one ctx mapping,
> > - * or a polled request, always complete locally,
> > - * it's pointless to redirect the completion.
> > + * For a polled request, always complete locally, it's pointless
> > + * to redirect the completion.
> >   */
> > -if (rq->mq_hctx->nr_ctx == 1 ||
> > -rq->cmd_flags & REQ_POLLED)
> > +if (rq->cmd_flags & REQ_POLLED)
> >  return false;
> >  
> >  if (blk_mq_complete_need_ipi(rq)) {
> > diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> > index 7cf8e44d135e..acc9b1ce071d 100644
> > --- a/drivers/nvme/host/nvme.h
> > +++ b/drivers/nvme/host/nvme.h
> > @@ -702,6 +702,10 @@ static inline bool
> nvme_try_complete_req(struct request *req, __le16 status,
> >  nvme_should_fail(req);
> >  if (unlikely(blk_should_fake_timeout(req->q)))
> >  return true;
> > +if (likely(req->mq_hctx->nr_ctx == 1)) {
> > +WRITE_ONCE(req->state, MQ_RQ_COMPLETE);
> > +return false;
> > +}
> 
> I don't think we want low level drivers directly messing with blk-mq
> request state.
> 
> Is the early nr_ctx check optimisation really worth it? Would the
> following work for your use case?
> 
> ---
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index f6dad0886a2fa..a2d65bb127e29 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -1176,7 +1176,8 @@ bool blk_mq_complete_request_remote(struct
> request *rq)
>          * or a polled request, always complete locally,
>          * it's pointless to redirect the completion.
>          */
> -       if (rq->mq_hctx->nr_ctx == 1 ||
> +       if ((rq->mq_hctx->nr_ctx == 1 &&
> +            rq->mq_ctx->cpu == raw_smp_processor_id()) ||
>                 rq->cmd_flags & REQ_POLLED)
>                 return false;
> --

Sorry, I missed for this part.
It looks good to me and I will update later.