| Message ID | 551AA0AD.4000604@plexistor.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Mar 31, 2015 at 6:27 AM, Boaz Harrosh <boaz@plexistor.com> wrote: > > Some error checks had unlikely some did not. Put unlikely > on all error handling paths. > (I like unlikely for error paths specially for readability) "unlikely()" is not a readability hint, it's specifically for branches that profiling shows adding it makes a difference. Just delete them all until profiling show they make a difference. They certainly don't make a difference in the slow paths.
On 03/31/2015 06:17 PM, Dan Williams wrote: > On Tue, Mar 31, 2015 at 6:27 AM, Boaz Harrosh <boaz@plexistor.com> wrote: >> >> Some error checks had unlikely some did not. Put unlikely >> on all error handling paths. >> (I like unlikely for error paths specially for readability) > > "unlikely()" is not a readability hint, it's specifically for branches > that profiling shows adding it makes a difference. Just delete them > all until profiling show they make a difference. They certainly don't > make a difference in the slow paths. > Why? So we do not fill up the branch predictor with useless predictions that will never matter. What is so bad with that. It may be cold path but added up all over it will show eventually. I do not see what is the harm of telling the compiler. "never store any prediction for this branch" So since it can never (ever) harm any one or anything, and at the mass if everywhere it was done this way it could actually help, then sure it can be a readability thing. Since no harm done, right? I still like it Thanks Boaz
On Tue, Mar 31, 2015 at 8:24 AM, Boaz Harrosh <boaz@plexistor.com> wrote: > On 03/31/2015 06:17 PM, Dan Williams wrote: >> On Tue, Mar 31, 2015 at 6:27 AM, Boaz Harrosh <boaz@plexistor.com> wrote: >>> >>> Some error checks had unlikely some did not. Put unlikely >>> on all error handling paths. >>> (I like unlikely for error paths specially for readability) >> >> "unlikely()" is not a readability hint, it's specifically for branches >> that profiling shows adding it makes a difference. Just delete them >> all until profiling show they make a difference. They certainly don't >> make a difference in the slow paths. >> > > Why? Because the compiler and cpu already does a decent job, and if you get the frequency wrong it can hurt performance [1]. It's pre-mature optimization to sprinkle them around, especially in slow paths. [1]: https://lwn.net/Articles/420019/
On 03/31/2015 06:30 PM, Dan Williams wrote: > On Tue, Mar 31, 2015 at 8:24 AM, Boaz Harrosh <boaz@plexistor.com> wrote: >> On 03/31/2015 06:17 PM, Dan Williams wrote: >>> On Tue, Mar 31, 2015 at 6:27 AM, Boaz Harrosh <boaz@plexistor.com> wrote: >>>> >>>> Some error checks had unlikely some did not. Put unlikely >>>> on all error handling paths. >>>> (I like unlikely for error paths specially for readability) >>> >>> "unlikely()" is not a readability hint, it's specifically for branches >>> that profiling shows adding it makes a difference. Just delete them >>> all until profiling show they make a difference. They certainly don't >>> make a difference in the slow paths. >>> >> >> Why? > > Because the compiler and cpu already does a decent job, and if you get > the frequency wrong it can hurt performance [1]. > > It's pre-mature optimization to sprinkle them around, especially in slow paths. > > [1]: https://lwn.net/Articles/420019/ > Sigh! It looks like a holy war. Again all that was said at above thread was about statistical prediction yes-or-no. And I agree with all the use cases. But not here. This is not an optimization this is the *error path*. What I'm saying is: "No compiler nor CPU, even if 99% of the time this branch is taken I still consider it cold. Because it is the error case and I do not care for it" And no I did not get it wrong. All these places are "error paths" that I do not care for. If any of these places are dependent on some input or code variable then yes let the smarts do it. But never in the "error path". That said. the patch is up for grabs. I like it ... Thanks Boaz
On Tue, Mar 31, 2015 at 06:43:40PM +0300, Boaz Harrosh wrote: > But not here. This is not an optimization this is the *error path*. > What I'm saying is: > "No compiler nor CPU, even if 99% of the time this branch is taken > I still consider it cold. Because it is the error case and > I do not care for it" GCC already understands that "if (foo) goto FORWARD_LABEL" should be predicted unlikely by default. All you're doing is cluttering the source code.
diff --git a/drivers/block/pmem.c b/drivers/block/pmem.c index 6a45fd5..209a410 100644 --- a/drivers/block/pmem.c +++ b/drivers/block/pmem.c @@ -76,7 +76,7 @@ static void pmem_make_request(struct request_queue *q, struct bio *bio) struct bvec_iter iter; int err = 0; - if (bio_end_sector(bio) > get_capacity(bdev->bd_disk)) { + if (unlikely(bio_end_sector(bio) > get_capacity(bdev->bd_disk))) { err = -EIO; goto out; } @@ -86,9 +86,7 @@ static void pmem_make_request(struct request_queue *q, struct bio *bio) goto out; } - rw = bio_rw(bio); - if (rw == READA) - rw = READ; + rw = bio_data_dir(bio); sector = bio->bi_iter.bi_sector; bio_for_each_segment(bvec, bio, iter) { @@ -124,7 +122,7 @@ static long pmem_direct_access(struct block_device *bdev, sector_t sector, struct pmem_device *pmem = bdev->bd_disk->private_data; size_t offset = sector << 9; - if (!pmem) + if (unlikely(!pmem)) return -ENODEV; *kaddr = pmem->virt_addr + offset; @@ -149,7 +147,7 @@ static int pmem_mapmem(struct pmem_device *pmem) res_mem = request_mem_region_exclusive(pmem->phys_addr, pmem->size, "pmem"); - if (!res_mem) { + if (unlikely(!res_mem)) { pr_warn("pmem: request_mem_region_exclusive phys=0x%llx size=0x%zx failed\n", pmem->phys_addr, pmem->size); return -EINVAL; @@ -192,7 +190,7 @@ static int pmem_probe(struct platform_device *pdev) return -ENXIO; res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - if (!res) + if (unlikely(!res)) return -ENXIO; pmem = kzalloc(sizeof(*pmem), GFP_KERNEL); @@ -273,11 +271,11 @@ static int __init pmem_init(void) int error; pmem_major = register_blkdev(0, "pmem"); - if (pmem_major < 0) + if (unlikely(pmem_major < 0)) return pmem_major; error = platform_driver_register(&pmem_driver); - if (error) + if (unlikely(error)) unregister_blkdev(pmem_major, "pmem"); return error; }
Some error checks had unlikely some did not. Put unlikely on all error handling paths. (I like unlikely for error paths specially for readability) Also use bio_data_dir() to extract away the READA flag Signed-off-by: Boaz Harrosh <boaz@plexistor.com> --- drivers/block/pmem.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)