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

[Leno Hou]

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 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
>