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

[Laine Stump]

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