Re: [Qemu-devel] [PATCH] net: Update netdev peer on link change

[Vlad Yasevich]

On 04/02/2014 11:29 AM, Michael S. Tsirkin wrote:
> On Wed, Apr 02, 2014 at 11:25:32AM -0400, Vlad Yasevich wrote:
>> On 04/02/2014 11:03 AM, Michael S. Tsirkin wrote:
>>> On Wed, Apr 02, 2014 at 10:57:08AM -0400, Vlad Yasevich wrote:
>>>> On 04/02/2014 06:46 AM, Yan Vugenfirer wrote:
>>>>>
>>>>> On Apr 2, 2014, at 11:51 AM, Michael S. Tsirkin <address@hidden> wrote:
>>>>>
>>>>>> On Thu, Nov 21, 2013 at 09:05:51PM -0500, Vlad Yasevich wrote:
>>>>>>> When a link change occurs on a backend (like tap), we currently do
>>>>>>> not propage such change to the nic.  As a result, when someone turns
>>>>>>> off a link on a tap device, for instance, then a guest doesn't see
>>>>>>> that change and continues to try to send traffic or run DHCP even
>>>>>>> though the lower-layer is disconnected.  This is OK when the network
>>>>>>> is set up as a HUB since the the guest may be connected to other HUB
>>>>>>> ports too, but when it's set up as a netdev, it makes thinkgs worse.
>>>>>>>
>>>>>>> The patch addresses this by setting the peers link down only when the
>>>>>>> peer is not a HUBPORT device.  With this patch, in the following config
>>>>>>>  -netdev tap,id=net0 -device e1000,mac=XXXXX,netdev=net0
>>>>>>> when net0 link is turned off, the guest e1000 shows lower-layer link
>>>>>>> down. This allows guests to boot much faster in such configurations.
>>>>>>> With windows guest, it also allows the network to recover properly
>>>>>>> since windows will not configure the link-local IPv4 address, and
>>>>>>> when the link is turned on, the proper address address is configured.
>>>>>>>
>>>>>>> Signed-off-by: Vlad Yasevich <address@hidden>
>>>>>>> ---
>>>>>>> net/net.c | 26 +++++++++++++++++---------
>>>>>>> 1 file changed, 17 insertions(+), 9 deletions(-)
>>>>>>>
>>>>>>> diff --git a/net/net.c b/net/net.c
>>>>>>> index 0a88e68..8a084bf 100644
>>>>>>> --- a/net/net.c
>>>>>>> +++ b/net/net.c
>>>>>>> @@ -1065,15 +1065,23 @@ void qmp_set_link(const char *name, bool up, 
>>>>>>> Error **errp)
>>>>>>>         nc->info->link_status_changed(nc);
>>>>>>>     }
>>>>>>>
>>>>>>> -    /* Notify peer. Don't update peer link status: this makes it 
>>>>>>> possible to
>>>>>>> -     * disconnect from host network without notifying the guest.
>>>>>>> -     * FIXME: is disconnected link status change operation useful?
>>>>>>> -     *
>>>>>>> -     * Current behaviour is compatible with qemu vlans where there 
>>>>>>> could be
>>>>>>> -     * multiple clients that can still communicate with each other in
>>>>>>> -     * disconnected mode. For now maintain this compatibility. */
>>>>>>
>>>>>> Hmm this went in without much discussion.
>>>>>>
>>>>>> But before this change went in, it was possible to control link state on 
>>>>>> NIC
>>>>>> and on link separately, without guest noticing.
>>>>>>
>>>>>> Why did we decide to drop this functionality?
>>>>>
>>>>> Not sure there was a real need for the patch.
>>>>>
>>>>> On other hand Windows guest will not receive IP address from DHCP server 
>>>>> if you start VM with tap link down and then set it to up without toggling 
>>>>> link state of the guest device as well.
>>>>
>>>> Same for a linux guest.  It a fault scenario.  So we either handle
>>>> it by propagating the link event, or we forbid it.  Doing neither
>>>> allows for a bad state in common configuration.
>>>
>>> It's how networking works.
>>> If I turn off my router at home I can't get dhcp either.
>>
>> And you device link will show that it has not carrier.
> 
> It doesn't as long as the switch it's connected to is up.
> 
> 
>>>
>>> We had a way to disable networking  at link or at
>>> the router, then removed the ability to turn it
>>> off at the router and I'm asking why.
>>
>> We didn't remove the ability to turn it off at router/switch.
>> We just now propagate carrier loss correctly.
>>
>> In fact, if you have a configuration where the VM is connected
>> to a hub in qemu, turning off the link on the 'tap' connected
>> to the same hub doesn't not change guest state.
> 
> So why is it a good idea to make -netdev inconsistent
> with this?

Because -netdev has a 1-1 relationship with -device.   You
could think of it a physical wire that connects the nic
to the network.  If something happens to the wire, the
nic should notice it.

> It doesn't seem to add anything useful.
> It might fix things for users who turned off link
> at the wrong place by mistake but I doubt it's
> a common scenario.

I don't think link state management in general is a common scenario, but
we support it.  I think this makes the behavior better and improves
the vm experience.
> 
> Where end users getting it wrong and complaining?

Yes, I can provide you with a list reported bugs if you wish.

-vlad

> 
> 
>>>
>>>> This patch chose to propagate the event under certain configuration.
>>>>
>>>> -vlad
>>>
>>> So don't do this then.
>>> If you want guest to know that link is down,
>>> put it down on guest side.
>>>
>>> It's that simple.
>>>
>>
>> Sorry, but that's not always possible.  The host administrator
>> may not always be vm administrator and without this patch vm
>> administrator has no idea what happened to his network.
>>
>> -vlad
>>>
>>>
>>>
>>>>>
>>>>> Yan. 
>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>> -    if (nc->peer && nc->peer->info->link_status_changed) {
>>>>>>> -        nc->peer->info->link_status_changed(nc->peer);
>>>>>>> +    if (nc->peer) {
>>>>>>> +        /* Change peer link only if the peer is NIC and then notify 
>>>>>>> peer.
>>>>>>> +         * If the peer is a HUBPORT or a backend, we do not change the
>>>>>>> +         * link status.
>>>>>>> +         *
>>>>>>> +         * This behavior is compatible with qemu vlans where there 
>>>>>>> could be
>>>>>>> +         * multiple clients that can still communicate with each other 
>>>>>>> in
>>>>>>> +         * disconnected mode. For now maintain this compatibility.
>>>>>>> +         */
>>>>>>> +        if (nc->peer->info->type == NET_CLIENT_OPTIONS_KIND_NIC) {
>>>>>>> +            for (i = 0; i < queues; i++) {
>>>>>>> +                ncs[i]->peer->link_down = !up;
>>>>>>> +            }
>>>>>>> +        }
>>>>>>> +        if (nc->peer->info->link_status_changed) {
>>>>>>> +            nc->peer->info->link_status_changed(nc->peer);
>>>>>>> +        }
>>>>>>>     }
>>>>>>> }
>>>>>>>
>>>>>>> -- 
>>>>>>> 1.8.4.2
>>>>>>>
>>>>>>
>>>>>