From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39977) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VOo45-0003gG-Ad for qemu-devel@nongnu.org; Wed, 25 Sep 2013 08:19:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VOo3z-0004PO-AQ for qemu-devel@nongnu.org; Wed, 25 Sep 2013 08:19:33 -0400 Received: from mx1.redhat.com ([209.132.183.28]:7119) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VOo3z-0004PG-2E for qemu-devel@nongnu.org; Wed, 25 Sep 2013 08:19:27 -0400 Date: Wed, 25 Sep 2013 15:21:35 +0300 From: "Michael S. Tsirkin" Message-ID: <20130925122135.GB19162@redhat.com> References: <20130922085920.GB32658@redhat.com> <20130925093127.GB6320@redhat.com> <20130925100631.GB7876@redhat.com> <20130925105335.GB17439@redhat.com> <87eh8d6sfv.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87eh8d6sfv.fsf@blackfin.pond.sub.org> Subject: Re: [Qemu-devel] [RFC] sync NIC's MAC maintained in NICConf as soon as emualted NIC's MAC changed in guest List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: "chenliang (T)" , "Huangweidong (C)" , Gleb Natapov , Luonengjun , Jason Wang , "paolo.bonzini@gmail.com" , qemu-devel , Xiahai , "Zhanghaoyu (A)" On Wed, Sep 25, 2013 at 01:39:48PM +0200, Markus Armbruster wrote: > "Michael S. Tsirkin" writes: > > > On Wed, Sep 25, 2013 at 10:14:49AM +0000, Zhanghaoyu (A) wrote: > >> >> >> >> Hi, all > >> >> >> >> > >> >> >> >> Do live migration if emulated NIC's MAC has been changed, RARP > >> >> >> >> with wrong MAC address will broadcast via > >> >> >> >> qemu_announce_self in destination, so, long time network > >> >> >> >> disconnection probably happen. > >> >> >> > > >> >> >> >Good catch. > >> >> >> > > >> >> >> >> I want to do below works to resolve this problem, 1. change > >> >> >> >> NICConf's MAC as soon as emulated NIC's MAC changed in guest > >> >> >> > > >> >> >> >This will make it impossible to revert it correctly on reset, won't it? > >> >> >> > > >> >> >> You are right. > >> >> >> virsh reboot , or virsh reset , or reboot VM > >> >> >> from guest, will revert emulated NIC's MAC to original one > >> >> >> maintained in NICConf. > >> >> >> During the reboot/reset flow in qemu, emulated NIC's reset handler > >> >> >> will sync the MAC address in NICConf to the MAC address in > >> >> >> emulated NIC structure, e.g., virtio_net_reset sync the MAC > >> >> >> address in NICConf to VirtIONet'mac. > >> >> >> > >> >> >> BTW, in native scenario, reboot will revert the changed MAC to > >> >> >> original one, too. > >> >> >> > >> >> >> >> 2. sync NIC's (more precisely, queue) MAC to corresponding > >> >> >> >> NICConf in NIC's migration load handler > >> >> >> >> > >> >> >> >> Any better ideas? > >> >> >> >> > >> >> >> >> Thanks, > >> >> >> >> Zhang Haoyu > >> >> >> > > >> >> >> >I think announce needs to poke at the current MAC instead of > >> >> >> > the default one in NICConf. > >> >> >> >We can make it respect link down state while we are at it. > >> >> >> > > >> >> >> NICConf structures are incorporated in different emulated NIC's > >> >> >> structure, e.g., VirtIONet, E1000State_st, RTL8139State, etc., > >> >> >> since so many kinds of emulated NICs, they are described by > >> >> >> different structures, how to find all NICs' current MAC? > >> >> >> > >> >> >> Maybe we can introduce a pointer member 'current_mac' to NICConf > >> >> >> structure, which points to the current MAC, then we can find > >> >> >> all current MACs from NICConf.current_mac. > >> >> > > >> >> >I wouldn't make it a pointer, just a buffer with the mac, copy it there. > >> >> >Maybe call it "softmac" that's what it is really. > >> >> > > >> >> >> Can we broadcast the RARP with current MAC in NIC's migration > >> >> >> load handler respectively? > >> >> >> > >> >> >> Thanks, > >> >> >> Zhang Haoyu > >> >> > > >> >> >It's not so simple, you need to retry several times. > >> >> > > >> >> Could you make a statement for 'retry several times' ? > >> >> Is it the process of retrying several times to sending RARP in > >> >> qemu_announce_self_once? > >> > > >> >yes > >> > > >> >> 'broadcast the RARP with current MAC in NIC's migration load handler > >> >> respectively' is distributing the job of what qemu_announce_self > >> >> does to every NIC's migration load handler, e.g., in virtio NIC's > >> >> migration load handler virtio_net_load, we can create a timer to > >> >> retry several times to send ARAP with current MAC for this NIC, > >> >> just as same as qemu_announce_self does. > >> > > >> >I don't see a lot of value in this yet. > >> > > >> In my opinion, it's not so good to introduce a 'softmac' member to > >> NICConf, which is not essential function of NICConf. > > > > Maybe not essential but 100% of hardware we emulate supports softmacs. > > Yes, but NICConf is about NIC *configuration*, not random common NIC > state. > > We can capture common NIC state in a separate, properly named data type. > > If we want to bunch it together with common configuration in NICConf > instead, then better rename NICConf to something that actually reflects > its changed purpose. I doubt this would be a good idea. I agree, it should go into NetClientState, not NICConf. My main point is it's a common thing, let's not duplicate code. > >> And, distributing the job of what qemu_announce_self does to every > >> NIC's migration load handler has no disadvantages over > >> qemu_announce_self, > > > > I see some disadvantages, yes. > > You are going to add code to all devices instead of > > doing it in one place, there better be a good reason for this. > > Keeping code common to many (most?) NICs factored out makes sense. > We've started doing that for block devices, in hw/block/block.c. So > far, the only code there is about configuration, thus we work with > BlockConf. > > [...]