Liang,
Would you please comment on this ticket?
Thank you,
James
Hi James, to narrow down the problem, have you ever seen this issue with other versions between 2.4 and 2.6? thanks
I see the reason here, I think it's because this patch changed returned value of lnet_post_routed_recv_locked() from positive to negative (http://review.whamcloud.com/#/c/9369/)
which means lnet_parse_forward_locked()->lnet_post_routed_recv_locked() will return -EAGAIN instead of EAGAIN, and it will be treated as real error in lnet_parse, although it is not a real error because EAGAIN means message is waiting for router buffer, and we will try to finalise a message which is still queued:
if (!for_me) {
rc = lnet_parse_forward_locked(ni, msg);
lnet_net_unlock(cpt);
if (rc < 0)
goto free_drop;
if (rc == 0) {
lnet_ni_recv(ni, msg->msg_private, msg, 0,
0, payload_length, payload_length);
}
return 0;
}
I have only seen the problem with 2.6.
patch is here: http://review.whamcloud.com/#/c/10625/
I changed it from EAGAIN to -EAGAIN so it matches the behavior in the upstream kernel. It is frowned on to use EAGAIN in kernel space. How about instead we just do:
diff --git a/lnet/lnet/lib-move.c b/lnet/lnet/lib-move.c
index 6097ae0..5fcc19b 100644
— a/lnet/lnet/lib-move.c
+++ b/lnet/lnet/lib-move.c
@@ -1961,12 +1961,14 @@ lnet_parse(lnet_ni_t *ni, lnet_hdr_t *hdr, lnet_nid_t from_nid,
rc = lnet_parse_forward_locked(ni, msg);
lnet_net_unlock(cpt);
- if (rc < 0)
+ if (rc == -EAGAIN) /* waiting for buffer */
+ return 0;
+
+ if (rc != 0)
goto free_drop; - if (rc == 0)
{
- lnet_ni_recv(ni, msg->msg_private, msg, 0,
- 0, payload_length, payload_length);
- }
+
+ lnet_ni_recv(ni, msg->msg_private, msg, 0,
+ 0, payload_length, payload_length);
return 0;
}
I tired the above version of my patch and it resolved the issue. Do you Liang mind if we go with that version instead.
Hi James, both way should be fine, my concern about for -EAGAIN is, we need to impose another condition to LND: -EAGAIN should never returned by lnet_ni_eager_recv.
It is true for now, but it's probably better not to have this limit?
I wouldn't consider that a huge limitation. The LASSERT in lnet_ni_eager_recv would handle this case today. So a potential driver writer would know not to return a -EAGAIN.
I see their are strong opinions on this. I purpose that if you want to continue positive values in some of the function that we call them something else besides EAGAIN and ENOENT. This will be flagged by the HPPD checker and upstream using those values will be frowned on. So I suggest you define your own errors so EAGAIN will become LNET_RETRY and ENOENT becomes LNET_MISMATCH. Can you live with this compromise?
sorry I didn't notice there is update and review comments on patch, and overwrote it.
After rethink, I agree with Isaac and still tend to keep positive value, it will be much cleaner to me.
James, thanks for reporting this problem. We have seen this at Cray as well, of course, and only on routers as you had. We have been working around the problem by using b2_5 vintage routers.
Thank you Liang for working with me on this issue. IMHO the new LNET_CREDIT_* values make it far more clear what is going on than using EAGAIN or just returning zero.
welcome, it's assigned to me anyway 
. I think we can close it now because it's landed which is the only place we have this issue.
Patch landed to Master.