[libvirt] [PATCH v3] libvirtd: Increase NL buffer size for lots of interface
1. When switching CPUs to offline/online in a system more than 128 cpus 2. When using virsh to destroy domain in a system with more interface All of above happens nl_recv returned with error: No buffer space available. This patch sets the socket buffer size to 128K and turns on message peeking for nl_recv,as this would solve this problem totally and permanetly. Signed-off-by: Leno Hou <houqy@linux.vnet.ibm.com> Cc: Wenyi Gao <wenyi@linux.vnet.ibm.com> CC: Laine Stump <laine@laine.org> CC: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnetlink.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 679b48e..ea65cbc 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -65,10 +65,12 @@ struct virNetlinkEventHandle { # ifdef HAVE_LIBNL1 # define virNetlinkAlloc nl_handle_alloc +# define virSocketSetBufferSize nl_set_buffer_size # define virNetlinkFree nl_handle_destroy typedef struct nl_handle virNetlinkHandle; # else # define virNetlinkAlloc nl_socket_alloc +# define virSocketSetBufferSize nl_socket_set_buffer_size # define virNetlinkFree nl_socket_free typedef struct nl_sock virNetlinkHandle; # endif @@ -696,6 +698,14 @@ virNetlinkEventServiceStart(unsigned int protocol, unsigned int groups) goto error_server; } + if (virSocketSetBufferSize(srv->netlinknh, 131702, 0) < 0) { + virReportSystemError(errno, + "%s",_("cannot set netlink socket buffer size to 128k")); + goto error_server; + } + + nl_socket_enable_msg_peek(srv->netlinknh); + if ((srv->eventwatch = virEventAddHandle(fd, VIR_EVENT_HANDLE_READABLE, virNetlinkEventCallback, -- 1.9.1
On Mon, Jan 11, 2016 at 02:59:00PM +0800, Leno Hou wrote:
1. When switching CPUs to offline/online in a system more than 128 cpus 2. When using virsh to destroy domain in a system with more interface
All of above happens nl_recv returned with error: No buffer space available. This patch sets the socket buffer size to 128K and turns on message peeking for nl_recv,as this would solve this problem totally and permanetly.
So if none of the above is true/happening...
Signed-off-by: Leno Hou <houqy@linux.vnet.ibm.com> Cc: Wenyi Gao <wenyi@linux.vnet.ibm.com> CC: Laine Stump <laine@laine.org> CC: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnetlink.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 679b48e..ea65cbc 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -65,10 +65,12 @@ struct virNetlinkEventHandle {
# ifdef HAVE_LIBNL1 # define virNetlinkAlloc nl_handle_alloc +# define virSocketSetBufferSize nl_set_buffer_size # define virNetlinkFree nl_handle_destroy typedef struct nl_handle virNetlinkHandle; # else # define virNetlinkAlloc nl_socket_alloc +# define virSocketSetBufferSize nl_socket_set_buffer_size # define virNetlinkFree nl_socket_free typedef struct nl_sock virNetlinkHandle; # endif @@ -696,6 +698,14 @@ virNetlinkEventServiceStart(unsigned int protocol, unsigned int groups) goto error_server; }
+ if (virSocketSetBufferSize(srv->netlinknh, 131702, 0) < 0) { + virReportSystemError(errno, + "%s",_("cannot set netlink socket buffer size to 128k")); + goto error_server; + } + + nl_socket_enable_msg_peek(srv->netlinknh); +
... shouldn't this be non-fatal just in case?
if ((srv->eventwatch = virEventAddHandle(fd, VIR_EVENT_HANDLE_READABLE, virNetlinkEventCallback, -- 1.9.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 01/11/2016 05:44 AM, Martin Kletzander wrote:
On Mon, Jan 11, 2016 at 02:59:00PM +0800, Leno Hou wrote:
1. When switching CPUs to offline/online in a system more than 128 cpus 2. When using virsh to destroy domain in a system with more interface
All of above happens nl_recv returned with error: No buffer space available. This patch sets the socket buffer size to 128K and turns on message peeking for nl_recv,as this would solve this problem totally and permanetly.
So if none of the above is true/happening...
Signed-off-by: Leno Hou <houqy@linux.vnet.ibm.com> Cc: Wenyi Gao <wenyi@linux.vnet.ibm.com> CC: Laine Stump <laine@laine.org> CC: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnetlink.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 679b48e..ea65cbc 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -65,10 +65,12 @@ struct virNetlinkEventHandle {
# ifdef HAVE_LIBNL1 # define virNetlinkAlloc nl_handle_alloc +# define virSocketSetBufferSize nl_set_buffer_size # define virNetlinkFree nl_handle_destroy typedef struct nl_handle virNetlinkHandle; # else # define virNetlinkAlloc nl_socket_alloc +# define virSocketSetBufferSize nl_socket_set_buffer_size # define virNetlinkFree nl_socket_free typedef struct nl_sock virNetlinkHandle; # endif @@ -696,6 +698,14 @@ virNetlinkEventServiceStart(unsigned int protocol, unsigned int groups) goto error_server; }
+ if (virSocketSetBufferSize(srv->netlinknh, 131702, 0) < 0) { + virReportSystemError(errno, + "%s",_("cannot set netlink socket buffer size to 128k")); + goto error_server; + } + + nl_socket_enable_msg_peek(srv->netlinknh); +
... shouldn't this be non-fatal just in case?
I at first agreed with this [*] if we just issue a warning and continue we would have the least possibility of regression on older systems (or maybe some odd/old system that didn't allow setting a 128k buffer?). But on the other hand, I think the likelyhood of this is very low, and if it *does* happen we (the developers/maintainers) want to know about it. If there's a warning in a log file and libvirt continues to operate, the user isn't likely to report it. If there is an error message and something doesn't work, then we will definitely hear about it. So I think this should remain as an error. Any other opinions? BTW, otherwise ACK on the change - I backported it to libvirt-0.10.2 and it built on RHEL6 (which uses libnl1) without problem. [*](every other error condition in virNetlinkEvenServiceStart() is due to a condition that would make the netlink listener completely non-functional, so it makes sense to shut it down. But if we failed to set the socket buffer size as requested, it would still function on *most* systems.
if ((srv->eventwatch = virEventAddHandle(fd, VIR_EVENT_HANDLE_READABLE, virNetlinkEventCallback, -- 1.9.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Jan 11, 2016 at 02:32:44PM -0500, Laine Stump wrote:
On 01/11/2016 05:44 AM, Martin Kletzander wrote:
On Mon, Jan 11, 2016 at 02:59:00PM +0800, Leno Hou wrote:
1. When switching CPUs to offline/online in a system more than 128 cpus 2. When using virsh to destroy domain in a system with more interface
All of above happens nl_recv returned with error: No buffer space available. This patch sets the socket buffer size to 128K and turns on message peeking for nl_recv,as this would solve this problem totally and permanetly.
So if none of the above is true/happening...
Signed-off-by: Leno Hou <houqy@linux.vnet.ibm.com> Cc: Wenyi Gao <wenyi@linux.vnet.ibm.com> CC: Laine Stump <laine@laine.org> CC: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnetlink.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 679b48e..ea65cbc 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -65,10 +65,12 @@ struct virNetlinkEventHandle {
# ifdef HAVE_LIBNL1 # define virNetlinkAlloc nl_handle_alloc +# define virSocketSetBufferSize nl_set_buffer_size # define virNetlinkFree nl_handle_destroy typedef struct nl_handle virNetlinkHandle; # else # define virNetlinkAlloc nl_socket_alloc +# define virSocketSetBufferSize nl_socket_set_buffer_size # define virNetlinkFree nl_socket_free typedef struct nl_sock virNetlinkHandle; # endif @@ -696,6 +698,14 @@ virNetlinkEventServiceStart(unsigned int protocol, unsigned int groups) goto error_server; }
+ if (virSocketSetBufferSize(srv->netlinknh, 131702, 0) < 0) { + virReportSystemError(errno, + "%s",_("cannot set netlink socket buffer size to 128k")); + goto error_server; + } + + nl_socket_enable_msg_peek(srv->netlinknh); +
... shouldn't this be non-fatal just in case?
I at first agreed with this [*] if we just issue a warning and continue we would have the least possibility of regression on older systems (or maybe some odd/old system that didn't allow setting a 128k buffer?). But on the other hand, I think the likelyhood of this is very low, and if it *does* happen we (the developers/maintainers) want to know about it. If there's a warning in a log file and libvirt continues to operate, the user isn't likely to report it. If there is an error message and something doesn't work, then we will definitely hear about it. So I think this should remain as an error.
Well, we might want to know about it, I just don't see the benefit of it. Anyway, it's ok with me the way it is now as well. I was just wondering. Have a nice day, Martin
Hi Laine Stump, Any other comments about this patch ? If not, could you help me to review and merge ? Thanks in advance ~~ On 2016年01月12日 03:32, Laine Stump wrote:
On 01/11/2016 05:44 AM, Martin Kletzander wrote:
On Mon, Jan 11, 2016 at 02:59:00PM +0800, Leno Hou wrote:
1. When switching CPUs to offline/online in a system more than 128 cpus 2. When using virsh to destroy domain in a system with more interface
All of above happens nl_recv returned with error: No buffer space available. This patch sets the socket buffer size to 128K and turns on message peeking for nl_recv,as this would solve this problem totally and permanetly.
So if none of the above is true/happening...
Signed-off-by: Leno Hou <houqy@linux.vnet.ibm.com> Cc: Wenyi Gao <wenyi@linux.vnet.ibm.com> CC: Laine Stump <laine@laine.org> CC: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnetlink.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 679b48e..ea65cbc 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -65,10 +65,12 @@ struct virNetlinkEventHandle {
# ifdef HAVE_LIBNL1 # define virNetlinkAlloc nl_handle_alloc +# define virSocketSetBufferSize nl_set_buffer_size # define virNetlinkFree nl_handle_destroy typedef struct nl_handle virNetlinkHandle; # else # define virNetlinkAlloc nl_socket_alloc +# define virSocketSetBufferSize nl_socket_set_buffer_size # define virNetlinkFree nl_socket_free typedef struct nl_sock virNetlinkHandle; # endif @@ -696,6 +698,14 @@ virNetlinkEventServiceStart(unsigned int protocol, unsigned int groups) goto error_server; }
+ if (virSocketSetBufferSize(srv->netlinknh, 131702, 0) < 0) { + virReportSystemError(errno, + "%s",_("cannot set netlink socket buffer size to 128k")); + goto error_server; + } + + nl_socket_enable_msg_peek(srv->netlinknh); +
... shouldn't this be non-fatal just in case?
I at first agreed with this [*] if we just issue a warning and continue we would have the least possibility of regression on older systems (or maybe some odd/old system that didn't allow setting a 128k buffer?). But on the other hand, I think the likelyhood of this is very low, and if it *does* happen we (the developers/maintainers) want to know about it. If there's a warning in a log file and libvirt continues to operate, the user isn't likely to report it. If there is an error message and something doesn't work, then we will definitely hear about it. So I think this should remain as an error.
Any other opinions?
BTW, otherwise ACK on the change - I backported it to libvirt-0.10.2 and it built on RHEL6 (which uses libnl1) without problem.
[*](every other error condition in virNetlinkEvenServiceStart() is due to a condition that would make the netlink listener completely non-functional, so it makes sense to shut it down. But if we failed to set the socket buffer size as requested, it would still function on *most* systems.
if ((srv->eventwatch = virEventAddHandle(fd, VIR_EVENT_HANDLE_READABLE, virNetlinkEventCallback, -- 1.9.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 01/13/2016 03:49 AM, Leno Hou wrote:
Hi Laine Stump,
Any other comments about this patch ? If not, could you help me to review and merge ? Thanks in advance ~~
I just noticed that you haven't done anything about the other places in libvirt where netlink sockets are being opened. I would prefer if we had a single patch that fixed them all (that's why I suggested turning the virNetlinkAlloc macro into a function that was defined differently for libnl-3 vs libnl-1, and did all three of 1) create socket, 2) set larger buffer size, and 3) turn on message peeking). Can you modify your patch to do that?
On 2016年01月12日 03:32, Laine Stump wrote:
On 01/11/2016 05:44 AM, Martin Kletzander wrote:
On Mon, Jan 11, 2016 at 02:59:00PM +0800, Leno Hou wrote:
1. When switching CPUs to offline/online in a system more than 128 cpus 2. When using virsh to destroy domain in a system with more interface
All of above happens nl_recv returned with error: No buffer space available. This patch sets the socket buffer size to 128K and turns on message peeking for nl_recv,as this would solve this problem totally and permanetly.
So if none of the above is true/happening...
Signed-off-by: Leno Hou <houqy@linux.vnet.ibm.com> Cc: Wenyi Gao <wenyi@linux.vnet.ibm.com> CC: Laine Stump <laine@laine.org> CC: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnetlink.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 679b48e..ea65cbc 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -65,10 +65,12 @@ struct virNetlinkEventHandle {
# ifdef HAVE_LIBNL1 # define virNetlinkAlloc nl_handle_alloc +# define virSocketSetBufferSize nl_set_buffer_size # define virNetlinkFree nl_handle_destroy typedef struct nl_handle virNetlinkHandle; # else # define virNetlinkAlloc nl_socket_alloc +# define virSocketSetBufferSize nl_socket_set_buffer_size # define virNetlinkFree nl_socket_free typedef struct nl_sock virNetlinkHandle; # endif @@ -696,6 +698,14 @@ virNetlinkEventServiceStart(unsigned int protocol, unsigned int groups) goto error_server; }
+ if (virSocketSetBufferSize(srv->netlinknh, 131702, 0) < 0) { + virReportSystemError(errno, + "%s",_("cannot set netlink socket buffer size to 128k")); + goto error_server; + } + + nl_socket_enable_msg_peek(srv->netlinknh); +
... shouldn't this be non-fatal just in case?
I at first agreed with this [*] if we just issue a warning and continue we would have the least possibility of regression on older systems (or maybe some odd/old system that didn't allow setting a 128k buffer?). But on the other hand, I think the likelyhood of this is very low, and if it *does* happen we (the developers/maintainers) want to know about it. If there's a warning in a log file and libvirt continues to operate, the user isn't likely to report it. If there is an error message and something doesn't work, then we will definitely hear about it. So I think this should remain as an error.
Any other opinions?
BTW, otherwise ACK on the change - I backported it to libvirt-0.10.2 and it built on RHEL6 (which uses libnl1) without problem.
[*](every other error condition in virNetlinkEvenServiceStart() is due to a condition that would make the netlink listener completely non-functional, so it makes sense to shut it down. But if we failed to set the socket buffer size as requested, it would still function on *most* systems.
if ((srv->eventwatch = virEventAddHandle(fd, VIR_EVENT_HANDLE_READABLE, virNetlinkEventCallback, -- 1.9.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 2016年01月14日 04:05, Laine Stump wrote:
On 01/13/2016 03:49 AM, Leno Hou wrote:
Hi Laine Stump,
Any other comments about this patch ? If not, could you help me to review and merge ? Thanks in advance ~~
I just noticed that you haven't done anything about the other places in libvirt where netlink sockets are being opened. I would prefer if we had a single patch that fixed them all (that's why I suggested turning the virNetlinkAlloc macro into a function that was defined differently for libnl-3 vs libnl-1, and did all three of 1) create socket, 2) set larger buffer size, and 3) turn on message peeking).
Can you modify your patch to do that? See [libvirt] [PATCH v4] libvirtd: Increase NL buffer size for lots of interface
please let me know if you have more comments, Thanks -Leno Hou
On 2016年01月12日 03:32, Laine Stump wrote:
On 01/11/2016 05:44 AM, Martin Kletzander wrote:
On Mon, Jan 11, 2016 at 02:59:00PM +0800, Leno Hou wrote:
1. When switching CPUs to offline/online in a system more than 128 cpus 2. When using virsh to destroy domain in a system with more interface
All of above happens nl_recv returned with error: No buffer space available. This patch sets the socket buffer size to 128K and turns on message peeking for nl_recv,as this would solve this problem totally and permanetly.
So if none of the above is true/happening...
Signed-off-by: Leno Hou <houqy@linux.vnet.ibm.com> Cc: Wenyi Gao <wenyi@linux.vnet.ibm.com> CC: Laine Stump <laine@laine.org> CC: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnetlink.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 679b48e..ea65cbc 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -65,10 +65,12 @@ struct virNetlinkEventHandle {
# ifdef HAVE_LIBNL1 # define virNetlinkAlloc nl_handle_alloc +# define virSocketSetBufferSize nl_set_buffer_size # define virNetlinkFree nl_handle_destroy typedef struct nl_handle virNetlinkHandle; # else # define virNetlinkAlloc nl_socket_alloc +# define virSocketSetBufferSize nl_socket_set_buffer_size # define virNetlinkFree nl_socket_free typedef struct nl_sock virNetlinkHandle; # endif @@ -696,6 +698,14 @@ virNetlinkEventServiceStart(unsigned int protocol, unsigned int groups) goto error_server; }
+ if (virSocketSetBufferSize(srv->netlinknh, 131702, 0) < 0) { + virReportSystemError(errno, + "%s",_("cannot set netlink socket buffer size to 128k")); + goto error_server; + } + + nl_socket_enable_msg_peek(srv->netlinknh); +
... shouldn't this be non-fatal just in case?
I at first agreed with this [*] if we just issue a warning and continue we would have the least possibility of regression on older systems (or maybe some odd/old system that didn't allow setting a 128k buffer?). But on the other hand, I think the likelyhood of this is very low, and if it *does* happen we (the developers/maintainers) want to know about it. If there's a warning in a log file and libvirt continues to operate, the user isn't likely to report it. If there is an error message and something doesn't work, then we will definitely hear about it. So I think this should remain as an error.
Any other opinions?
BTW, otherwise ACK on the change - I backported it to libvirt-0.10.2 and it built on RHEL6 (which uses libnl1) without problem.
[*](every other error condition in virNetlinkEvenServiceStart() is due to a condition that would make the netlink listener completely non-functional, so it makes sense to shut it down. But if we failed to set the socket buffer size as requested, it would still function on *most* systems.
if ((srv->eventwatch = virEventAddHandle(fd, VIR_EVENT_HANDLE_READABLE, virNetlinkEventCallback, -- 1.9.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (3)
-
Laine Stump -
Leno Hou -
Martin Kletzander