| Message ID | 20180809194149.15285-9-bart.vanassche@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blk-mq: Implement runtime power management | expand |
Hi Bart On 08/10/2018 03:41 AM, Bart Van Assche wrote: > +void blk_pm_runtime_exit(struct request_queue *q) > +{ > + if (!q->dev) > + return; > + > + pm_runtime_get_sync(q->dev); > + q->dev = NULL; > +} > +EXPORT_SYMBOL(blk_pm_runtime_exit); > + > /** > * blk_pre_runtime_suspend - Pre runtime suspend check > * @q: the queue of the device > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c > index 69ab459abb98..5537762dfcfd 100644 > --- a/drivers/scsi/sd.c > +++ b/drivers/scsi/sd.c > @@ -3420,12 +3420,11 @@ static int sd_probe(struct device *dev) > **/ > static int sd_remove(struct device *dev) > { > - struct scsi_disk *sdkp; > - dev_t devt; > + struct scsi_disk *sdkp = dev_get_drvdata(dev); > + struct scsi_device *sdp = sdkp->device; > + dev_t devt = disk_devt(sdkp->disk); > > - sdkp = dev_get_drvdata(dev); > - devt = disk_devt(sdkp->disk); > - scsi_autopm_get_device(sdkp->device); > + blk_pm_runtime_exit(sdp->request_queue) Based on __scsi_device_remove, sd_remove will be invoked before blk_cleanup_queue. So it should be not safe that we set the q->dev to NULL here. We have following operations in followed patch: +static inline void blk_pm_mark_last_busy(struct request *rq) +{ + if (rq->q->dev && !(rq->rq_flags & RQF_PM)) + pm_runtime_mark_last_busy(rq->q->dev); +} Thanks Jianchao
On Fri, 2018-08-10 at 10:39 +0800, jianchao.wang wrote: > On 08/10/2018 03:41 AM, Bart Van Assche wrote: > > +void blk_pm_runtime_exit(struct request_queue *q) > > +{ > > + if (!q->dev) > > + return; > > + > > + pm_runtime_get_sync(q->dev); > > + q->dev = NULL; > > +} > > +EXPORT_SYMBOL(blk_pm_runtime_exit); > > + > > /** > > * blk_pre_runtime_suspend - Pre runtime suspend check > > * @q: the queue of the device > > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c > > index 69ab459abb98..5537762dfcfd 100644 > > --- a/drivers/scsi/sd.c > > +++ b/drivers/scsi/sd.c > > @@ -3420,12 +3420,11 @@ static int sd_probe(struct device *dev) > > **/ > > static int sd_remove(struct device *dev) > > { > > - struct scsi_disk *sdkp; > > - dev_t devt; > > + struct scsi_disk *sdkp = dev_get_drvdata(dev); > > + struct scsi_device *sdp = sdkp->device; > > + dev_t devt = disk_devt(sdkp->disk); > > > > - sdkp = dev_get_drvdata(dev); > > - devt = disk_devt(sdkp->disk); > > - scsi_autopm_get_device(sdkp->device); > > + blk_pm_runtime_exit(sdp->request_queue) > > > Based on __scsi_device_remove, sd_remove will be invoked before blk_cleanup_queue. > So it should be not safe that we set the q->dev to NULL here. > > We have following operations in followed patch: > +static inline void blk_pm_mark_last_busy(struct request *rq) > +{ > + if (rq->q->dev && !(rq->rq_flags & RQF_PM)) > + pm_runtime_mark_last_busy(rq->q->dev); > +} Hello Jianchao, How about moving the blk_pm_runtime_exit() call into blk_cleanup_queue() at a point where it is sure that there are no requests in progress? That should avoid that blk_pm_mark_last_busy() races against blk_pm_runtime_exit(). Thanks, Bart.
On Fri, 2018-08-10 at 15:27 +0000, Bart Van Assche wrote: > On Fri, 2018-08-10 at 10:39 +0800, jianchao.wang wrote: > > On 08/10/2018 03:41 AM, Bart Van Assche wrote: > > > +void blk_pm_runtime_exit(struct request_queue *q) > > > +{ > > > + if (!q->dev) > > > + return; > > > + > > > + pm_runtime_get_sync(q->dev); > > > + q->dev = NULL; > > > +} > > > +EXPORT_SYMBOL(blk_pm_runtime_exit); > > > + > > > /** > > > * blk_pre_runtime_suspend - Pre runtime suspend check > > > * @q: the queue of the device > > > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c > > > index 69ab459abb98..5537762dfcfd 100644 > > > --- a/drivers/scsi/sd.c > > > +++ b/drivers/scsi/sd.c > > > @@ -3420,12 +3420,11 @@ static int sd_probe(struct device *dev) > > > **/ > > > static int sd_remove(struct device *dev) > > > { > > > - struct scsi_disk *sdkp; > > > - dev_t devt; > > > + struct scsi_disk *sdkp = dev_get_drvdata(dev); > > > + struct scsi_device *sdp = sdkp->device; > > > + dev_t devt = disk_devt(sdkp->disk); > > > > > > - sdkp = dev_get_drvdata(dev); > > > - devt = disk_devt(sdkp->disk); > > > - scsi_autopm_get_device(sdkp->device); > > > + blk_pm_runtime_exit(sdp->request_queue) > > > > > > Based on __scsi_device_remove, sd_remove will be invoked before blk_cleanup_queue. > > So it should be not safe that we set the q->dev to NULL here. > > > > We have following operations in followed patch: > > +static inline void blk_pm_mark_last_busy(struct request *rq) > > +{ > > + if (rq->q->dev && !(rq->rq_flags & RQF_PM)) > > + pm_runtime_mark_last_busy(rq->q->dev); > > +} > > How about moving the blk_pm_runtime_exit() call into blk_cleanup_queue() at > a point where it is sure that there are no requests in progress? That should > avoid that blk_pm_mark_last_busy() races against blk_pm_runtime_exit(). That wouldn't work because unbinding the SCSI ULD doesn't remove the request queue. How about changing blk_pm_runtime_exit() into something like the following? void blk_pm_runtime_exit(struct request_queue *q) { if (!q->dev) return; pm_runtime_get_sync(q->dev); blk_freeze_queue(q); q->dev = NULL; blk_unfreeze_queue(q); } Bart.
Hi Bart On 08/11/2018 12:17 AM, Bart Van Assche wrote: > void blk_pm_runtime_exit(struct request_queue *q) > { > if (!q->dev) > return; > > pm_runtime_get_sync(q->dev); > blk_freeze_queue(q); > q->dev = NULL; > blk_unfreeze_queue(q); > } I'm afraid this will not work. In following patch: @@ -972,6 +972,8 @@ int blk_queue_enter(struct request_queue *q, blk_mq_req_flags_t flags) if (success) return 0; + blk_pm_request_resume(q); + if (flags & BLK_MQ_REQ_NOWAIT) return -EBUSY; We could still invoke blk_pm_request_resume even if the queue is frozen. And we have blk_pm_request_resume later as following: +static inline void blk_pm_request_resume(struct request_queue *q) +{ + if (q->dev && (q->rpm_status == RPM_SUSPENDED || + q->rpm_status == RPM_SUSPENDING)) + pm_request_resume(q->dev); +} Thanks Jianchao
On Mon, 2018-08-13 at 17:24 +0800, jianchao.wang wrote:
> I'm afraid this will not work.
Since this patch fixes a bug that nobody has reported so far and since no
later patches rely on this patch, I will leave it out.
Thanks,
Bart.
diff --git a/block/blk-pm.c b/block/blk-pm.c index 9b636960d285..bf8532da952d 100644 --- a/block/blk-pm.c +++ b/block/blk-pm.c @@ -40,6 +40,24 @@ void blk_pm_runtime_init(struct request_queue *q, struct device *dev) } EXPORT_SYMBOL(blk_pm_runtime_init); +/** + * blk_pm_runtime_exit - runtime PM exit routine + * @q: the queue of the device + * + * This function should be called from the device_driver.remove() callback + * function to avoid that further calls to runtime power management functions + * occur. + */ +void blk_pm_runtime_exit(struct request_queue *q) +{ + if (!q->dev) + return; + + pm_runtime_get_sync(q->dev); + q->dev = NULL; +} +EXPORT_SYMBOL(blk_pm_runtime_exit); + /** * blk_pre_runtime_suspend - Pre runtime suspend check * @q: the queue of the device diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index 69ab459abb98..5537762dfcfd 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -3420,12 +3420,11 @@ static int sd_probe(struct device *dev) **/ static int sd_remove(struct device *dev) { - struct scsi_disk *sdkp; - dev_t devt; + struct scsi_disk *sdkp = dev_get_drvdata(dev); + struct scsi_device *sdp = sdkp->device; + dev_t devt = disk_devt(sdkp->disk); - sdkp = dev_get_drvdata(dev); - devt = disk_devt(sdkp->disk); - scsi_autopm_get_device(sdkp->device); + blk_pm_runtime_exit(sdp->request_queue); async_synchronize_full_domain(&scsi_sd_pm_domain); async_synchronize_full_domain(&scsi_sd_probe_domain); diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c index de4413e66eca..476987f7ed48 100644 --- a/drivers/scsi/sr.c +++ b/drivers/scsi/sr.c @@ -1002,8 +1002,9 @@ static void sr_kref_release(struct kref *kref) static int sr_remove(struct device *dev) { struct scsi_cd *cd = dev_get_drvdata(dev); + struct scsi_device *sdev = cd->device; - scsi_autopm_get_device(cd->device); + blk_pm_runtime_exit(sdev->request_queue); del_gendisk(cd->disk); dev_set_drvdata(dev, NULL); diff --git a/include/linux/blk-pm.h b/include/linux/blk-pm.h index b80c65aba249..6d654f41acbf 100644 --- a/include/linux/blk-pm.h +++ b/include/linux/blk-pm.h @@ -11,6 +11,7 @@ struct request_queue; */ #ifdef CONFIG_PM extern void blk_pm_runtime_init(struct request_queue *q, struct device *dev); +extern void blk_pm_runtime_exit(struct request_queue *q); extern int blk_pre_runtime_suspend(struct request_queue *q); extern void blk_post_runtime_suspend(struct request_queue *q, int err); extern void blk_pre_runtime_resume(struct request_queue *q); @@ -19,6 +20,7 @@ extern void blk_set_runtime_active(struct request_queue *q); #else static inline void blk_pm_runtime_init(struct request_queue *q, struct device *dev) {} +static inline void blk_pm_runtime_exit(struct request_queue *q) {} #endif #endif /* _BLK_PM_H_ */
Since it is possible to unbind a SCSI ULD and since unbinding removes the association between a request queue and struct device, the q->dev pointer has to be reset during unbind. Hence introduce a function in the block layer that clears request_queue.dev. Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com> Cc: Martin K. Petersen <martin.petersen@oracle.com> Cc: Christoph Hellwig <hch@lst.de> Cc: Ming Lei <ming.lei@redhat.com> Cc: Jianchao Wang <jianchao.w.wang@oracle.com> Cc: Hannes Reinecke <hare@suse.com> Cc: Johannes Thumshirn <jthumshirn@suse.de> Cc: Alan Stern <stern@rowland.harvard.edu> --- block/blk-pm.c | 18 ++++++++++++++++++ drivers/scsi/sd.c | 9 ++++----- drivers/scsi/sr.c | 3 ++- include/linux/blk-pm.h | 2 ++ 4 files changed, 26 insertions(+), 6 deletions(-)