| Message ID | 20230222143443.69599-1-p.raghav@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: shift with PAGE_SHIFT instead of dividing with PAGE_SIZE | expand |
On 2/22/23 06:34, Pankaj Raghav wrote: > No functional change. Division will be costly, especially in the hot > path (page_is_mergeable() and bio_copy_user_iov()) Although the change looks fine to me, is there any compiler for which this patch makes a difference? I would expect that a compiler performs this optimization even without this patch. Thanks, Bart.
On 2023-02-22 20:57, Bart Van Assche wrote: > On 2/22/23 06:34, Pankaj Raghav wrote: >> No functional change. Division will be costly, especially in the hot >> path (page_is_mergeable() and bio_copy_user_iov()) > > Although the change looks fine to me, is there any compiler for which this > patch makes a difference? I would expect that a compiler performs this > optimization even without this patch. > I didn't notice any for x86_64. But I was thinking this also as a way to maintain consistency across block code where we do a shift with PAGE_SHIFT instead of dividing with PAGE_SIZE. > Thanks, > > Bart. >
On 2/22/23 9:25 AM, Pankaj Raghav wrote: > > On 2023-02-22 20:57, Bart Van Assche wrote: >> On 2/22/23 06:34, Pankaj Raghav wrote: >>> No functional change. Division will be costly, especially in the hot >>> path (page_is_mergeable() and bio_copy_user_iov()) >> >> Although the change looks fine to me, is there any compiler for which this >> patch makes a difference? I would expect that a compiler performs this >> optimization even without this patch. >> > > I didn't notice any for x86_64. But I was thinking this also as a way to > maintain consistency across block code where we do a shift with PAGE_SHIFT > instead of dividing with PAGE_SIZE. It won't make a difference on any architecture, it'd be a pretty awful compiler that didn't turn a division by a constant power-of-2 into a shift.
On 2/22/2023 6:34 AM, Pankaj Raghav wrote: > No functional change. Division will be costly, especially in the hot > path (page_is_mergeable() and bio_copy_user_iov()) > > Signed-off-by: Pankaj Raghav <p.raghav@samsung.com> without any quantitative data provided in the commit log on different architectures it is hard to ratify this patch. -ck
diff --git a/block/bio.c b/block/bio.c index 2e421c0dad13..2dc248e03ec2 100644 --- a/block/bio.c +++ b/block/bio.c @@ -922,7 +922,8 @@ static inline bool page_is_mergeable(const struct bio_vec *bv, return true; else if (IS_ENABLED(CONFIG_KMSAN)) return false; - return (bv->bv_page + bv_end / PAGE_SIZE) == (page + off / PAGE_SIZE); + return (bv->bv_page + (bv_end >> PAGE_SHIFT)) == + (page + (off >> PAGE_SHIFT)); } /** diff --git a/block/blk-map.c b/block/blk-map.c index 9137d16cecdc..22a0b65cafce 100644 --- a/block/blk-map.c +++ b/block/blk-map.c @@ -160,7 +160,7 @@ static int bio_copy_user_iov(struct request *rq, struct rq_map_data *map_data, if (map_data) { nr_pages = 1U << map_data->page_order; - i = map_data->offset / PAGE_SIZE; + i = map_data->offset >> PAGE_SHIFT; } while (len) { unsigned int bytes = PAGE_SIZE; diff --git a/block/ioctl.c b/block/ioctl.c index 9c5f637ff153..afb435adce71 100644 --- a/block/ioctl.c +++ b/block/ioctl.c @@ -521,7 +521,7 @@ static int blkdev_common_ioctl(struct block_device *bdev, fmode_t mode, case BLKFRASET: if(!capable(CAP_SYS_ADMIN)) return -EACCES; - bdev->bd_disk->bdi->ra_pages = (arg * 512) / PAGE_SIZE; + bdev->bd_disk->bdi->ra_pages = arg >> PAGE_SECTORS_SHIFT; return 0; case BLKRRPART: if (!capable(CAP_SYS_ADMIN))
No functional change. Division will be costly, especially in the hot path (page_is_mergeable() and bio_copy_user_iov()) Signed-off-by: Pankaj Raghav <p.raghav@samsung.com> --- block/bio.c | 3 ++- block/blk-map.c | 2 +- block/ioctl.c | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-)