Re: [Linux-nvdimm] [PATCH 4/6] SQUSHME: pmem: Micro cleaning
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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Linux-nvdimm] [PATCH 4/6] SQUSHME: pmem: Micro cleaning
On 03/31/2015 06:30 PM, Dan Williams wrote: > On Tue, Mar 31, 2015 at 8:24 AM, Boaz Harrosh wrote: >> On 03/31/2015 06:17 PM, Dan Williams wrote: >>> On Tue, Mar 31, 2015 at 6:27 AM, Boaz Harrosh 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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Linux-nvdimm] [PATCH 4/6] SQUSHME: pmem: Micro cleaning
On Tue, Mar 31, 2015 at 8:24 AM, Boaz Harrosh wrote: > On 03/31/2015 06:17 PM, Dan Williams wrote: >> On Tue, Mar 31, 2015 at 6:27 AM, Boaz Harrosh 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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Linux-nvdimm] [PATCH 4/6] SQUSHME: pmem: Micro cleaning
On 03/31/2015 06:17 PM, Dan Williams wrote: > On Tue, Mar 31, 2015 at 6:27 AM, Boaz Harrosh 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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Linux-nvdimm] [PATCH 4/6] SQUSHME: pmem: Micro cleaning
On Tue, Mar 31, 2015 at 6:27 AM, Boaz Harrosh 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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Linux-nvdimm] [PATCH 4/6] SQUSHME: pmem: Micro cleaning
On Tue, Mar 31, 2015 at 8:24 AM, Boaz Harrosh b...@plexistor.com wrote: On 03/31/2015 06:17 PM, Dan Williams wrote: On Tue, Mar 31, 2015 at 6:27 AM, Boaz Harrosh b...@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-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Linux-nvdimm] [PATCH 4/6] SQUSHME: pmem: Micro cleaning
On 03/31/2015 06:30 PM, Dan Williams wrote: On Tue, Mar 31, 2015 at 8:24 AM, Boaz Harrosh b...@plexistor.com wrote: On 03/31/2015 06:17 PM, Dan Williams wrote: On Tue, Mar 31, 2015 at 6:27 AM, Boaz Harrosh b...@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-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Linux-nvdimm] [PATCH 4/6] SQUSHME: pmem: Micro cleaning
On Tue, Mar 31, 2015 at 6:27 AM, Boaz Harrosh b...@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-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Linux-nvdimm] [PATCH 4/6] SQUSHME: pmem: Micro cleaning
On 03/31/2015 06:17 PM, Dan Williams wrote: On Tue, Mar 31, 2015 at 6:27 AM, Boaz Harrosh b...@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-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Linux-nvdimm] [PATCH 4/6] SQUSHME: pmem: Micro cleaning
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-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
