| Message ID | 1458030067-46850-1-git-send-email-hare@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Looks fine,
Reviewed-by: Christoph Hellwig <hch@lst.de>
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Hannes Reinecke <hare@suse.de> writes: > Use standard cpumap_print_to_pagebuf() instead of > hand-crafted function when displaying the cpu map. > > Signed-off-by: Hannes Reinecke <hare@suse.com> > --- > block/blk-mq-sysfs.c | 15 +-------------- > 1 file changed, 1 insertion(+), 14 deletions(-) > > diff --git a/block/blk-mq-sysfs.c b/block/blk-mq-sysfs.c > index 1cf1878..f817561 100644 > --- a/block/blk-mq-sysfs.c > +++ b/block/blk-mq-sysfs.c > @@ -231,20 +231,7 @@ static ssize_t blk_mq_hw_sysfs_active_show(struct blk_mq_hw_ctx *hctx, char *pag > > static ssize_t blk_mq_hw_sysfs_cpus_show(struct blk_mq_hw_ctx *hctx, char *page) > { > - unsigned int i, first = 1; > - ssize_t ret = 0; > - > - for_each_cpu(i, hctx->cpumask) { > - if (first) > - ret += sprintf(ret + page, "%u", i); > - else > - ret += sprintf(ret + page, ", %u", i); > - > - first = 0; > - } > - > - ret += sprintf(ret + page, "\n"); > - return ret; > + return cpumap_print_to_pagebuf(true, page, hctx->cpumask); > } You've just changed the output format[1] (and you didn't mention that in your patch description). I don't think this change is worth potentially breaking existing scripts. Cheers, Jeff [1] Before: # cat /sys/block/rssda/mq/0/cpu_list 0, 1, 2, 3 After: # cat /sys/block/rssda/mq/0/cpu_list 0-3 -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 15, 2016 at 11:07:58AM -0400, Jeff Moyer wrote: > You've just changed the output format[1] (and you didn't mention that in > your patch description). I don't think this change is worth potentially > breaking existing scripts. Oops. I followed the code into lib/bitmap.c and up until then this wasn't obvious. Yes, let's keep it as-is. -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/15/2016 04:07 PM, Jeff Moyer wrote: > Hannes Reinecke <hare@suse.de> writes: > >> Use standard cpumap_print_to_pagebuf() instead of >> hand-crafted function when displaying the cpu map. >> >> Signed-off-by: Hannes Reinecke <hare@suse.com> >> --- >> block/blk-mq-sysfs.c | 15 +-------------- >> 1 file changed, 1 insertion(+), 14 deletions(-) >> >> diff --git a/block/blk-mq-sysfs.c b/block/blk-mq-sysfs.c >> index 1cf1878..f817561 100644 >> --- a/block/blk-mq-sysfs.c >> +++ b/block/blk-mq-sysfs.c >> @@ -231,20 +231,7 @@ static ssize_t blk_mq_hw_sysfs_active_show(struct blk_mq_hw_ctx *hctx, char *pag >> >> static ssize_t blk_mq_hw_sysfs_cpus_show(struct blk_mq_hw_ctx *hctx, char *page) >> { >> - unsigned int i, first = 1; >> - ssize_t ret = 0; >> - >> - for_each_cpu(i, hctx->cpumask) { >> - if (first) >> - ret += sprintf(ret + page, "%u", i); >> - else >> - ret += sprintf(ret + page, ", %u", i); >> - >> - first = 0; >> - } >> - >> - ret += sprintf(ret + page, "\n"); >> - return ret; >> + return cpumap_print_to_pagebuf(true, page, hctx->cpumask); >> } > > You've just changed the output format[1] (and you didn't mention that in > your patch description). I don't think this change is worth potentially > breaking existing scripts. > > Cheers, > Jeff > > [1] > Before: > > # cat /sys/block/rssda/mq/0/cpu_list > 0, 1, 2, 3 > > After: > > # cat /sys/block/rssda/mq/0/cpu_list > 0-3 > But this was precisely the point. The smp_affinity is using the above function, making it _really_ hard to check if both settings match. When using the same function the output is identical, and we can just punt the output into smp_affinity. Cheers, Hannes
Hannes Reinecke <hare@suse.de> writes: >> You've just changed the output format[1] (and you didn't mention that in >> your patch description). I don't think this change is worth potentially >> breaking existing scripts. > But this was precisely the point. > > The smp_affinity is using the above function, making it _really_ > hard to check if both settings match. > When using the same function the output is identical, and we can > just punt the output into smp_affinity. This is already out in the wild, Hannes. I agree that your way is better, I just don't feel comfortable changing the format after it's been released. Cheers, Jeff -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: linux-block-owner@vger.kernel.org [mailto:linux-block- > owner@vger.kernel.org] On Behalf Of Jeff Moyer > Sent: Tuesday, March 15, 2016 10:08 AM > To: Hannes Reinecke <hare@suse.de> > Cc: Jens Axboe <axboe@kernel.dk>; Christoph Hellwig <hch@lst.de>; linux- > block@vger.kernel.org; Hannes Reinecke <hare@suse.com> > Subject: Re: [PATCH] blk-mq: use cpumap_print_to_pagebuf() > > Hannes Reinecke <hare@suse.de> writes: ... > You've just changed the output format[1] (and you didn't mention that in > your patch description). I don't think this change is worth potentially > breaking existing scripts. > > Cheers, > Jeff > > [1] > Before: > > # cat /sys/block/rssda/mq/0/cpu_list > 0, 1, 2, 3 > > After: > > # cat /sys/block/rssda/mq/0/cpu_list > 0-3 How about creating another file called cpulist that is allowed to use dash for ranges? None of the blk-mq sysfs files are defined in Documentation/ yet, so any script parsing them is just guessing what they might contain. --- Robert Elliott, HPE Persistent Memory -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/15/2016 01:21 AM, Hannes Reinecke wrote: > Use standard cpumap_print_to_pagebuf() instead of > hand-crafted function when displaying the cpu map. You need a big disclaimer when you are changing the output format. Either you didn't realize this, so it wasn't really tested, or you did realize this and was being sneaky. Neither of those two options are great.
diff --git a/block/blk-mq-sysfs.c b/block/blk-mq-sysfs.c index 1cf1878..f817561 100644 --- a/block/blk-mq-sysfs.c +++ b/block/blk-mq-sysfs.c @@ -231,20 +231,7 @@ static ssize_t blk_mq_hw_sysfs_active_show(struct blk_mq_hw_ctx *hctx, char *pag static ssize_t blk_mq_hw_sysfs_cpus_show(struct blk_mq_hw_ctx *hctx, char *page) { - unsigned int i, first = 1; - ssize_t ret = 0; - - for_each_cpu(i, hctx->cpumask) { - if (first) - ret += sprintf(ret + page, "%u", i); - else - ret += sprintf(ret + page, ", %u", i); - - first = 0; - } - - ret += sprintf(ret + page, "\n"); - return ret; + return cpumap_print_to_pagebuf(true, page, hctx->cpumask); } static struct blk_mq_ctx_sysfs_entry blk_mq_sysfs_dispatched = {
Use standard cpumap_print_to_pagebuf() instead of hand-crafted function when displaying the cpu map. Signed-off-by: Hannes Reinecke <hare@suse.com> --- block/blk-mq-sysfs.c | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-)