diff mbox

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

Message ID 1499894262-10761-5-git-send-email-jgunthorpe@obsidianresearch.com (mailing list archive)
State Superseded
Headers show

Commit Message

Jason Gunthorpe July 12, 2017, 9:17 p.m. UTC 
Sparse says:
 warning: dubious: !x & y

Unclear why sparse thinks this is bad, but casting to C99 bool is the
same as !! and much easier to read.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 providers/mlx5/cq.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

Comments

Leon Romanovsky July 13, 2017, 7:34 a.m. UTC | #1 
On Wed, Jul 12, 2017 at 03:17:38PM -0600, Jason Gunthorpe wrote:
> Sparse says:
>  warning: dubious: !x & y
>
> Unclear why sparse thinks this is bad, but casting to C99 bool is the
> same as !! and much easier to read.

IMHO, it is because of usage "&" to compare boolean result.

>
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> ---
>  providers/mlx5/cq.c | 21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/providers/mlx5/cq.c b/providers/mlx5/cq.c
> index 500116133c1f9e..9a8d958a9ced68 100644
> --- a/providers/mlx5/cq.c
> +++ b/providers/mlx5/cq.c
> @@ -206,11 +206,12 @@ static inline int handle_responder(struct ibv_wc *wc, struct mlx5_cqe64 *cqe,
>  		if (likely(cur_rsc->type == MLX5_RSC_TYPE_QP)) {
>  			wq = &qp->rq;
>  			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) &

Those "&" should be &&.

> +				     (get_cqe_l3_hdr_type(cqe) ==
> +				      MLX5_CQE_L3_HDR_TYPE_IPV4))
> +				    << IBV_WC_IP_CSUM_OK_SHIFT;
>  		} else {
>  			wq = &(rsc_to_mrwq(cur_rsc)->rq);
>  		}
> @@ -1105,11 +1106,11 @@ static inline int mlx5_cq_read_wc_flags(struct ibv_cq_ex *ibcq)
>  	int wc_flags = 0;
>
>  	if (cq->flags & MLX5_CQ_FLAGS_RX_CSUM_VALID)
> -		wc_flags = (!!(cq->cqe64->hds_ip_ext & MLX5_CQE_L4_OK) &
> -				 !!(cq->cqe64->hds_ip_ext & MLX5_CQE_L3_OK) &
> -				 (get_cqe_l3_hdr_type(cq->cqe64) ==
> -				  MLX5_CQE_L3_HDR_TYPE_IPV4)) <<
> -				IBV_WC_IP_CSUM_OK_SHIFT;
> +		wc_flags = ((bool)(cq->cqe64->hds_ip_ext & MLX5_CQE_L4_OK) &
> +			    (bool)(cq->cqe64->hds_ip_ext & MLX5_CQE_L3_OK) &
> +			    (get_cqe_l3_hdr_type(cq->cqe64) ==
> +			     MLX5_CQE_L3_HDR_TYPE_IPV4))
> +			   << IBV_WC_IP_CSUM_OK_SHIFT;
>
>  	switch (mlx5dv_get_cqe_opcode(cq->cqe64)) {
>  	case MLX5_CQE_RESP_WR_IMM:
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Hellwig July 13, 2017, 7:51 a.m. UTC | #2 
On Wed, Jul 12, 2017 at 03:17:38PM -0600, Jason Gunthorpe wrote:
> Sparse says:
>  warning: dubious: !x & y
> 
> Unclear why sparse thinks this is bad, but casting to C99 bool is the
> same as !! and much easier to read.
> 
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> ---
>  providers/mlx5/cq.c | 21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/providers/mlx5/cq.c b/providers/mlx5/cq.c
> index 500116133c1f9e..9a8d958a9ced68 100644
> --- a/providers/mlx5/cq.c
> +++ b/providers/mlx5/cq.c
> @@ -206,11 +206,12 @@ static inline int handle_responder(struct ibv_wc *wc, struct mlx5_cqe64 *cqe,
>  		if (likely(cur_rsc->type == MLX5_RSC_TYPE_QP)) {
>  			wq = &qp->rq;
>  			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.

Something like:

static bool mlx4_ipv4_csum_ok(sruct mlx5_cqe64 *cqe)
{
	if (get_cqe_l3_hdr_type(cqe) != MLX5_CQE_L3_HDR_TYPE_IPV4)
		return false;
	if (!(cqe->hds_ip_ext & MLX5_CQE_L4_OK))
		return false;
	if (!(cqe->hds_ip_ext & MLX5_CQE_L3_OK))
		return false;
	return true;
}

...

		if (mlx4_ipv4_csum_ok(cqe))
			wc->wc_flags |= 1 << IBV_WC_IP_CSUM_OK_SHIFT;
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jason Gunthorpe July 13, 2017, 5:20 p.m. UTC | #3 
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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bart Van Assche July 13, 2017, 5:52 p.m. UTC | #4 
On Thu, 2017-07-13 at 11:20 -0600, Jason Gunthorpe wrote:
> 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.
> 
> As far as I could make out, this ugly thing is designed like this for
> performance.

Hello Jason,

How about using an expression like the below to avoid that branches get inserted
for testing the MLX5_CQE_L4_OK and MLX5_CQE_L3_OK flags?

(cqe->hds_ip_ext & (MLX5_CQE_L4_OK | MLX5_CQE_L3_OK)) == MLX5_CQE_L4_OK | MLX5_CQE_L3_OK

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

Patch

diff --git a/providers/mlx5/cq.c b/providers/mlx5/cq.c
index 500116133c1f9e..9a8d958a9ced68 100644
--- a/providers/mlx5/cq.c
+++ b/providers/mlx5/cq.c
@@ -206,11 +206,12 @@  static inline int handle_responder(struct ibv_wc *wc, struct mlx5_cqe64 *cqe,
 		if (likely(cur_rsc->type == MLX5_RSC_TYPE_QP)) {
 			wq = &qp->rq;
 			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;
 		} else {
 			wq = &(rsc_to_mrwq(cur_rsc)->rq);
 		}
@@ -1105,11 +1106,11 @@  static inline int mlx5_cq_read_wc_flags(struct ibv_cq_ex *ibcq)
 	int wc_flags = 0;
 
 	if (cq->flags & MLX5_CQ_FLAGS_RX_CSUM_VALID)
-		wc_flags = (!!(cq->cqe64->hds_ip_ext & MLX5_CQE_L4_OK) &
-				 !!(cq->cqe64->hds_ip_ext & MLX5_CQE_L3_OK) &
-				 (get_cqe_l3_hdr_type(cq->cqe64) ==
-				  MLX5_CQE_L3_HDR_TYPE_IPV4)) <<
-				IBV_WC_IP_CSUM_OK_SHIFT;
+		wc_flags = ((bool)(cq->cqe64->hds_ip_ext & MLX5_CQE_L4_OK) &
+			    (bool)(cq->cqe64->hds_ip_ext & MLX5_CQE_L3_OK) &
+			    (get_cqe_l3_hdr_type(cq->cqe64) ==
+			     MLX5_CQE_L3_HDR_TYPE_IPV4))
+			   << IBV_WC_IP_CSUM_OK_SHIFT;
 
 	switch (mlx5dv_get_cqe_opcode(cq->cqe64)) {
 	case MLX5_CQE_RESP_WR_IMM: