diff mbox

[v4,14/17] net/rds: Simplify ib_post_(send|recv|srq_recv)() calls

Message ID 20180716235905.6387-15-bart.vanassche@wdc.com (mailing list archive)
State Superseded
Headers show

Commit Message

Bart Van Assche July 16, 2018, 11:59 p.m. UTC 
Instead of declaring and passing a dummy 'bad_wr' pointer, pass NULL
as third argument to ib_post_(send|recv|srq_recv)().

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Santosh Shilimkar <santosh.shilimkar@oracle.com>
---
 net/rds/ib_frmr.c | 11 +++--------
 net/rds/ib_recv.c |  6 ++----
 2 files changed, 5 insertions(+), 12 deletions(-)

Comments

Santosh Shilimkar July 17, 2018, 12:20 a.m. UTC | #1 
On 7/16/2018 4:59 PM, Bart Van Assche wrote:
> Instead of declaring and passing a dummy 'bad_wr' pointer, pass NULL
> as third argument to ib_post_(send|recv|srq_recv)().
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Santosh Shilimkar <santosh.shilimkar@oracle.com>
> ---
>   net/rds/ib_frmr.c | 11 +++--------
>   net/rds/ib_recv.c |  6 ++----
>   2 files changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/net/rds/ib_frmr.c b/net/rds/ib_frmr.c
> index 48332a6ed738..09ab97475fc9 100644
> --- a/net/rds/ib_frmr.c
> +++ b/net/rds/ib_frmr.c
> @@ -102,7 +102,6 @@ static void rds_ib_free_frmr(struct rds_ib_mr *ibmr, bool drop)
>   static int rds_ib_post_reg_frmr(struct rds_ib_mr *ibmr)
>   {
>   	struct rds_ib_frmr *frmr = &ibmr->u.frmr;
> -	struct ib_send_wr *failed_wr;
>   	struct ib_reg_wr reg_wr;
>   	int ret, off = 0;
>   
> @@ -135,9 +134,7 @@ static int rds_ib_post_reg_frmr(struct rds_ib_mr *ibmr)
>   			IB_ACCESS_REMOTE_WRITE;
>   	reg_wr.wr.send_flags = IB_SEND_SIGNALED;
>   
> -	failed_wr = &reg_wr.wr;
> -	ret = ib_post_send(ibmr->ic->i_cm_id->qp, &reg_wr.wr, &failed_wr);
> -	WARN_ON(failed_wr != &reg_wr.wr);
> +	ret = ib_post_send(ibmr->ic->i_cm_id->qp, &reg_wr.wr, NULL);

This was one way to find out if the post_send failed with intended
wr. WARN_ON(failed_wr != &reg_wr.wr). That dummy was just place
holder to copy the failed one into it.

Do we get same behavior with NULL Bart ? I guess not.

regards,
Santosh
--
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 17, 2018, 1 a.m. UTC | #2 
On Mon, 2018-07-16 at 17:20 -0700, Santosh Shilimkar wrote:
> On 7/16/2018 4:59 PM, Bart Van Assche wrote:
> > Instead of declaring and passing a dummy 'bad_wr' pointer, pass NULL
> > as third argument to ib_post_(send|recv|srq_recv)().
> > 
> > Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> > Cc: Santosh Shilimkar <santosh.shilimkar@oracle.com>
> > ---
> >   net/rds/ib_frmr.c | 11 +++--------
> >   net/rds/ib_recv.c |  6 ++----
> >   2 files changed, 5 insertions(+), 12 deletions(-)
> > 
> > diff --git a/net/rds/ib_frmr.c b/net/rds/ib_frmr.c
> > index 48332a6ed738..09ab97475fc9 100644
> > --- a/net/rds/ib_frmr.c
> > +++ b/net/rds/ib_frmr.c
> > @@ -102,7 +102,6 @@ static void rds_ib_free_frmr(struct rds_ib_mr *ibmr, bool drop)
> >   static int rds_ib_post_reg_frmr(struct rds_ib_mr *ibmr)
> >   {
> >   	struct rds_ib_frmr *frmr = &ibmr->u.frmr;
> > -	struct ib_send_wr *failed_wr;
> >   	struct ib_reg_wr reg_wr;
> >   	int ret, off = 0;
> >   
> > @@ -135,9 +134,7 @@ static int rds_ib_post_reg_frmr(struct rds_ib_mr *ibmr)
> >   			IB_ACCESS_REMOTE_WRITE;
> >   	reg_wr.wr.send_flags = IB_SEND_SIGNALED;
> >   
> > -	failed_wr = &reg_wr.wr;
> > -	ret = ib_post_send(ibmr->ic->i_cm_id->qp, &reg_wr.wr, &failed_wr);
> > -	WARN_ON(failed_wr != &reg_wr.wr);
> > +	ret = ib_post_send(ibmr->ic->i_cm_id->qp, &reg_wr.wr, NULL);
> 
> This was one way to find out if the post_send failed with intended
> wr. WARN_ON(failed_wr != &reg_wr.wr). That dummy was just place
> holder to copy the failed one into it.
> 
> Do we get same behavior with NULL Bart ? I guess not.

It's very easy to restore the WARN_ON(). But what makes you think that that
WARN_ON() statement is useful? The above ib_post_send() call posts a single
work request. So all that WARN_ON() statement does is veryfing whether the
RDMA transport driver implements the ib_post_send() API correctly. I don't
think that this is something that should be done by ULPs.

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/net/rds/ib_frmr.c b/net/rds/ib_frmr.c
index 48332a6ed738..09ab97475fc9 100644
--- a/net/rds/ib_frmr.c
+++ b/net/rds/ib_frmr.c
@@ -102,7 +102,6 @@  static void rds_ib_free_frmr(struct rds_ib_mr *ibmr, bool drop)
 static int rds_ib_post_reg_frmr(struct rds_ib_mr *ibmr)
 {
 	struct rds_ib_frmr *frmr = &ibmr->u.frmr;
-	struct ib_send_wr *failed_wr;
 	struct ib_reg_wr reg_wr;
 	int ret, off = 0;
 
@@ -135,9 +134,7 @@  static int rds_ib_post_reg_frmr(struct rds_ib_mr *ibmr)
 			IB_ACCESS_REMOTE_WRITE;
 	reg_wr.wr.send_flags = IB_SEND_SIGNALED;
 
-	failed_wr = &reg_wr.wr;
-	ret = ib_post_send(ibmr->ic->i_cm_id->qp, &reg_wr.wr, &failed_wr);
-	WARN_ON(failed_wr != &reg_wr.wr);
+	ret = ib_post_send(ibmr->ic->i_cm_id->qp, &reg_wr.wr, NULL);
 	if (unlikely(ret)) {
 		/* Failure here can be because of -ENOMEM as well */
 		frmr->fr_state = FRMR_IS_STALE;
@@ -230,7 +227,7 @@  static int rds_ib_map_frmr(struct rds_ib_device *rds_ibdev,
 
 static int rds_ib_post_inv(struct rds_ib_mr *ibmr)
 {
-	struct ib_send_wr *s_wr, *failed_wr;
+	struct ib_send_wr *s_wr;
 	struct rds_ib_frmr *frmr = &ibmr->u.frmr;
 	struct rdma_cm_id *i_cm_id = ibmr->ic->i_cm_id;
 	int ret = -EINVAL;
@@ -255,9 +252,7 @@  static int rds_ib_post_inv(struct rds_ib_mr *ibmr)
 	s_wr->ex.invalidate_rkey = frmr->mr->rkey;
 	s_wr->send_flags = IB_SEND_SIGNALED;
 
-	failed_wr = s_wr;
-	ret = ib_post_send(i_cm_id->qp, s_wr, &failed_wr);
-	WARN_ON(failed_wr != s_wr);
+	ret = ib_post_send(i_cm_id->qp, s_wr, NULL);
 	if (unlikely(ret)) {
 		frmr->fr_state = FRMR_IS_STALE;
 		frmr->fr_inv = false;
diff --git a/net/rds/ib_recv.c b/net/rds/ib_recv.c
index b4e421aa9727..4c5a937304b2 100644
--- a/net/rds/ib_recv.c
+++ b/net/rds/ib_recv.c
@@ -383,7 +383,6 @@  void rds_ib_recv_refill(struct rds_connection *conn, int prefill, gfp_t gfp)
 {
 	struct rds_ib_connection *ic = conn->c_transport_data;
 	struct rds_ib_recv_work *recv;
-	struct ib_recv_wr *failed_wr;
 	unsigned int posted = 0;
 	int ret = 0;
 	bool can_wait = !!(gfp & __GFP_DIRECT_RECLAIM);
@@ -417,7 +416,7 @@  void rds_ib_recv_refill(struct rds_connection *conn, int prefill, gfp_t gfp)
 				&recv->r_frag->f_sg));
 
 		/* XXX when can this fail? */
-		ret = ib_post_recv(ic->i_cm_id->qp, &recv->r_wr, &failed_wr);
+		ret = ib_post_recv(ic->i_cm_id->qp, &recv->r_wr, NULL);
 		if (ret) {
 			rds_ib_conn_error(conn, "recv post on "
 			       "%pI4 returned %d, disconnecting and "
@@ -650,7 +649,6 @@  static u64 rds_ib_get_ack(struct rds_ib_connection *ic)
 static void rds_ib_send_ack(struct rds_ib_connection *ic, unsigned int adv_credits)
 {
 	struct rds_header *hdr = ic->i_ack;
-	struct ib_send_wr *failed_wr;
 	u64 seq;
 	int ret;
 
@@ -663,7 +661,7 @@  static void rds_ib_send_ack(struct rds_ib_connection *ic, unsigned int adv_credi
 	rds_message_make_checksum(hdr);
 	ic->i_ack_queued = jiffies;
 
-	ret = ib_post_send(ic->i_cm_id->qp, &ic->i_ack_wr, &failed_wr);
+	ret = ib_post_send(ic->i_cm_id->qp, &ic->i_ack_wr, NULL);
 	if (unlikely(ret)) {
 		/* Failed to send. Release the WR, and
 		 * force another ACK.