Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event

[Michael S. Tsirkin]

On Tue, May 28, 2013 at 08:25:56AM -0400, Luiz Capitulino wrote:
> On Tue, 28 May 2013 06:43:04 +0800
> Amos Kong <address@hidden> wrote:
> 
> > On Mon, May 27, 2013 at 09:24:28AM -0400, Luiz Capitulino wrote:
> > > On Mon, 27 May 2013 09:10:11 -0400
> > > Luiz Capitulino <address@hidden> wrote:
> > > 
> > > > > We use the QMP event to notify management about the mac changing.
> > > > > 
> > > > > In this thread, we _wrongly_ considered to use qmp approach to delay
> > > > > the event for avoiding the flooding.
> > > > > 
> > > > >   eg: monitor_protocol_event_throttle(NIC_RX_FILTER_CHANGED, 1000);
> > > > > 
> > > > > Now we have a solution (using a flag to turn on/off the notify) to 
> > > > > avoid the
> > > > > flooding, only emit the event if we have no un-read event.
> > > > > 
> > > > > If we want to (flag is on) emit the event, we wish the event be sent 
> > > > > ASAP
> > > > > (so event_throttle isn't needed).
> > > > 
> > > > Unfortunately this doesn't answer my question. I did understand why 
> > > > you're
> > > > not using the event throttle API (which is because you don't want to 
> > > > slow down
> > > > the guest, not the QMP client).
> > > > 
> > > > My point is whether coupling the event with the query command is really
> > > > justified or even if it really fixes the problem. Two points:
> > > > 
> > > >  1. Coupling them is bad design, and will probably strike back, as we 
> > > > plan
> > > >     for a better API for events where events can be disabled
> > > 
> > > I meant we may in the future, for example, introduce the ability to 
> > > disable
> > > commands (and events). One could argue that the event w/o a query command
> > > is not that useful, as events can be lost. But loosing an event is one 
> > > thing,
> > > not having it because it got disabled by a side effect is another.
> > 
> > event_throttle() couples the event in QMP framework, but we use flags
> > to disabled the event from real source (emit points/senders). 
> > 
> > If we set the evstate->rate to -1, we can ignore the events in
> > monitor_protocol_event_queue(), but we could not control the event
> > emitting of each emit point (each nic).
> >  
> > > But anyway, my main point in this thread is to make sure we at least
> > > justify having this coupling. Aren't we optimizing prematurely? Aren't
> > > we optimizing for a corner case? That's what I want to see answered.
> > 
> > If it's a corner case, we don't need a general API to disable event.
> 
> If it's a corner case, it's really worth to fix it?
> 
> I think that what we need a real world test-case to show us we're
> doing the right thing.
> 
> > We can disable this event by a flag, and introduce a new API
> > if we have same request from other events.
> > 
> > > >  2. Can you actually show the problem does exist so that we ensure this 
> > > > is
> > > >     not premature optimization? Might be a good idea to have this in the
> > > >     commit log
> > > > 
> > > > > > (which is to couple the event with the query command) is
> > > > > > appropriate. We're in user-space already, many things could slow
> > > > > > the guest down apart from the event generation.
> > > > > > 
> > > > > > Two questions:
> > > > > > 
> > > > > >  1. Do we know how slow (or how many packets are actually dropped)
> > > > > >     if the mac is changed too often *and* the event is always sent?
> > > > > 
> > > > > We always disable interface first, then change the macaddr.
> > > > > But we just have patch to allow guest to change macaddr of
> > > > > virtio-net when the interface is running.
> > > > > 
> > > > > | commit 2dcd0cce551983afe2f900125457f10bb5d980ae
> > > > > | Author: Jiri Pirko <address@hidden>
> > > > > | Date:   Tue Dec 11 15:33:56 2012 -0500
> > > > > | 
> > > > > |     [virt] virtio_net: allow to change mac when iface is running
> > > > > 
> > > > > >  2. Does this solution consider what happens if the QMP client does
> > > > > >     respond timely to the event by issuing the query-rx-filter
> > > > > >     command?
> > > > > 
> > > > > We assume that the QMP client (management) cares about the mac 
> > > > > changing
> > > > > event, and will query the latest rx-filter state and sync to macvtap
> > > > > device.
> > > > > 
> > > > > 1) If QMP client respond timely to the event: that's what we expected 
> > > > > :)
> > > > 
> > > > Won't this slow down the guest? If not, why?
> > 
> > If guest changes fx-filter configs frequently & management always query the
> > event very timely, this will slow down the guest.
> > 
> > We should detect & process the abnormal behavior from management.
> 
> That's not abnormal. Management is doing what it should do.
> 
> Maybe using the event throttle API can solve the mngt side of the problem,
> but I still think we need a reproducible test-case to ensure we're doing
> the right thing.

I agree we should make sure this code is tested.
It's pretty easy: run ifconfig in a loop in guest.

Amos, did you try this? Probably should otherwise
we don't really know whether the logic works.


> > Management (qmp client) always respond timely to the event in the
> > begining. If guest changes rx-filter very frequently & continuous.
> > Then we increase the evstate->rate, even disable the event.
> > 
> > In the normal usecase, we should consider packet losing first (caused by
> > event delay + the delay is used by management to execute the change)
> > 
> > ---
> > btw, currently we could not test in real environment. If related
> > libvirt work finishes, we can evaluate with real delays, packet
> > losing, etc.
> > 
> > The worst condition is we could not accept the delay(packet losing),
> > we need to consider other solution for mac programming of macvtap.
> > 
> > > > > 2) If QMP client doesn't respond timely to the event: packets might 
> > > > > drop.
> > > > >    If we change mac when the interface is running, we can accept 
> > > > > trivial
> > > > >    packets dropping.
> > > > > 
> > > > > For second condition, we need to test in real environment when libvirt
> > > > > finishs the work of processing events.
> >