diff mbox

[4/6] SQUSHME: pmem: Micro cleaning

Message ID 551AA0AD.4000604@plexistor.com (mailing list archive)
State New, archived
Headers show

Commit Message

Boaz Harrosh March 31, 2015, 1:27 p.m. UTC
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(-)

Comments

Dan Williams March 31, 2015, 3:17 p.m. UTC | #1
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Boaz Harrosh March 31, 2015, 3:24 p.m. UTC | #2
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

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Williams March 31, 2015, 3:30 p.m. UTC | #3
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/
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Boaz Harrosh March 31, 2015, 3:43 p.m. UTC | #4
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

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matthew Wilcox March 31, 2015, 7:40 p.m. UTC | #5
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;
 }