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

On 12/18/2015 02:30 AM, Leno Hou wrote: > > On 2015年12月17日 02:33, Laine Stump wrote: >> On 12/16/2015 10:24 AM, Michal Privoznik wrote: >>> On 10.12.2015 07:34, 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 set socket buffer size to 128K and turn on message >>>> peeking for nl_recv, >>>> as this would solve this problem totally and permanetly. >>>> >> >>>> Signed-off-by: Leno Hou <houqy(a)linux.vnet.ibm.com&gt; >>>> Cc: Wenyi Gao <wenyi(a)linux.vnet.ibm.com&gt; >>>> --- >>>> src/util/virnetlink.c | 8 ++++++++ >>>> 1 file changed, 8 insertions(+) >>>> >>>> diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c >>>> index 679b48e..c8c9fe0 100644 >>>> --- a/src/util/virnetlink.c >>>> +++ b/src/util/virnetlink.c >>>> @@ -696,6 +696,14 @@ virNetlinkEventServiceStart(unsigned int >>>> protocol, unsigned int groups) >>>> goto error_server; >>>> } >>>> + if (nl_socket_set_buffer_size(srv->netlinknh, 131702, 0) < >>>> 0) { >> >> The above function doesn't exist in libnl 1.1 (still used in >> RHEL6/CentOS6, for example), so that would cause a build failure on >> some systems. In libnl 1.1 the function is called nl_set_buffer_size(). >> >> Also, how did you arrive at 128k for the default buffer size? What >> kind of sizes are you seeing? > This buffer size is the receive socket buffer size. When I switching > CPUs to offline/online, it's receives 107K. Wow! What is in this message, and where/how is it used in the code? > >> >> >>>> + virReportSystemError(errno, >>>> + "%s",_("cannot set netlink socket buffer size to >>>> 128k")); >>>> + goto error_server; >>>> + } >>>> + >>>> + nl_socket_enable_msg_peek(srv->netlinknh); >>>> + >> >> According to a link I followed from another message on this topic >> last week, libnl's message peeking can't be guaranteed to always >> work, because netlink doesn't always return the proper buffer size >> (depending on version). > I would not use message peeking due to I agree with you on that > message peeking can't be guaranteed to always work. I'm hoping that is only for "older" versions of kernel/netlink/libnl. Thomas? >> >>>> if ((srv->eventwatch = virEventAddHandle(fd, >>>> VIR_EVENT_HANDLE_READABLE, >>>> virNetlinkEventCallback, >>>> >>> I believe this patch appears over and over again. Usually, the problem >>> was in libnl library we use and this was just a workaround. Can you >>> test >>> with the latest libnl version (probably even GIT HEAD) and see if that >>> helps? >> >> I had the same memory. So I just looked back through the history of >> bug reports about this issue, and found the following: >> >> * libnl-1.1 and libnl-3 both originally set the default message >> buffersize to 4096 bytes, with MSG_PEEK turned off. >> >> * when this problem came up in RHEL6, it was unfortunately reported >> as a private BZ (a pet peeve of mine), and the result of the >> discussion about it was that libnl-1.1 (the version used in RHEL6) >> was patched *upstream* to set the default message buffersize to >> 16384 bytes (getpagesize() * 4), which would solve the problem for >> even very large numbers of VFs. That was in 2013 and there have been >> no further reports against RHEL6. >> >> * Although I had assumed the problem was solved, it again came up in >> RHEL7 (which uses libnl-3 - a slightly different API, and maintained >> in a separate git repo), this time in a public BZ: >> >> https://bugzilla.redhat.com/show_bug.cgi?id=1040626 > I'm not familar with this bug :-( I think this maybe different > from my case. Well, I wasn't even aware that libvirt's netlink service was used for CPU hotplug (and still don't see how/where that happens) :-) The above bug reports were due to 4096 bytes not being enough to read an entire IFLA_VF_PORTS array. > >> >> I asked if perhaps the change that had been made upstream in >> libnl-1.1 hadn't also been made to libnl-3 (this is what I assumed >> during the previous incident). It hadn't. So the same change was >> made for libnl-3, both upstream and as a backport to RHEL7, and >> everyone was happy. > >> I have very little detailed memory of that time (the above was all >> recalled by looking at archives of discussions) but what had stuck >> in my mind was "This problem has been fixed in libnl, so libvirt >> should NOT put in "workarounds" for broken versions of libnl." >> >> But if you are using a version of libnl3 with this patch (which was >> in libnl-3.2.22 upstream, and is in the libnl-3.2.21 that's in >> RHEL7.0+), : >> >> https://github.com/tgraf/libnl/commit/807fddc4cd9ecb12ba64e1b7fa26d86b6c2... >> > This patch sets *message* buffer size to 4 pages. But I'm prefer to > say I wanna to set *socket* buffer size to 128k > >> >> then the change to quadruple the buffer size in libnl was >> insufficient, (and also, when I looked back at the discussion now, > >> I see that the libnl maintainer had said "The permanent fix would be >> for libvirt to enable message peeking", so I suppose it's time to >> "bite the bullet" and enable netlink message > Yes, I would to set socket buffer size in libvirt because If we can > set socket buffer size in libnl, it's can give impact on the app that > do not need 128K socket buffer size. Yes. 16K is one thing, but 128K is quite a lot more, and could more easily cause a problem with other applications. > >> peeking in libvirt (but, since there are apparently versions of >> netlink that don't properly inform libnl when a re-read is >> necessary, we also need to increase the default buffer size). >> >> However, your patch is only fixing the problem in one place. There >> are several places that we allocate netlink sockets, and they should >> all get the same fix, implying that there should be a common >> function called by all three. Fortunately, we already have a macro >> called "virNetlinkAlloc" that is #defined differently depending on >> the libnl version - this macro can simply be made into a static >> function that is defined differently depending on libnl version. It >> can call nl_handle_alloc or nl_socket_alloc depending on libnl >> version, then call nl_socket_set_buffer_size/nl_set_buffer_size >> depending on version, and finally call nl_socket_enable_msg_peek. >> >> A bit of due diligence about the default buffer size is in order >> though - just to make sure that we don't open a ton of netlink >> sockets at the same time, each with an unnecessarily huge buffer. >> > Does anyone has an good idea to solve this problem permanently? i.e. > Dynamically allocate the socket buffer size accordingly. Thanks > I *think* (based on the things I've read, not on personal experience) that sufficiently new versions of libnl and kernel *do* properly implement message peeking, so we can just start off with a buffer size larger than what we think will ever be needed on a current system, and also enable message peeking, which should future-proof the code (so, basically what you've done). I'm Cc'ing Thomas Graf (from libnl) for his opinion though.