| Message ID | 20250403211937.2225615-13-bvanassche@acm.org |
|---|---|
| State | New |
| Headers | show |
| Series | Optimize the hot path in the UFS driver | expand |
On Thu, 2025-04-03 at 14:17 -0700, Bart Van Assche wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > > > Replace a tag loop with blk_mq_tagset_busy_iter(). This patch > prepares > for removing the hba->lrb[] array. > > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/ufs/core/ufshcd.c | 74 ++++++++++++++++++++++--------------- > -- > 1 file changed, 42 insertions(+), 32 deletions(-) > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index 4b5734bbb12b..a5faf5af462e 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -5651,6 +5651,44 @@ static int ufshcd_poll(struct Scsi_Host > *shost, unsigned int queue_num) > return completed_reqs != 0; > } > > +static bool ufshcd_mcq_force_compl_one(struct request *rq, void > *priv) > +{ > + struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq); > + struct scsi_device *sdev = rq->q->queuedata; > + struct Scsi_Host *shost = sdev->host; > + struct ufs_hba *hba = shost_priv(shost); > + struct ufshcd_lrb *lrbp = &hba->lrb[rq->tag]; > + struct ufs_hw_queue *hwq = ufshcd_mcq_req_to_hwq(hba, rq); > + unsigned long flags; > + > + ufshcd_mcq_compl_all_cqes_lock(hba, hwq); > + /* > + * For those cmds of which the cqes are not present > + * in the cq, complete them explicitly. > + */ > + spin_lock_irqsave(&hwq->cq_lock, flags); > + if (cmd && !test_bit(SCMD_STATE_COMPLETE, &cmd->state)) { > + set_host_byte(cmd, DID_REQUEUE); > + ufshcd_release_scsi_cmd(hba, lrbp); > + scsi_done(cmd); > + } > + spin_unlock_irqrestore(&hwq->cq_lock, flags); > + > + return true; > +} > + > +static bool ufshcd_mcq_compl_one(struct request *rq, void *priv) > +{ > + struct scsi_device *sdev = rq->q->queuedata; > + struct Scsi_Host *shost = sdev->host; > + struct ufs_hba *hba = shost_priv(shost); > + struct ufs_hw_queue *hwq = ufshcd_mcq_req_to_hwq(hba, rq); > + > + ufshcd_mcq_poll_cqe_lock(hba, hwq); > + > + return true; > +} > + > /** > * ufshcd_mcq_compl_pending_transfer - MCQ mode function. It is > * invoked from the error handler context or > ufshcd_host_reset_and_restore() > @@ -5665,38 +5703,10 @@ static int ufshcd_poll(struct Scsi_Host > *shost, unsigned int queue_num) > static void ufshcd_mcq_compl_pending_transfer(struct ufs_hba *hba, > bool force_compl) > { > - struct ufs_hw_queue *hwq; > - struct ufshcd_lrb *lrbp; > - struct scsi_cmnd *cmd; > - unsigned long flags; > - int tag; > - > - for (tag = 0; tag < hba->nutrs; tag++) { > - lrbp = &hba->lrb[tag]; > - cmd = lrbp->cmd; > - if (!ufshcd_cmd_inflight(cmd) || > - test_bit(SCMD_STATE_COMPLETE, &cmd->state)) > - continue; > Hi Bart, Removing this check might cause racing issues? Leading to the possibility that the hwq in the subsequent function could be null? Thanks. Peter > - > - hwq = ufshcd_mcq_req_to_hwq(hba, > scsi_cmd_to_rq(cmd)); > - > - if (force_compl) { > - ufshcd_mcq_compl_all_cqes_lock(hba, hwq); > - /* > - * For those cmds of which the cqes are not > present > - * in the cq, complete them explicitly. > - */ > - spin_lock_irqsave(&hwq->cq_lock, flags); > - if (cmd && !test_bit(SCMD_STATE_COMPLETE, > &cmd->state)) { > - set_host_byte(cmd, DID_REQUEUE); > - ufshcd_release_scsi_cmd(hba, lrbp); > - scsi_done(cmd); > - } > - spin_unlock_irqrestore(&hwq->cq_lock, flags); > - } else { > - ufshcd_mcq_poll_cqe_lock(hba, hwq); > - } > - } > + blk_mq_tagset_busy_iter(&hba->host->tag_set, > + force_compl ? > ufshcd_mcq_force_compl_one : > + ufshcd_mcq_compl_one, > + NULL); > } > > /**
On 4/15/25 1:00 AM, Peter Wang (王信友) wrote: >> /** >> * ufshcd_mcq_compl_pending_transfer - MCQ mode function. It is >> * invoked from the error handler context or >> ufshcd_host_reset_and_restore() >> @@ -5665,38 +5703,10 @@ static int ufshcd_poll(struct Scsi_Host >> *shost, unsigned int queue_num) >> static void ufshcd_mcq_compl_pending_transfer(struct ufs_hba *hba, >> bool force_compl) >> { >> - struct ufs_hw_queue *hwq; >> - struct ufshcd_lrb *lrbp; >> - struct scsi_cmnd *cmd; >> - unsigned long flags; >> - int tag; >> - >> - for (tag = 0; tag < hba->nutrs; tag++) { >> - lrbp = &hba->lrb[tag]; >> - cmd = lrbp->cmd; >> - if (!ufshcd_cmd_inflight(cmd) || >> - test_bit(SCMD_STATE_COMPLETE, &cmd->state)) >> - continue; >> > > Removing this check might cause racing issues? > Leading to the possibility that the hwq in the subsequent function > could be null? Hi Peter, The ufshcd_cmd_inflight() check has not been removed. blk_mq_tagset_busy_iter() only calls the callback function that is passed as second argument for requests that have been started. The definition of ufshcd_cmd_inflight() is as follows: bool ufshcd_cmd_inflight(struct scsi_cmnd *cmd) { return cmd && blk_mq_rq_state(scsi_cmd_to_rq(cmd)) == MQ_RQ_IN_FLIGHT; } From the blk_mq_tagset_busy_iter() implementation: if (!(iter_data->flags & BT_TAG_ITER_STARTED) || blk_mq_request_started(rq)) ret = iter_data->fn(rq, iter_data->data); From blk-mq.h: static inline int blk_mq_request_started(struct request *rq) { return blk_mq_rq_state(rq) != MQ_RQ_IDLE; } In other words, if flag BT_TAG_ITER_STARTED has not been set, blk_mq_tagset_busy_iter() only calls its callback function for requests for which blk_mq_start_request() has been called and that have not yet been freed. Bart.
On Tue, 2025-04-15 at 13:22 -0700, Bart Van Assche wrote: > Hi Peter, > > The ufshcd_cmd_inflight() check has not been removed. > blk_mq_tagset_busy_iter() only calls the callback function that is > passed as second argument for requests that have been started. The > definition of ufshcd_cmd_inflight() is as follows: > > bool ufshcd_cmd_inflight(struct scsi_cmnd *cmd) > { > return cmd && > blk_mq_rq_state(scsi_cmd_to_rq(cmd)) == > MQ_RQ_IN_FLIGHT; > } > > From the blk_mq_tagset_busy_iter() implementation: > > if (!(iter_data->flags & BT_TAG_ITER_STARTED) || > blk_mq_request_started(rq)) > ret = iter_data->fn(rq, iter_data->data); > > From blk-mq.h: > > static inline int blk_mq_request_started(struct request *rq) > { > return blk_mq_rq_state(rq) != MQ_RQ_IDLE; > } > > In other words, if flag BT_TAG_ITER_STARTED has not been set, > blk_mq_tagset_busy_iter() only calls its callback function for > requests > for which blk_mq_start_request() has been called and that have not > yet > been freed. > > Bart. Hi Bart, If blk_mq_rq_state(rq) state is MQ_RQ_COMPLETE, the code before modification would skip this tag. But the code after modification will execute the function(fn). This should cause issues, right? Thanks. Peter
On 4/17/25 5:39 AM, Peter Wang (王信友) wrote: > If blk_mq_rq_state(rq) state is MQ_RQ_COMPLETE, the code > before modification would skip this tag. > But the code after modification will execute the function(fn). > This should cause issues, right? Hi Peter, My understanding is that the function modified by patch 12/24, ufshcd_mcq_compl_pending_transfer(), is only called by the UFS error handler. The UFS error handler is only activated after all pending requests have completed (MQ_RQ_IDLE) or timed out (MQ_RQ_STARTED). No requests should have the state MQ_RQ_COMPLETE when the error handler is activated. Thanks, Bart.
On Thu, 2025-04-17 at 14:33 -0700, Bart Van Assche wrote: > Hi Peter, > > My understanding is that the function modified by patch 12/24, > ufshcd_mcq_compl_pending_transfer(), is only called by the UFS error > handler. The UFS error handler is only activated after all pending > requests have completed (MQ_RQ_IDLE) or timed out (MQ_RQ_STARTED). > No requests should have the state MQ_RQ_COMPLETE when the error > handler > is activated. > > Thanks, > > Bart. Hi Bart, Yes, ufshcd_mcq_compl_pending_transfer() is only called form error handler. Consider a situation where a UIC error occurs and triggers the error handler, followed by receiving a request complete interrupt. At this point, the error handler might check this request that hasn't completed yet, and just as it is about to complete it, the tag has already been completed by the ISR. This request status may in MQ_RQ_COMPLETE. Thanks. Peter
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 4b5734bbb12b..a5faf5af462e 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -5651,6 +5651,44 @@ static int ufshcd_poll(struct Scsi_Host *shost, unsigned int queue_num) return completed_reqs != 0; } +static bool ufshcd_mcq_force_compl_one(struct request *rq, void *priv) +{ + struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq); + struct scsi_device *sdev = rq->q->queuedata; + struct Scsi_Host *shost = sdev->host; + struct ufs_hba *hba = shost_priv(shost); + struct ufshcd_lrb *lrbp = &hba->lrb[rq->tag]; + struct ufs_hw_queue *hwq = ufshcd_mcq_req_to_hwq(hba, rq); + unsigned long flags; + + ufshcd_mcq_compl_all_cqes_lock(hba, hwq); + /* + * For those cmds of which the cqes are not present + * in the cq, complete them explicitly. + */ + spin_lock_irqsave(&hwq->cq_lock, flags); + if (cmd && !test_bit(SCMD_STATE_COMPLETE, &cmd->state)) { + set_host_byte(cmd, DID_REQUEUE); + ufshcd_release_scsi_cmd(hba, lrbp); + scsi_done(cmd); + } + spin_unlock_irqrestore(&hwq->cq_lock, flags); + + return true; +} + +static bool ufshcd_mcq_compl_one(struct request *rq, void *priv) +{ + struct scsi_device *sdev = rq->q->queuedata; + struct Scsi_Host *shost = sdev->host; + struct ufs_hba *hba = shost_priv(shost); + struct ufs_hw_queue *hwq = ufshcd_mcq_req_to_hwq(hba, rq); + + ufshcd_mcq_poll_cqe_lock(hba, hwq); + + return true; +} + /** * ufshcd_mcq_compl_pending_transfer - MCQ mode function. It is * invoked from the error handler context or ufshcd_host_reset_and_restore() @@ -5665,38 +5703,10 @@ static int ufshcd_poll(struct Scsi_Host *shost, unsigned int queue_num) static void ufshcd_mcq_compl_pending_transfer(struct ufs_hba *hba, bool force_compl) { - struct ufs_hw_queue *hwq; - struct ufshcd_lrb *lrbp; - struct scsi_cmnd *cmd; - unsigned long flags; - int tag; - - for (tag = 0; tag < hba->nutrs; tag++) { - lrbp = &hba->lrb[tag]; - cmd = lrbp->cmd; - if (!ufshcd_cmd_inflight(cmd) || - test_bit(SCMD_STATE_COMPLETE, &cmd->state)) - continue; - - hwq = ufshcd_mcq_req_to_hwq(hba, scsi_cmd_to_rq(cmd)); - - if (force_compl) { - ufshcd_mcq_compl_all_cqes_lock(hba, hwq); - /* - * For those cmds of which the cqes are not present - * in the cq, complete them explicitly. - */ - spin_lock_irqsave(&hwq->cq_lock, flags); - if (cmd && !test_bit(SCMD_STATE_COMPLETE, &cmd->state)) { - set_host_byte(cmd, DID_REQUEUE); - ufshcd_release_scsi_cmd(hba, lrbp); - scsi_done(cmd); - } - spin_unlock_irqrestore(&hwq->cq_lock, flags); - } else { - ufshcd_mcq_poll_cqe_lock(hba, hwq); - } - } + blk_mq_tagset_busy_iter(&hba->host->tag_set, + force_compl ? ufshcd_mcq_force_compl_one : + ufshcd_mcq_compl_one, + NULL); } /**
Replace a tag loop with blk_mq_tagset_busy_iter(). This patch prepares for removing the hba->lrb[] array. Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/ufs/core/ufshcd.c | 74 ++++++++++++++++++++++----------------- 1 file changed, 42 insertions(+), 32 deletions(-)