Re: FW: [PATCH] dm integrity: drop excess unlikely() from BUG_ON()

[Nicholas Mc Guire <der.herr@hofr.at>]

On Tue, Sep 18, 2018 at 06:52:49AM -0400, Mikulas Patocka wrote:
> I think this patch is not needed.
> 
> In the statement "BUG_ON(unlikely(journal_entry_is_inprogress(je)) && 
> !from_replay);", the "unlikely" macro suggests that the first condition is 
> likely to be false and therefore the compiler will move the test for the 
> second condition to the cold section.
> 
> If you remove unlikely, the compiler will know that that one of the 
> conditions will be false, but it won't know which one.
>
If I understand builtin_expect correctly it is basically only changing the
relativ location of the branch wich impacts the probability of having it
in the active page-set or not - as the whole condition is already under
unlikely() it will potentially be pushed to some non-local position already
which affects both conditions and then differenciating within that moved
block which should be located where seems usless - but it may be that some
architectures can actually gain something from this differenciation I was 
though not able to figure out an actual case where it would matter. In the
case of this patch it seems to have no effect (inspected the .lst file 
before and after the patch was applied).

thx!
hofrat
 
> 
> 
> On Sat, 15 Sep 2018, Alasdair G Kergon wrote:
> 
> > One for you to decide and respond - it's not *identical* of course...
> > 
> > (I wonder if this was generated by a script rather than looking at what the compiler actually does)
> > 
> > ----- Forwarded message from Nicholas Mc Guire <hofrat@osadl.org> -----
> > 
> > Date: Fri, 14 Sep 2018 09:37:03 +0200
> > From: Nicholas Mc Guire <hofrat@osadl.org>
> > Subject: [dm-devel] [PATCH] dm integrity: drop excess unlikely() from
> > 	BUG_ON()
> > To: Alasdair Kergon <agk@redhat.com>
> > Cc: dm-devel@redhat.com, linux-kernel@vger.kernel.org,
> > 	Mike Snitzer <snitzer@redhat.com>,
> > 	Nicholas Mc Guire <hofrat@osadl.org>
> > 
> >  include/asm-generic/bug.h defines BUG_ON(condition) as
> > do { if (unlikely(condition)) BUG(); } while (0).
> > So BUG_ON already provides unlikely() on the condition thus 
> > there is no point in having BUG_ON(unlikely(condition)),
> > thus simply drop the excess unlikely().
> > 
> > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> > ---
> > 
> > Found during code review
> > 
> > Patch was compile tested with: x86_64_defconfig + SCSI_LOWLEVEL=y,
> > DM_INTEGRITY=y
> > (with warnings from sparse and smatch - not related to the proposed
> >  change though)
> > 
> > Patch is against 4.19-rc3 (localversion-next is next-20180913)
> > 
> >  drivers/md/dm-integrity.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c
> > index 89ccb64..8574e73 100644
> > --- a/drivers/md/dm-integrity.c
> > +++ b/drivers/md/dm-integrity.c
> > @@ -1948,7 +1948,8 @@ static void do_journal_write(struct dm_integrity_c *ic, unsigned write_start,
> >  
> >  			if (journal_entry_is_unused(je))
> >  				continue;
> > -			BUG_ON(unlikely(journal_entry_is_inprogress(je)) && !from_replay);
> > +			BUG_ON(journal_entry_is_inprogress(je) &&
> > +			       !from_replay);
> >  			sec = journal_entry_get_sector(je);
> >  			if (unlikely(from_replay)) {
> >  				if (unlikely(sec & (unsigned)(ic->sectors_per_block - 1))) {
> > @@ -1963,7 +1964,8 @@ static void do_journal_write(struct dm_integrity_c *ic, unsigned write_start,
> >  				sector_t sec2, area2, offset2;
> >  				if (journal_entry_is_unused(je2))
> >  					break;
> > -				BUG_ON(unlikely(journal_entry_is_inprogress(je2)) && !from_replay);
> > +				BUG_ON(journal_entry_is_inprogress(je2) &&
> > +				       !from_replay);
> >  				sec2 = journal_entry_get_sector(je2);
> >  				get_area_and_offset(ic, sec2, &area2, &offset2);
> >  				if (area2 != area || offset2 != offset + ((k - j) << ic->sb->log2_sectors_per_block))
> > -- 
> > 2.1.4
> > 
> > --
> > dm-devel mailing list
> > dm-devel@redhat.com
> > https://www.redhat.com/mailman/listinfo/dm-devel
> > 
> > ----- End forwarded message -----
> >