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

Jason Gunthorpe
2017-07-13T17:20:35Z

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