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