Re: [PATCH rdma-core 4/8] mlx5: Avoid sparse complaints about !!

[Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>]

On Thu, Jul 13, 2017 at 12:51:16AM -0700, Christoph Hellwig wrote:
> >  			if (qp->qp_cap_cache & MLX5_RX_CSUM_VALID)
> > -				wc->wc_flags |= (!!(cqe->hds_ip_ext & MLX5_CQE_L4_OK) &
> > -						 !!(cqe->hds_ip_ext & MLX5_CQE_L3_OK) &
> > -						(get_cqe_l3_hdr_type(cqe) ==
> > -						MLX5_CQE_L3_HDR_TYPE_IPV4)) <<
> > -						IBV_WC_IP_CSUM_OK_SHIFT;
> > +				wc->wc_flags |=
> > +				    ((bool)(cqe->hds_ip_ext & MLX5_CQE_L4_OK) &
> > +				     (bool)(cqe->hds_ip_ext & MLX5_CQE_L3_OK) &
> > +				     (get_cqe_l3_hdr_type(cqe) ==
> > +				      MLX5_CQE_L3_HDR_TYPE_IPV4))
> > +				    << IBV_WC_IP_CSUM_OK_SHIFT;
> 
> Meh.  This code is complete crap.  Please factor it out into a little
> helper that mere humans can read first.  And then replace the odd ^ used
> as && with proper if constructs and all should make much more sense.

Leon/Christoph:

As far as I could make out, this ugly thing is designed like this for
performance.

The choice of & vs && avoids branching (since && is short circuiting)
and the above boils down to some bitwise maths with no branches.

Other easier to read alternatives have branching and many more
instructions.

I don't want to argue with Mellanox on performance just to fix a
sparse warning, and they use the same construct in their kernel
driver, IIRC.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html