From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48160) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UoadK-0006zl-L7 for qemu-devel@nongnu.org; Mon, 17 Jun 2013 10:42:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UoadC-00072l-28 for qemu-devel@nongnu.org; Mon, 17 Jun 2013 10:42:14 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59847) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UoadB-00072G-Qy for qemu-devel@nongnu.org; Mon, 17 Jun 2013 10:42:05 -0400 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r5HEg5rx032172 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 17 Jun 2013 10:42:05 -0400 Date: Mon, 17 Jun 2013 17:42:50 +0300 From: "Michael S. Tsirkin" Message-ID: <20130617144250.GE10085@redhat.com> References: <1371195952-13922-1-git-send-email-akong@redhat.com> <20130617091127.444de556@redhat.com> <20130617132131.GB9383@redhat.com> <20130617093042.4e1d6566@redhat.com> <20130617142013.GC10085@redhat.com> <20130617103428.3a1d7db7@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130617103428.3a1d7db7@redhat.com> Subject: Re: [Qemu-devel] [PATCH v6] net: add support of mac-programming over macvtap in QEMU side List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: laine@redhat.com, Amos Kong , qemu-devel@nongnu.org, stefanha@redhat.com On Mon, Jun 17, 2013 at 10:34:28AM -0400, Luiz Capitulino wrote: > On Mon, 17 Jun 2013 17:20:13 +0300 > "Michael S. Tsirkin" wrote: > > > On Mon, Jun 17, 2013 at 09:30:42AM -0400, Luiz Capitulino wrote: > > > On Mon, 17 Jun 2013 16:21:31 +0300 > > > "Michael S. Tsirkin" wrote: > > > > > > > On Mon, Jun 17, 2013 at 09:11:27AM -0400, Luiz Capitulino wrote: > > > > > On Fri, 14 Jun 2013 15:45:52 +0800 > > > > > Amos Kong wrote: > > > > > > > > > > > Currently macvtap based macvlan device is working in promiscuous > > > > > > mode, we want to implement mac-programming over macvtap through > > > > > > Libvirt for better performance. > > > > > > > > > > > > Design: > > > > > > QEMU notifies Libvirt when rx-filter config is changed in guest, > > > > > > then Libvirt query the rx-filter information by a monitor command, > > > > > > and sync the change to macvtap device. Related rx-filter config > > > > > > of the nic contains main mac, rx-mode items and vlan table. > > > > > > > > > > > > This patch adds a QMP event to notify management of rx-filter change, > > > > > > and adds a monitor command for management to query rx-filter > > > > > > information. > > > > > > > > > > > > Test: > > > > > > If we repeatedly add/remove vlan, and change macaddr of vlan > > > > > > interfaces in guest by a loop script. > > > > > > > > > > > > Result: > > > > > > The events will flood the QMP client(management), management takes > > > > > > too much resource to process the events. > > > > > > > > > > > > Event_throttle API (set rate to 1 ms) can avoid the events to flood > > > > > > > > > > I doubt this is a valid value. Today, the three events that use the event > > > > > throttle API set the delay rate to 1000 ms. > > > > > > > > > > > QMP client, but it could cause an unexpected delay (~1ms), guests > > > > > > guests normally expect rx-filter updates immediately. > > > > > > > > > > What you mean by "immediately"? There's a round-trip to the host plus > > > > > all the stuff QEMU will execute to fulfil the request. And how did you > > > > > measure this, btw? > > > > > > > > > > What you have to do is is to measure your test-case in three different > > > > > scenarios: > > > > > > > > > > 1. Against upstream QEMU (ie. no patches) > > > > > 2. With the event throttle API > > > > > 3. With this patch > > > > > > > > > > Only then you'll be able which is better. > > > > > > > > > > > > I doubt there's a lot of value in splitting this patch in two and > > > > testing whether we can reproduce any problems with a partial patch. > > > > > > I didn't suggest that. Actually, I didn't even talk about code. > > > > > > > The > > > > problem of management not keeping up with event flood triggered by mac > > > > updates by guest might be purely theoretical, > > > > > > That's not the problem the flag thing is trying to solve. We have the > > > event throttle API for that. What the flag is trying to solve is _guest_ > > > latency due to event generation and it's not clear at all if this is a real > > > problem. > > > > > > > but a robust solution that > > > > does not have theoretical holes makes me sleep better at night, and it's a > > > > trivial amount of code. > > > > > > It has the drawback of coupling the query command with the event. > > > > What specifically is the problem? > > Events and commands are independent entities. Coupling them can have bad > side effects, like we may have the ability to disable commands in > the future. In this case this event would only be sent once! > We could, of course, disable the event along, but that's an unclear side > effect too. If query command is disabled, this event is useless. > > > I only see a fix for a problem, that might be theoretical, but I'm > > happier knowing I don't need to worry about it. > > > > > IMO, such > > > a coupling has to be justified with real data. > > > > It's a quality of implementation issue. > > > > No spec says that you should not delay RX filter updates for a very long > > time (yes 1000ms is very long), but if a NIC does it it's a low quality > > NIC, and it will cause trouble in the field. > > The 1000ms I talked about is *not* what the guest will see. If there are > events pending, the throttle API just queues the event and returns right > away. I'd even _guess_ that this is faster then emitting the event. If the filter is not updated for 1000ms then that is guest visible: it is not getting packets with the new MAC. > > The timeout you have to specify in the throttle API is what *mngt* will > see if events are flooded. That's the point. It's a good fit for events which are not guest visible. This event should lead to a guest visible effect so it should not be delayed. > > I don't think we need to spend time proving that with real data, it's just > > obvious. > > Why is it so difficult to show if it's that obvious? > > Let me clarify that I don't oppose to the implementation, as long as it's > shown that it's really needed.