Increase size of receive buffer #365
Conversation
…ishvananda#354" This reverts commit 028453c.
|
@vishvananda, @fcrisciani, @aboch, @paravmellanox - PTAL |
|
ping |
While waiting for merge of vishvananda/netlink#365 which is needed to read vlan info of VFs using PF - switching to fork containing required patch.
While waiting for merge of vishvananda/netlink#365 which is needed to read vlan info of VFs using PF - switching to fork containing required patch.
While waiting for merge of vishvananda/netlink#365 which is needed to read vlan info of VFs using PF - switching to fork containing required patch.
|
@paravmellanox can you confirm or deny that this resolves an issue in #354? |
nl/nl_linux.go
Outdated
| // Set a big temporary buffer because returned message from kernel can | ||
| // be quite verbose if kernel is asked e.g. for statistics, | ||
| // tc rules or filters, or other more memory requiring data. | ||
| rb := make([]byte, 65536) |
There was a problem hiding this comment.
@jellonek, I will test later today.
There was a problem hiding this comment.
Please define a constant for this.
There was a problem hiding this comment.
@jellonek , sorry for the late response. Something came up.
I tested now with 100 VFs invalid request error is not present with this change.
Please define a const and also add comment there.
There was a problem hiding this comment.
Thx for the test.
Const added and commented.
nl/nl_linux.go
Outdated
| // Set a big temporary buffer because returned message from kernel can | ||
| // be quite verbose if kernel is asked e.g. for statistics, | ||
| // tc rules or filters, or other more memory requiring data. | ||
| rb := make([]byte, 65536) |
There was a problem hiding this comment.
Please define a constant for this.
Closes vishvananda#354 Previous attemt to fix vishvananda#354 was only hiding a true issue with too small buffer to pick up the message from kernel. According to vishvananda#354 (comment) such situation could occur not only during dump of VF list, but also * statistics * tc rules and tc filters * large conn track dump * rdma resource details dump for debugging or any other place where kernel can return more data than default (4kB) sized buffer could hold. iproute2 in this case for rtnl_dump_filter_l has buffer with size of 16kB, but we don't have distinction between different receiving funcs, so I'm proposing to stick with original issue cause finder (kudos to Parav Pandit aka paravmellanox) who is proposing 64kB as a buffer size.
|
LGTM |
|
Considering @aboch LGTM also, will merge |
|
Actually @jellonek can you squash in 1 commmit? |
|
I would like to have revert in a separate commit. That seems to be more logical and more standard in git world. What would be the purpose of squashing them to single one? |
|
I also prefer two commits as they are unrelated. When in future when we address truncated response, it will be logical to refer to 64K limit commit. |
|
@aboch I see your LGTM, but review is still blocked by you ;) |
|
sure no prob, did not notice that was a revert of a previous patch |
|
comments got addressed and LGTM given, merging it |
|
This change is quite painful for performance - allocating then discarding 64K on every call drives the garbage-collector to run faster. I'll open an issue to discuss. |
|
In PR description i have noted what needs to be done to correctly support smaller packages - we need to handle truncate flag (msg.msg_flags & MSG_TRUNC). |
Closes #354
Previous attemt to fix #354 was only hiding a true issue with too small buffer to pick up the message from kernel.
According to #354 (comment)
such situation could occur not only during dump of VF list, but also when we are asking for:
or any other place where kernel can return more data than default (4kB) sized buffer could hold.
iproute2 in this case for rtnl_dump_filter_l has buffer with size of 16kB, but we don't have distinction between different receiving funcs, so I'm proposing to stick with original issue cause finder (kudos to Parav Pandit aka paravmellanox) who is proposing 64kB as a buffer size.
That's improvement in handling of received packets which should keep us safe with issues like #354 but the real solution should be to add in future handling of truncate flag (msg.msg_flags & MSG_TRUNC) if kernel wants to sent us something even bigger than 64k.