diff mbox series

[rdma-next,2/3] IB/mad: Remove unnecessary done list by utilizing MAD states

Message ID 8f746ee2eac86138b1051908b95a21fdff24af6c.1733233636.git.leonro@nvidia.com (mailing list archive)
State New
Headers show
Series Add Flow Control for Solicited MADs | expand

Commit Message

Leon Romanovsky Dec. 3, 2024, 1:52 p.m. UTC 
From: Or Har-Toov <ohartoov@nvidia.com>

Remove the done list, which has become unnecessary with the
introduction of the `state` parameter.

Previously, the done list was used to ensure that MADs removed from
the wait list would still be in some list, preventing failures in
the call to `list_del` in `ib_mad_complete_send_wr`. However, with the
new state management, we can mark a MAD as done when it is completed
and simply not delete those MADs.

Removing the done list eliminates unnecessary memory usage and
simplifies the code.

Signed-off-by: Or Har-Toov <ohartoov@nvidia.com>
Reviewed-by: Maher Sanalla <msanalla@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/infiniband/core/mad.c      | 13 ++++++-------
 drivers/infiniband/core/mad_priv.h |  1 -
 2 files changed, 6 insertions(+), 8 deletions(-)

Comments

Jason Gunthorpe Dec. 10, 2024, 7 p.m. UTC | #1 
On Tue, Dec 03, 2024 at 03:52:22PM +0200, Leon Romanovsky wrote:
> From: Or Har-Toov <ohartoov@nvidia.com>
> 
> Remove the done list, which has become unnecessary with the
> introduction of the `state` parameter.
> 
> Previously, the done list was used to ensure that MADs removed from
> the wait list would still be in some list, preventing failures in
> the call to `list_del` in `ib_mad_complete_send_wr`. 

Yuk, that is a terrible reason for this. list_del_init() would solve
that problem.

> @@ -1772,13 +1771,11 @@ ib_find_send_mad(const struct ib_mad_agent_private *mad_agent_priv,
>  void ib_mark_mad_done(struct ib_mad_send_wr_private *mad_send_wr)
>  {
>  	mad_send_wr->timeout = 0;
> -	if (mad_send_wr->state == IB_MAD_STATE_WAIT_RESP) {
> +	list_del(&mad_send_wr->agent_list);

This is doing more than the commit message says, we are now changing
the order for when the mad is in the list, here you are removing it as
soon as it becomes done, or semi-done instead of letting
ib_mad_complete_send_wr() always remove it.

I couldn't find a reason it is not OK, but I think it should be in the
commit message.

>  static void ib_mad_complete_recv(struct ib_mad_agent_private *mad_agent_priv,
> @@ -2249,7 +2246,9 @@ void ib_mad_complete_send_wr(struct ib_mad_send_wr_private *mad_send_wr,
>  	}
>  
>  	/* Remove send from MAD agent and notify client of completion */
> -	list_del(&mad_send_wr->agent_list);
> +	if (mad_send_wr->state == IB_MAD_STATE_SEND_START)
> +		list_del(&mad_send_wr->agent_list);
> +

This extra if is confusing now.. There are two callers to
ib_mad_complete_send_wr(), the receive path did ib_mark_mad_done()
first so state should be DONE or EARLY_RESP and the list_del was done
already.

The other one is send completion which should have state be SEND_START
*and* we hit an error making the send, then we remove it from the list?

Again I think this needs to go further and stop using ->status as part
of the FSM too.

Trying again, maybe like this:

	spin_lock_irqsave(&mad_agent_priv->lock, flags);
	if (ib_mad_kernel_rmpp_agent(&mad_agent_priv->agent)) {
		ret = ib_process_rmpp_send_wc(mad_send_wr, mad_send_wc);
		if (ret == IB_RMPP_RESULT_CONSUMED)
			goto done;
	} else
		ret = IB_RMPP_RESULT_UNHANDLED;

	if (mad_send_wr->state == IB_MAD_STATE_SEND_START) {
		if (mad_send_wc->status != IB_WC_SUCCESS &&
		    mad_send_wr->timeout) {
			wait_for_response(mad_send_wr);
			goto done;
		    }
		/* Otherwise error posting send */
		list_del(&mad_send_wr->agent_list);
	}

	WARN_ON(mad_send_wr->state != IB_MAD_STATE_EARLY_RESP &&
		mad_send_wr->state != IB_MAD_STATE_DONE);

	mad_send_wr->state = IB_MAD_STATE_DONE;
	mad_send_wr->status = mad_send_wc->status;
	adjust_timeout(mad_agent_priv);
	spin_unlock_irqrestore(&mad_agent_priv->lock, flags);


Though that might require changing cancel_mad too, as in the other
message, I think with the FSM cancel_mad should put the state to DONE
and leave the status alone.

This status fiddling is probably another patch.

Jason
Or Har-Toov Dec. 10, 2024, 11:51 p.m. UTC | #2 
From: Jason Gunthorpe <jgg@nvidia.com> 
Sent: Tuesday, 10 December 2024 21:01
To: Leon Romanovsky <leon@kernel.org>
Cc: Or Har-Toov <ohartoov@nvidia.com>; linux-rdma@vger.kernel.org; Maher Sanalla <msanalla@nvidia.com>
Subject: Re: [PATCH rdma-next 2/3] IB/mad: Remove unnecessary done list by utilizing MAD states

On Tue, Dec 03, 2024 at 03:52:22PM +0200, Leon Romanovsky wrote:
> From: Or Har-Toov <ohartoov@nvidia.com>
> 
> Remove the done list, which has become unnecessary with the 
> introduction of the `state` parameter.
> 
> Previously, the done list was used to ensure that MADs removed from 
> the wait list would still be in some list, preventing failures in the 
> call to `list_del` in `ib_mad_complete_send_wr`.

Yuk, that is a terrible reason for this. list_del_init() would solve that problem.

> @@ -1772,13 +1771,11 @@ ib_find_send_mad(const struct 
> ib_mad_agent_private *mad_agent_priv,  void ib_mark_mad_done(struct 
> ib_mad_send_wr_private *mad_send_wr)  {
>  	mad_send_wr->timeout = 0;
> -	if (mad_send_wr->state == IB_MAD_STATE_WAIT_RESP) {
> +	list_del(&mad_send_wr->agent_list);

This is doing more than the commit message says, we are now changing the order for when the mad is in the list, here you are removing it as soon as it becomes done, or semi-done instead of letting
ib_mad_complete_send_wr() always remove it.

I couldn't find a reason it is not OK, but I think it should be in the commit message.

>  static void ib_mad_complete_recv(struct ib_mad_agent_private 
> *mad_agent_priv, @@ -2249,7 +2246,9 @@ void ib_mad_complete_send_wr(struct ib_mad_send_wr_private *mad_send_wr,
>  	}
>  
>  	/* Remove send from MAD agent and notify client of completion */
> -	list_del(&mad_send_wr->agent_list);
> +	if (mad_send_wr->state == IB_MAD_STATE_SEND_START)
> +		list_del(&mad_send_wr->agent_list);
> +

This extra if is confusing now.. There are two callers to ib_mad_complete_send_wr(), the receive path did ib_mark_mad_done() first so state should be DONE or EARLY_RESP and the list_del was done already.

The other one is send completion which should have state be SEND_START
*and* we hit an error making the send, then we remove it from the list?

Again I think this needs to go further and stop using ->status as part of the FSM too.

Trying again, maybe like this:

	spin_lock_irqsave(&mad_agent_priv->lock, flags);
	if (ib_mad_kernel_rmpp_agent(&mad_agent_priv->agent)) {
		ret = ib_process_rmpp_send_wc(mad_send_wr, mad_send_wc);
		if (ret == IB_RMPP_RESULT_CONSUMED)
			goto done;
	} else
		ret = IB_RMPP_RESULT_UNHANDLED;

	if (mad_send_wr->state == IB_MAD_STATE_SEND_START) {
		if (mad_send_wc->status != IB_WC_SUCCESS &&
		    mad_send_wr->timeout) {
			wait_for_response(mad_send_wr);
			goto done;
		    }
		/* Otherwise error posting send */
		list_del(&mad_send_wr->agent_list);
	}

	WARN_ON(mad_send_wr->state != IB_MAD_STATE_EARLY_RESP &&
		mad_send_wr->state != IB_MAD_STATE_DONE);

	mad_send_wr->state = IB_MAD_STATE_DONE;
	mad_send_wr->status = mad_send_wc->status;
	adjust_timeout(mad_agent_priv);
	spin_unlock_irqrestore(&mad_agent_priv->lock, flags);


Though that might require changing cancel_mad too, as in the other message, I think with the FSM cancel_mad should put the state to DONE and leave the status alone.

>>OH: Wouldn't it be better to remove the status from the wr entirely and add a state of "canceled"? We can't just put the state to be DONE in cancel_mad cause we need to know to set the wc status to FLUSH_ERR

This status fiddling is probably another patch.

Jason
diff mbox series

Patch

diff --git a/drivers/infiniband/core/mad.c b/drivers/infiniband/core/mad.c
index 9b101f91ca3e..e16bc396f6bc 100644
--- a/drivers/infiniband/core/mad.c
+++ b/drivers/infiniband/core/mad.c
@@ -391,7 +391,6 @@  struct ib_mad_agent *ib_register_mad_agent(struct ib_device *device,
 	spin_lock_init(&mad_agent_priv->lock);
 	INIT_LIST_HEAD(&mad_agent_priv->send_list);
 	INIT_LIST_HEAD(&mad_agent_priv->wait_list);
-	INIT_LIST_HEAD(&mad_agent_priv->done_list);
 	INIT_LIST_HEAD(&mad_agent_priv->rmpp_list);
 	INIT_DELAYED_WORK(&mad_agent_priv->timed_work, timeout_sends);
 	INIT_LIST_HEAD(&mad_agent_priv->local_list);
@@ -1772,13 +1771,11 @@  ib_find_send_mad(const struct ib_mad_agent_private *mad_agent_priv,
 void ib_mark_mad_done(struct ib_mad_send_wr_private *mad_send_wr)
 {
 	mad_send_wr->timeout = 0;
-	if (mad_send_wr->state == IB_MAD_STATE_WAIT_RESP) {
+	list_del(&mad_send_wr->agent_list);
+	if (mad_send_wr->state == IB_MAD_STATE_WAIT_RESP)
 		mad_send_wr->state = IB_MAD_STATE_DONE;
-		list_move_tail(&mad_send_wr->agent_list,
-			      &mad_send_wr->mad_agent_priv->done_list);
-	} else {
+	else
 		mad_send_wr->state = IB_MAD_STATE_EARLY_RESP;
-	}
 }
 
 static void ib_mad_complete_recv(struct ib_mad_agent_private *mad_agent_priv,
@@ -2249,7 +2246,9 @@  void ib_mad_complete_send_wr(struct ib_mad_send_wr_private *mad_send_wr,
 	}
 
 	/* Remove send from MAD agent and notify client of completion */
-	list_del(&mad_send_wr->agent_list);
+	if (mad_send_wr->state == IB_MAD_STATE_SEND_START)
+		list_del(&mad_send_wr->agent_list);
+
 	adjust_timeout(mad_agent_priv);
 	spin_unlock_irqrestore(&mad_agent_priv->lock, flags);
 
diff --git a/drivers/infiniband/core/mad_priv.h b/drivers/infiniband/core/mad_priv.h
index cc2de81ea6f6..4af63c1664c2 100644
--- a/drivers/infiniband/core/mad_priv.h
+++ b/drivers/infiniband/core/mad_priv.h
@@ -96,7 +96,6 @@  struct ib_mad_agent_private {
 	spinlock_t lock;
 	struct list_head send_list;
 	struct list_head wait_list;
-	struct list_head done_list;
 	struct delayed_work timed_work;
 	unsigned long timeout;
 	struct list_head local_list;