[ofa-general] [PATCH] RDMA/CMA: Implement rdma_resolve_ip retry enhancement.

[ofa-general] [PATCH] RDMA/CMA: Implement rdma_resolve_ip retry enhancement.

[ggrundstrom]

RDMA/CMA: Implement rdma_resolve_ip retry enhancement.

If an application is calling rdma_resolve_ip() and a status of -ENODATA is returned from addr_resolve_local/remote(), the timeout mechanism waits until the application's timeout occurs before rechecking the address resolution status; the application will wait until it's full timeout occurs.  This case is seen when the work thread call to process_req() is made before the arp packet is processed.

This patch is in addition to Steve Wise's neigh_event_send patch to initiate neighbour discovery sent on 9/12/2007.

Signed-off-by: Glenn Grundstrom <ggrundstrom@neteffect.com>
---
 drivers/infiniband/core/addr.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/core/addr.c b/drivers/infiniband/core/addr.c
index c5c33d3..a953780 100644
--- a/drivers/infiniband/core/addr.c
+++ b/drivers/infiniband/core/addr.c
@@ -55,6 +55,7 @@ struct addr_req {
 	int status;
 };
 
+#define MIN_ADDR_TIMEOUT_MS 500
 static void process_req(struct work_struct *work);
 
 static DEFINE_MUTEX(lock);
@@ -136,6 +137,7 @@ static void set_timeout(unsigned long ti
 static void queue_req(struct addr_req *req)
 {
 	struct addr_req *temp_req;
+	unsigned long req_timeout = msecs_to_jiffies(MIN_ADDR_TIMEOUT_MS) + jiffies;
 
 	mutex_lock(&lock);
 	list_for_each_entry_reverse(temp_req, &req_list, list) {
@@ -145,8 +147,10 @@ static void queue_req(struct addr_req *r
 
 	list_add(&req->list, &temp_req->list);
 
-	if (req_list.next == &req->list)
+	if (req_list.next == &req->list) {
+		req_timeout = min(req_timeout, req->timeout);
 		set_timeout(req->timeout);
+	}
 	mutex_unlock(&lock);
 }
 
@@ -220,6 +224,7 @@ static void process_req(struct work_stru
 	struct addr_req *req, *temp_req;
 	struct sockaddr_in *src_in, *dst_in;
 	struct list_head done_list;
+	unsigned long req_timeout;
 
 	INIT_LIST_HEAD(&done_list);
 
@@ -238,9 +243,11 @@ static void process_req(struct work_stru
 		list_move_tail(&req->list, &done_list);
 	}
 
+	req_timeout = msecs_to_jiffies(MIN_ADDR_TIMEOUT_MS) + jiffies;
 	if (!list_empty(&req_list)) {
 		req = list_entry(req_list.next, struct addr_req, list);
-		set_timeout(req->timeout);
+		req_timeout = min(req_timeout, req->timeout);
+		set_timeout(req_timeout);
 	}
 	mutex_unlock(&lock);
 

Re: [ofa-general] [PATCH] RDMA/CMA: Implement rdma_resolve_ip retry enhancement.

[Roland Dreier]

Thanks for the patch...

 > If an application is calling rdma_resolve_ip() and a status of -ENODATA is returned from addr_resolve_local/remote(), the timeout mechanism waits until the application's timeout occurs before rechecking the address resolution status; the application will wait until it's full timeout occurs.  This case is seen when the work thread call to process_req() is made before the arp packet is processed.

I'm having a hard time understanding this changelog.  Could you please
resend with a description that lets me understand:

 - What the current behavior is and what is wrong with that;
 - What the behavior should be;
 - And how your patch changes the behavior to be correct.

 > This patch is in addition to Steve Wise's neigh_event_send patch to initiate neighbour discovery sent on 9/12/2007.

Does this mean it depends on Steve's patch being applied first?

Also please try to keep lines in the changelog to 72 characters or so...

 > @@ -136,6 +137,7 @@ static void set_timeout(unsigned long ti
 >  static void queue_req(struct addr_req *req)
 >  {
 >  	struct addr_req *temp_req;
 > +	unsigned long req_timeout = msecs_to_jiffies(MIN_ADDR_TIMEOUT_MS) + jiffies;
 >  
 >  	mutex_lock(&lock);
 >  	list_for_each_entry_reverse(temp_req, &req_list, list) {
 > @@ -145,8 +147,10 @@ static void queue_req(struct addr_req *r
 >  
 >  	list_add(&req->list, &temp_req->list);
 >  
 > -	if (req_list.next == &req->list)
 > +	if (req_list.next == &req->list) {
 > +		req_timeout = min(req_timeout, req->timeout);
 >  		set_timeout(req->timeout);
 > +	}
 >  	mutex_unlock(&lock);
 >  }

I don't understand this code.  It seems you keep track of the minimum
timeout, and then ignore the value you computed.  What am I missing?

Thanks,
  Roland

Re: [ofa-general] [PATCH] RDMA/CMA: Implement rdma_resolve_ip retry enhancement.

[Sean Hefty]

> If an application is calling rdma_resolve_ip() and a status of -ENODATA is returned from addr_resolve_local/remote(), the timeout mechanism waits until the application's timeout occurs before rechecking the address resolution status; the application will wait until it's full timeout occurs.  This case is seen when the work thread call to process_req() is made before the arp packet is processed.

I don't understand the issue.  process_req() is invoked whenever a 
network event occurs, which rechecks all pending requests.

> This patch is in addition to Steve Wise's neigh_event_send patch to initiate neighbour discovery sent on 9/12/2007.

This patch looks unrelated to Steve's patch.  Can you clarify the 
relationship?

- Sean

RE: [ofa-general] [PATCH] RDMA/CMA: Implement rdma_resolve_ip retry enhancement.

[Glenn Grundstrom]

> > If an application is calling rdma_resolve_ip() and a status 
> of -ENODATA is returned from addr_resolve_local/remote(), the 
> timeout mechanism waits until the application's timeout 
> occurs before rechecking the address resolution status; the 
> application will wait until it's full timeout occurs.  This 
> case is seen when the work thread call to process_req() is 
> made before the arp packet is processed.
> 
> I don't understand the issue.  process_req() is invoked whenever a 
> network event occurs, which rechecks all pending requests.

Yes, I see the netevent_callback().  I now agree that this patch is not
necessary.

Roland, please disregard.

Glenn.
 
> 
> > This patch is in addition to Steve Wise's neigh_event_send 
> patch to initiate neighbour discovery sent on 9/12/2007.
> 
> This patch looks unrelated to Steve's patch.  Can you clarify the 
> relationship?
> 
> - Sean
>