diff mbox

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

Message ID 20170713183737.GF11069@obsidianresearch.com (mailing list archive)
State Accepted
Headers show

Commit Message

Jason Gunthorpe July 13, 2017, 6:37 p.m. UTC 
On Thu, Jul 13, 2017 at 05:52:06PM +0000, Bart Van Assche wrote:
> 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

Yes, thanks, that seems a reasonable middle ground:

--
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

Comments

Leon Romanovsky July 15, 2017, 8:07 a.m. UTC | #1 
On Thu, Jul 13, 2017 at 12:37:37PM -0600, Jason Gunthorpe wrote:
> On Thu, Jul 13, 2017 at 05:52:06PM +0000, Bart Van Assche wrote:
> > 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
>
> Yes, thanks, that seems a reasonable middle ground:

Thanks Jason and Bart.

>
> diff --git a/providers/mlx5/cq.c b/providers/mlx5/cq.c
> index b845127de937d0..0a6274e6878d3c 100644
> --- a/providers/mlx5/cq.c
> +++ b/providers/mlx5/cq.c
> @@ -182,6 +182,15 @@ static inline int handle_responder_lazy(struct mlx5_cq *cq, struct mlx5_cqe64 *c
>  	return err;
>  }
>
> +/* Returns IBV_WC_IP_CSUM_OK or 0 */
> +static inline int get_csum_ok(struct mlx5_cqe64 *cqe)
> +{
> +	return (((cqe->hds_ip_ext & (MLX5_CQE_L4_OK | MLX5_CQE_L3_OK)) ==
> +		 (MLX5_CQE_L4_OK | MLX5_CQE_L3_OK)) &
> +		(get_cqe_l3_hdr_type(cqe) == MLX5_CQE_L3_HDR_TYPE_IPV4))
> +	       << IBV_WC_IP_CSUM_OK_SHIFT;
> +}
> +
>  static inline int handle_responder(struct ibv_wc *wc, struct mlx5_cqe64 *cqe,
>  				   struct mlx5_resource *cur_rsc, struct mlx5_srq *srq)
>  {
> @@ -206,12 +215,7 @@ 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 |=
> -				    ((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;
> +				wc->wc_flags |= get_csum_ok(cqe);
>  		} else {
>  			wq = &(rsc_to_mrwq(cur_rsc)->rq);
>  		}
> @@ -1106,11 +1110,7 @@ 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 = ((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;
> +		wc_flags = get_csum_ok(cq->cqe64);
>
>  	switch (mlx5dv_get_cqe_opcode(cq->cqe64)) {
>  	case MLX5_CQE_RESP_WR_IMM:
> --
> 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 b845127de937d0..0a6274e6878d3c 100644
--- a/providers/mlx5/cq.c
+++ b/providers/mlx5/cq.c
@@ -182,6 +182,15 @@  static inline int handle_responder_lazy(struct mlx5_cq *cq, struct mlx5_cqe64 *c
 	return err;
 }
 
+/* Returns IBV_WC_IP_CSUM_OK or 0 */
+static inline int get_csum_ok(struct mlx5_cqe64 *cqe)
+{
+	return (((cqe->hds_ip_ext & (MLX5_CQE_L4_OK | MLX5_CQE_L3_OK)) ==
+		 (MLX5_CQE_L4_OK | MLX5_CQE_L3_OK)) &
+		(get_cqe_l3_hdr_type(cqe) == MLX5_CQE_L3_HDR_TYPE_IPV4))
+	       << IBV_WC_IP_CSUM_OK_SHIFT;
+}
+
 static inline int handle_responder(struct ibv_wc *wc, struct mlx5_cqe64 *cqe,
 				   struct mlx5_resource *cur_rsc, struct mlx5_srq *srq)
 {
@@ -206,12 +215,7 @@  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 |=
-				    ((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;
+				wc->wc_flags |= get_csum_ok(cqe);
 		} else {
 			wq = &(rsc_to_mrwq(cur_rsc)->rq);
 		}
@@ -1106,11 +1110,7 @@  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 = ((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;
+		wc_flags = get_csum_ok(cq->cqe64);
 
 	switch (mlx5dv_get_cqe_opcode(cq->cqe64)) {
 	case MLX5_CQE_RESP_WR_IMM: