Re: [Qemu-devel] [PATCH V7 0/5] Send the gratuitous by guest

[Jason Wang <jasowang@redhat.com>]

On 03/07/2013 06:52 PM, Michael S. Tsirkin wrote:
> On Thu, Mar 07, 2013 at 06:33:30PM +0800, Jason Wang wrote:
>> On 03/07/2013 06:25 PM, Michael S. Tsirkin wrote:
>>> On Thu, Mar 07, 2013 at 06:13:41PM +0800, Jason Wang wrote:
>>>> On 03/07/2013 06:04 PM, Michael S. Tsirkin wrote:
>>>>> On Thu, Mar 07, 2013 at 04:23:46PM +0800, Jason Wang wrote:
>>>>>> This series tries to let guest instead of qemu to send the gratuitous packets
>>>>>> after migration when guest is capable of doing this. This is needed since it's
>>>>>> impossible for qemu to keep track of all configurations (e.g 802.1Q) and mac
>>>>>> addresses (more than one mac address may be used by guest). So qemu can't build
>>>>>> gratuitous packets for all those configurations properly. The only solution is
>>>>>> let guest driver who knew all needed information to do this.
>>>>>>
>>>>>> The series first introduces a new runstate which just tracks the state when the
>>>>>> migration is finished and guest is about to start. And then we can just trying
>>>>>> to notify the guest to send the GARP after changing from this state to
>>>>>> running. A model specific announcing method were also also introduced to let
>>>>>> each kinds of nic do its own notification. When there's no such method register
>>>>>> for the nic, the old style of sending RARP were kept. And the last two patches
>>>>>> implemented the virtio-net method of notification.
>>>>> Do we want to retry SELF_ANNOUNCE_ROUNDS?
>>>> Yes, we do the announcement several times like in the past.
>>>>>> Changes from V6:
>>>>>> - introduce a new runstate instead of using a global variable check the state
>>>>>>
>>>>>> Changes from V5:
>>>>>> - use a global variable to decide whether an announcement is needed after migration
>>>>>> - align with virtio spec and let guest ack the announcement notification through
>>>>>>   control vq instead of config status writing
>>>>>>
>>>>>> Changes from V4:
>>>>>> - keep the old behavior that send the gratuitous packets only after migration
>>>>> I wonder why it's a sane thing to do. How about simply sending the event after load?
>>>> The aim is to limit the change of the behaviour to focus on migration.
>>>> We may also need this after cont,
>>> Hmm why after cont?
>> If we stop the vm for a long period, the mac will be missed in the
>> forward table of the bridge also.
> Hmm okay, needs some thought.
>
>>>> and then maybe we can just do this
>>>> unconditionally in vm_start().
>>> OK but then the new infrastructure we are adding will be dead code,
>>> won't it?
>> If we do this, there's no need to introduce a new state then.
>>> Can we do this simply using a post load hook for now?
>> Maybe not, this means we may want to inject an interrupt to guest when
>> vm is not running.
> What I'm suggesting is basically:
> - set some per device flag on load
> - announce based on vmstart if flag is set

The only difference is how to figure out the correct time to do the
announcement. My proposal is using a global runstate  but yours is using
both device specific method and global changes (and may also lead the
garp to be sent after loadvm).
> We can drop the flag later if we want it on every vmstart.

If we want it on every vmstart, we can just register per-deivce vmstate
change handler to do this.
>
>>>>>> - decide whether to send gratuitous packets by previous runstate instead of a dedicated parameter
>>>>>> - check virtio_net_started() instead of VIRTIO_NET_S_LINK_UP before issue the config update interrupt
>>>>>> - move VIRTIO_NET_S_ANNOUNCE to 0x100 and supress guest config write to RO bits
>>>>>> - cleanups suggested by Michael
>>>>>>
>>>>>> Tested with migration within 802.1Q.
>>>>>>
>>>>>> Jason Wang (5):
>>>>>>   runstate: introduce prelaunch-migrate state
>>>>>>   net: announce self after vm is started
>>>>>>   net: model specific announcing support
>>>>>>   virtio-net: notify guest to annouce itself
>>>>>>   virtio-net: compat guest announce
>>>>>>
>>>>>>  hw/pc.h           |    6 +++++-
>>>>>>  hw/virtio-net.c   |   30 ++++++++++++++++++++++++++++++
>>>>>>  hw/virtio-net.h   |   15 ++++++++++++++-
>>>>>>  include/net/net.h |    2 ++
>>>>>>  migration.c       |    4 +---
>>>>>>  qapi-schema.json  |    5 ++++-
>>>>>>  savevm.c          |    8 ++++++--
>>>>>>  vl.c              |    8 +++++++-
>>>>>>  8 files changed, 69 insertions(+), 9 deletions(-)