[PATCH 1/2] fs: ntfs: drop unneeded likely() call around IS_ERR()

[PATCH 1/2] fs: ntfs: drop unneeded likely() call around IS_ERR()

[Enrico Weigelt, metux IT consult]

From: Enrico Weigelt <info@metux.net>

IS_ERR() already calls unlikely(), so this extra likely() call
around the !IS_ERR() is not needed.

Signed-off-by: Enrico Weigelt <info@metux.net>
---
 fs/ntfs/mft.c     | 8 ++++----
 fs/ntfs/namei.c   | 2 +-
 fs/ntfs/runlist.c | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/ntfs/mft.c b/fs/ntfs/mft.c
index 20c841a..06f706b 100644
--- a/fs/ntfs/mft.c
+++ b/fs/ntfs/mft.c
@@ -71,7 +71,7 @@ static inline MFT_RECORD *map_mft_record_page(ntfs_inode *ni)
 	}
 	/* Read, map, and pin the page. */
 	page = ntfs_map_page(mft_vi->i_mapping, index);
-	if (likely(!IS_ERR(page))) {
+	if (!IS_ERR(page)) {
 		/* Catch multi sector transfer fixup errors. */
 		if (likely(ntfs_is_mft_recordp((le32*)(page_address(page) +
 				ofs)))) {
@@ -154,7 +154,7 @@ MFT_RECORD *map_mft_record(ntfs_inode *ni)
 	mutex_lock(&ni->mrec_lock);
 
 	m = map_mft_record_page(ni);
-	if (likely(!IS_ERR(m)))
+	if (!IS_ERR(m))
 		return m;
 
 	mutex_unlock(&ni->mrec_lock);
@@ -271,7 +271,7 @@ MFT_RECORD *map_extent_mft_record(ntfs_inode *base_ni, MFT_REF mref,
 		m = map_mft_record(ni);
 		/* map_mft_record() has incremented this on success. */
 		atomic_dec(&ni->count);
-		if (likely(!IS_ERR(m))) {
+		if (!IS_ERR(m)) {
 			/* Verify the sequence number. */
 			if (likely(le16_to_cpu(m->sequence_number) == seq_no)) {
 				ntfs_debug("Done 1.");
@@ -1776,7 +1776,7 @@ static int ntfs_mft_data_extend_allocation_nolock(ntfs_volume *vol)
 	do {
 		rl2 = ntfs_cluster_alloc(vol, old_last_vcn, nr, lcn, MFT_ZONE,
 				true);
-		if (likely(!IS_ERR(rl2)))
+		if (!IS_ERR(rl2))
 			break;
 		if (PTR_ERR(rl2) != -ENOSPC || nr == min_nr) {
 			ntfs_error(vol->sb, "Failed to allocate the minimal "
diff --git a/fs/ntfs/namei.c b/fs/ntfs/namei.c
index 2d3cc9e..4e6a44b 100644
--- a/fs/ntfs/namei.c
+++ b/fs/ntfs/namei.c
@@ -115,7 +115,7 @@ static struct dentry *ntfs_lookup(struct inode *dir_ino, struct dentry *dent,
 		dent_ino = MREF(mref);
 		ntfs_debug("Found inode 0x%lx. Calling ntfs_iget.", dent_ino);
 		dent_inode = ntfs_iget(vol->sb, dent_ino);
-		if (likely(!IS_ERR(dent_inode))) {
+		if (!IS_ERR(dent_inode)) {
 			/* Consistency check. */
 			if (is_bad_inode(dent_inode) || MSEQNO(mref) ==
 					NTFS_I(dent_inode)->seq_no ||
diff --git a/fs/ntfs/runlist.c b/fs/ntfs/runlist.c
index 508744a..97932fb 100644
--- a/fs/ntfs/runlist.c
+++ b/fs/ntfs/runlist.c
@@ -951,7 +951,7 @@ runlist_element *ntfs_mapping_pairs_decompress(const ntfs_volume *vol,
 	}
 	/* Now combine the new and old runlists checking for overlaps. */
 	old_rl = ntfs_runlists_merge(old_rl, rl);
-	if (likely(!IS_ERR(old_rl)))
+	if (!IS_ERR(old_rl))
 		return old_rl;
 	ntfs_free(rl);
 	ntfs_error(vol->sb, "Failed to merge runlists.");
-- 
1.9.1

[PATCH 2/2] drivers: input: mouse: alps: drop unneeded likely() call around IS_ERR()

[Enrico Weigelt, metux IT consult]

From: Enrico Weigelt <info@metux.net>

IS_ERR() already calls unlikely(), so this extra unlikely() call
around IS_ERR() is not needed.

Signed-off-by: Enrico Weigelt <info@metux.net>
---
 drivers/input/mouse/alps.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 34700ed..ed16614 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -1476,7 +1476,7 @@ static void alps_report_bare_ps2_packet(struct psmouse *psmouse,
 		/* On V2 devices the DualPoint Stick reports bare packets */
 		dev = priv->dev2;
 		dev2 = psmouse->dev;
-	} else if (unlikely(IS_ERR_OR_NULL(priv->dev3))) {
+	} else if (IS_ERR_OR_NULL(priv->dev3)) {
 		/* Register dev3 mouse if we received PS/2 packet first time */
 		if (!IS_ERR(priv->dev3))
 			psmouse_queue_work(psmouse, &priv->dev3_register_work,
-- 
1.9.1

Re: [PATCH 2/2] drivers: input: mouse: alps: drop unneeded likely() call around IS_ERR()

[Pali Rohár]

On Tuesday 20 August 2019 12:56:12 Enrico Weigelt, metux IT consult wrote:
> From: Enrico Weigelt <info@metux.net>
> 
> IS_ERR() already calls unlikely(), so this extra unlikely() call
> around IS_ERR() is not needed.
> 
> Signed-off-by: Enrico Weigelt <info@metux.net>
> ---
>  drivers/input/mouse/alps.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index 34700ed..ed16614 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -1476,7 +1476,7 @@ static void alps_report_bare_ps2_packet(struct psmouse *psmouse,
>  		/* On V2 devices the DualPoint Stick reports bare packets */
>  		dev = priv->dev2;
>  		dev2 = psmouse->dev;
> -	} else if (unlikely(IS_ERR_OR_NULL(priv->dev3))) {
> +	} else if (IS_ERR_OR_NULL(priv->dev3)) {
>  		/* Register dev3 mouse if we received PS/2 packet first time */
>  		if (!IS_ERR(priv->dev3))
>  			psmouse_queue_work(psmouse, &priv->dev3_register_work,

Hello! Two months ago I already saw this patch. See discussion:
https://patchwork.kernel.org/patch/10977099/

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 2/2] drivers: input: mouse: alps: drop unneeded likely() call around IS_ERR()

[Enrico Weigelt, metux IT consult]

On 20.08.19 13:17, Pali Rohár wrote:
> On Tuesday 20 August 2019 12:56:12 Enrico Weigelt, metux IT consult wrote:
>> From: Enrico Weigelt <info@metux.net>
>>
>> IS_ERR() already calls unlikely(), so this extra unlikely() call
>> around IS_ERR() is not needed.
>>
>> Signed-off-by: Enrico Weigelt <info@metux.net>
>> ---
>>   drivers/input/mouse/alps.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
>> index 34700ed..ed16614 100644
>> --- a/drivers/input/mouse/alps.c
>> +++ b/drivers/input/mouse/alps.c
>> @@ -1476,7 +1476,7 @@ static void alps_report_bare_ps2_packet(struct psmouse *psmouse,
>>   		/* On V2 devices the DualPoint Stick reports bare packets */
>>   		dev = priv->dev2;
>>   		dev2 = psmouse->dev;
>> -	} else if (unlikely(IS_ERR_OR_NULL(priv->dev3))) {
>> +	} else if (IS_ERR_OR_NULL(priv->dev3)) {
>>   		/* Register dev3 mouse if we received PS/2 packet first time */
>>   		if (!IS_ERR(priv->dev3))
>>   			psmouse_queue_work(psmouse, &priv->dev3_register_work,
> 
> Hello! Two months ago I already saw this patch. See discussion:
> https://patchwork.kernel.org/patch/10977099/

phuh, that's long chain of links to folow ;-)

So, your primary argument is just *documenting* that this supposed to
be a rare condition ?

In that case, wouldn't a comment be more suitable for that ?

It seems that this issue is coming up again and again ... when people
run cocci scans (like I didn't), I'd suspect this to happen even more
in the future.


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [PATCH 2/2] drivers: input: mouse: alps: drop unneeded likely() call around IS_ERR()

[Pali Rohár]

On Tuesday 20 August 2019 14:21:33 Enrico Weigelt, metux IT consult wrote:
> On 20.08.19 13:17, Pali Rohár wrote:
> > On Tuesday 20 August 2019 12:56:12 Enrico Weigelt, metux IT consult wrote:
> > > From: Enrico Weigelt <info@metux.net>
> > > 
> > > IS_ERR() already calls unlikely(), so this extra unlikely() call
> > > around IS_ERR() is not needed.
> > > 
> > > Signed-off-by: Enrico Weigelt <info@metux.net>
> > > ---
> > >   drivers/input/mouse/alps.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> > > index 34700ed..ed16614 100644
> > > --- a/drivers/input/mouse/alps.c
> > > +++ b/drivers/input/mouse/alps.c
> > > @@ -1476,7 +1476,7 @@ static void alps_report_bare_ps2_packet(struct psmouse *psmouse,
> > >   		/* On V2 devices the DualPoint Stick reports bare packets */
> > >   		dev = priv->dev2;
> > >   		dev2 = psmouse->dev;
> > > -	} else if (unlikely(IS_ERR_OR_NULL(priv->dev3))) {
> > > +	} else if (IS_ERR_OR_NULL(priv->dev3)) {
> > >   		/* Register dev3 mouse if we received PS/2 packet first time */
> > >   		if (!IS_ERR(priv->dev3))
> > >   			psmouse_queue_work(psmouse, &priv->dev3_register_work,
> > 
> > Hello! Two months ago I already saw this patch. See discussion:
> > https://patchwork.kernel.org/patch/10977099/
> 
> phuh, that's long chain of links to folow ;-)
> 
> So, your primary argument is just *documenting* that this supposed to
> be a rare condition ?

Yes. Also Dmitry said that prefer explicit unlikely.

> In that case, wouldn't a comment be more suitable for that ?

And why to add comment if current state of code is more-readable and
does not need it?

I do not see anything wrong on this code path.

People normally add comments to code which is problematic to understand
or is somehow tricky, no so obvious or document how should code behave.

> It seems that this issue is coming up again and again ... when people
> run cocci scans (like I didn't), I'd suspect this to happen even more
> in the future.

People first need to understand that static code analysis cannot work
100% reliably and always is needed to properly review changes.

And if the only reason for this change is to satisfy some static code
analysis program, would not it better to teach this program to no
generate such false-positive results?

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 2/2] drivers: input: mouse: alps: drop unneeded likely() call around IS_ERR()

[Enrico Weigelt, metux IT consult]

On 20.08.19 16:22, Pali Rohár wrote:

Hi,

>> In that case, wouldn't a comment be more suitable for that ?
> 
> And why to add comment if current state of code is more-readable and
> does not need it?

Readability is probably a bit subjective :p

With ongoing efforts of automatically identifying redundant code pathes,
the current situation causes the same discussion coming up over and over
again. Sooner or later somebody might get the idea to add a comment on
that line, that it's exactly as intented :o

OTOH, I'm unsure whether it's important to document that is particular
error path is unlikely, while we don't do it in thousands of other
places. IMHO, error pathes are supposed to be unlikely by nature,
otherwise we wouldn't call it an error situation ;-)

> People normally add comments to code which is problematic to understand
> or is somehow tricky, no so obvious or document how should code behave.

Yes, but isn't this case so obvious that it doesn't need any
documentation at all ? Is it so important to never ever forget that this
particular path is a rare situation ?

I might be wrong, but IMHO the likely()/unlikely() macros have been
introduced for optimization (better pipeline optimization for the
frequent cases).

> People first need to understand that static code analysis cannot work
> 100% reliably and always is needed to properly review changes.

Yes, but we see that this particular case is coming up again and again,
from different people, who can't know the background of this (nobody
can read the whole lkml and remember everything). This could be
prevented by adding a comment on that line, but then the macro call
just for documentation wouldn't be necessary anymore.

> And if the only reason for this change is to satisfy some static code
> analysis program,

Actually, it's more for people who're using the tools for the purpose
of tidying up the code and find potential problems. They need some
information to act on properly, which is currently missing ...

> would not it better to teach this program to no
> generate such false-positive results?

Okay, but how to do that practically ? There's no information in the
code which neither some tool nor any human could recognize this false
alarm. That information is just in your brain, now also in mine, and
the other folks who previously proposed the same change (assuming they
still recall it after years).

Blacklisting doesn't seem to be a good idea in the long run. Once going
that route, such blacklists grow quickly and get hard to maintain (we
now have two entirely separately maintained data sources that need to
be synced manually, when something changes).

I believe the code should be kinda self-documenting, so the reader
shouldn't need any detailed special knowledge to understand it. IMHO,
when such discussions on actual implementation details come up, the
code isn't expressive enough.

How can we improve the situation ?

Shall we consider introducing yet another macro (explicitly different
name) for stating/documenting such cases ?


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [PATCH 2/2] drivers: input: mouse: alps: drop unneeded likely() call around IS_ERR()

[Dmitry Torokhov]

Hi,

On Wed, Aug 21, 2019 at 01:37:09PM +0200, Enrico Weigelt, metux IT consult wrote:
> On 20.08.19 16:22, Pali Rohár wrote:
> 
> Hi,
> 
> > > In that case, wouldn't a comment be more suitable for that ?
> > 
> > And why to add comment if current state of code is more-readable and
> > does not need it?
> 
> Readability is probably a bit subjective :p
> 
> With ongoing efforts of automatically identifying redundant code pathes,
> the current situation causes the same discussion coming up over and over
> again. Sooner or later somebody might get the idea to add a comment on
> that line, that it's exactly as intented :o
> 
> OTOH, I'm unsure whether it's important to document that is particular
> error path is unlikely, while we don't do it in thousands of other
> places. IMHO, error pathes are supposed to be unlikely by nature,
> otherwise we wouldn't call it an error situation ;-)
> 
> > People normally add comments to code which is problematic to understand
> > or is somehow tricky, no so obvious or document how should code behave.
> 
> Yes, but isn't this case so obvious that it doesn't need any
> documentation at all ? Is it so important to never ever forget that this
> particular path is a rare situation ?

Because if I see "if (IS_ERR(...))" in an interrupt path I will try to
see if it can be optimized out, but in this particular case we document
it with explicit "unlikely" and I know that I do not need to bother.

The fact that there is unlikely in IS_ERR is an implementation detail.
It may be gone tomorrow. I do not want to have to remember all
implementation details of all kernel APIs and readjust the code all the
time as they are change underneath me.

Thanks.

-- 
Dmitry