[Qemu-devel] Re: [PATCH RFC] net: add a flag to disable mac/vlan filtering

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Mar 09, 2010 at 09:11:33AM -0700, Alex Williamson wrote:
> On Tue, 2010-03-09 at 17:30 +0200, Michael S. Tsirkin wrote:
> > On Tue, Mar 09, 2010 at 08:19:12AM -0700, Alex Williamson wrote:
> > > On Tue, 2010-03-09 at 15:15 +0200, Michael S. Tsirkin wrote:
> > > > diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> > > > index 5c0093e..01b45ed 100644
> > > > --- a/hw/virtio-net.c
> > > > +++ b/hw/virtio-net.c
> > > > @@ -47,6 +47,7 @@ typedef struct VirtIONet
> > > >      uint8_t nomulti;
> > > >      uint8_t nouni;
> > > >      uint8_t nobcast;
> > > > +    uint32_t filtering;
> > > >      struct {
> > > >          int in_use;
> > > >          int first_multi;
> > > > @@ -475,12 +476,17 @@ static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)
> > > >          ptr += sizeof(struct virtio_net_hdr);
> > > >      }
> > > >  
> > > > -    if (!memcmp(&ptr[12], vlan, sizeof(vlan))) {
> > > > +    if ((n->filtering & (0x1 << NICCONF_F_VLAN_FILTERING)) &&
> > > > +        !memcmp(&ptr[12], vlan, sizeof(vlan))) {
> > > >          int vid = be16_to_cpup((uint16_t *)(ptr + 14)) & 0xfff;
> > > >          if (!(n->vlans[vid >> 5] & (1U << (vid & 0x1f))))
> > > >              return 0;
> > > >      }
> > > >  
> > > > +    if (!(n->filtering & (0x1 << NICCONF_F_MAC_FILTERING))) {
> > > > +            return 1;
> > > > +    }
> > > > +
> > > 
> > > A filtering flags bitmap is a logical choice here, but I found the
> > > overhead to be non-trivial, which is why we have separate variables for
> > > the other filtering options.
> > 
> > You suggest more flags for multicast etc?
> 
> I'm suggesting we may get slightly better performance if we use separate
> filter_mac and filter_vlan variable flags instead of a single
> "filtering" flags bitmap.

Why? It's a single operation anyway, and we use less cache.

>  However, a couple other ideas... Should we
> call receive_filter() as a function pointer so we can make filter
> specific versions or remove it completely?  Some overhead to calling it
> as a pointer, but could still be a win.  Alternatively we could make
> "effective" filter flags which are used by receive_filter(), but
> maintained separately from the guest requested flags.  For instance:
> 
> virtio_net_handle_rx_mode(...)
> {
> ...
> n->alluni_effective = (n->filtering & (0x1 << NICCONF_F_MAC_FILTERING)) ? n->alluni : 1;
> n->allmulti_effective = (n->filtering & (0x1 << NICCONF_F_MAC_FILTERING)) ? n->allmulti : 1;
> 
> These could be recreated on loadvm, so we still wouldn't need to save
> them.  Question would be whether that creates a sufficiently fast path
> through receive_filter().  We'd still need a new flag for vlan filtering
> or maybe make the vlan header match bytes settable.
> 
> Alex

I agree, merging flags set by guest and by host makes sense, this way we
don't need to test both. E.g. all filtering off is equivalent to promisc
mode, we test that already. More complex ideas would I guess need to be
benchmarked to be shown being worth it.  So I'll do the simple thing and
we can tweak it further later on.

-- 
MST