RE: [PATCH net-next 5/5] net: sctp: fix incorrect type in gfp initializer

[David Laight <David.Laight@ACULAB.COM>]

From: Alexei Starovoitov
> On Wed, Jun 11, 2014 at 8:09 AM, Daniel Borkmann <dborkman@redhat.com> wrote:
> > On 06/11/2014 04:55 PM, David Laight wrote:
> >>
> >> From: Daniel Borkmann
...
> >>> --- a/net/sctp/associola.c
> >>> +++ b/net/sctp/associola.c
> >>> @@ -1591,7 +1591,7 @@ int sctp_assoc_lookup_laddr(struct sctp_association
> >>> *asoc,
> >>>   /* Set an association id for a given association */
> >>>   int sctp_assoc_set_id(struct sctp_association *asoc, gfp_t gfp)
> >>>   {
> >>> -       bool preload = gfp & __GFP_WAIT;
> >>> +       bool preload = !!(gfp & __GFP_WAIT);
> >>>         int ret;
> >>>
> >>>         /* If the id is already assigned, keep it. */
> >>> --
> >>
> >>
> >> I was wondering if the compiler still manages to optimise this in a
> >> manner that avoids actually calculating the boolean value...
> >>
> >> So I disassembled the compilation I just did of the old code (gcc 4.7.3).
> >> The object code looks strange.
> 
> I'm not sure where you see this.
> Just tried with gcc 4.7.2 on x64 and assembler code is exactly the
> same before/after this change.

I was slightly worried it might generate the boolean value - something
that you really don't want it to do.

I only looked at the output for the old version.
The compiler seemed to have converted:
	if (preload)
		x();
	y;
	if (preload)
		z();
into:
	if (preload) {
		x(); y; z();
	} else {
		y;
	}
and then found out that z() was empty, leaving two copies of y().

	David

> >> I think that idr_preload_end() must be an empty inline function.
> >
> >
> > Cc'ing Tejun. ;-)
> >
> >
> >> The compiler has duplicated the code between the two 'if (preload)'
> >> clauses (to avoid the conditional test), and the failed to tail
> >> merge everything in the latter stages.
> >> I suspect that an empty '#define' would generate smaller code.
> >>
> >>         David