Re: [libvirt] [PATCH v3] libvirtd: Increase NL buffer size for lots of interface

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(a)linux.vnet.ibm.com&gt; >>>> Cc: Wenyi Gao <wenyi(a)linux.vnet.ibm.com&gt; >>>> CC: Laine Stump <laine(a)laine.org&gt; >>>> CC: Michal Privoznik <mprivozn(a)redhat.com&gt; >>>> --- >>>> 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(a)redhat.com >>>> https://www.redhat.com/mailman/listinfo/libvir-list >> > >