Re: [Qemu-devel] [PATCH 3/4] qemu:virtio-net: Add support for qemu_vlan_rxfilter

[Alex Williamson]

On Thu, 2009-02-12 at 17:05 +0000, Paul Brook wrote:
> On Thursday 12 February 2009, Alex Williamson wrote:
> > On Thu, 2009-02-12 at 16:26 +0000, Paul Brook wrote:
> > > > +static void virtio_net_vlan_client_added(void *opaque)
> > > >...
> > > > +static void virtio_net_vlan_client_removed(void *opaque)
> > >
> > > Why are these two different?
> > >
> > > It looks like what you really want is a callback for "Something changed,
> > > and you need to reset your MAC filter."
> >
> > I think we'd end up with a race if we only had one callback.  For
> > instance if "change" was the result of a vlan client being removed, the
> > tap would clear the filter and the nic would try to install a filter.
> > The results would be different based on the calling order.
> 
> In that case either your implementation or your abstraction is wrong. 
> Devices shouldn't need to know or care why a filter failed to apply.

They don't, but they do need to know whether a filter they previously
applied successfully is no longer in place.  If they don't know this,
they have to assume the filter on the other side of the vlan is
transient and always double check it with their own filtering, which
seems like a waste of cache and cycles.

Is your objection that the NIC needs to associate the change in filter
state with a vlan event?  If so, maybe we can re-architect the callbacks
to something more appropriate.  We would need both a filter_cleared()
and a filter_retry() to achieve the same mechanics.  filter_cleared()
feels like something the tap device (the "filterer") would call on the
NIC (the "filteree").  But then the "filterer" needs some callback to
know when to clear and notify, which gets us back to somebody needs to
associate a vlan event with a change in the filter.  Suggestions?

> I'm not sure what you mean by "the tap would clear the filter".

By clear, I mean disable the filter, bringing the vlan back to an
unfiltered state.

> You have three options:
> 
> - Devices request a filter, and that request may fail. qemu notifies the 
> device when it needs to retry the filter. It doesn't make any difference 
> whether we think we just broke the old filter, or we think a previously 
> failed filter may succeed now, the device response is the same: Try to set a 
> filter and see if it works. This is what I assume you're trying to implement.
> - Implement reliable filters. The device requests a filter once[1]. qemu 
> makes 
> sure that filter is always honored.
> - Devices request a filter once. qemu remembers what that filter is, and 
> notifies the device if/when that filter becomes active/inactive.

None of these are complete solutions.  My code behaves like this:

- A device requests a filter and is told if the request is successful
  - On success the device may skip it's own filtering
- If another vlan client is added, the following _must_ occur:
  - The "filterer" must clear (remove) the filter
  - The "filteree" must revert to using their own filtering
- If a vlan client is removed, the following _may_ occur:
  - vlan clients are notified that they may retry their filter

The added()/removed() interface feels like the right solution to this.
We could use a changed() function, but it would need to know the
direction of the change, which leads back to the same mechanics.  If
there's a better way, please suggest it.  Thanks,

Alex