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

[Leno Hou]

On 2016年01月14日 04:05, Laine Stump wrote:
> 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?
See [libvirt] [PATCH v4] libvirtd: Increase NL buffer size for lots of 
interface

please let me know if you have more comments, Thanks

-Leno Hou

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