| Message ID | 1443633873-13359-6-git-send-email-santosh.shilimkar@oracle.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
From: Santosh Shilimkar <santosh.shilimkar@oracle.com> Date: Wed, 30 Sep 2015 13:24:24 -0400 > @@ -423,7 +423,9 @@ over_batch: > !list_empty(&conn->c_send_queue)) && > send_gen == conn->c_send_gen) { > rds_stats_inc(s_send_lock_queue_raced); > - goto restart; > + if (batch_count < 1024) > + goto restart; > + queue_delayed_work(rds_wq, &conn->c_send_w, 1); Sorry, you can't just use a magic number like this. You have to describe, in detail, exactly how this value was choosen, derived, and tested to be effeective and in exactly what environment those tests were done. You must also use a mnenomic for this value rather than a raw magic constant. Thanks. -- 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
On 10/5/2015 3:30 AM, David Miller wrote: > From: Santosh Shilimkar <santosh.shilimkar@oracle.com> > Date: Wed, 30 Sep 2015 13:24:24 -0400 > >> @@ -423,7 +423,9 @@ over_batch: >> !list_empty(&conn->c_send_queue)) && >> send_gen == conn->c_send_gen) { >> rds_stats_inc(s_send_lock_queue_raced); >> - goto restart; >> + if (batch_count < 1024) >> + goto restart; >> + queue_delayed_work(rds_wq, &conn->c_send_w, 1); > > Sorry, you can't just use a magic number like this. > > You have to describe, in detail, exactly how this value was > choosen, derived, and tested to be effeective and in exactly > what environment those tests were done. > > You must also use a mnenomic for this value rather than a > raw magic constant. > Right. Infact If I look at it now again, there is already a batch count parameter which is module parameter. I will use that in this hunk as well as the other couple places and send an update. Thanks for review !! 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
diff --git a/net/rds/send.c b/net/rds/send.c index 4df61a5..f1e709c 100644 --- a/net/rds/send.c +++ b/net/rds/send.c @@ -423,7 +423,9 @@ over_batch: !list_empty(&conn->c_send_queue)) && send_gen == conn->c_send_gen) { rds_stats_inc(s_send_lock_queue_raced); - goto restart; + if (batch_count < 1024) + goto restart; + queue_delayed_work(rds_wq, &conn->c_send_w, 1); } } out:
Current process gives up if its send work over the batch limit. The work queue will get kicked to finish off any other requests. This fixes remainder condition from commit 443be0e5affe ("RDS: make sure not to loop forever inside rds_send_xmit"). The restart condition is only for the case where we reached to over_batch code for some other reason so just retrying again before giving up. Signed-off-by: Santosh Shilimkar <ssantosh@kernel.org> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@oracle.com> --- net/rds/send.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)