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

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

[Michael S. Tsirkin]

New bridge in linux 2.6.34 adds IGMP snooping support,
after which bridge should not normally flood any packets.
While we still need mac table to arm forwarding tables
after migration, we can thus ignore it for rx datapath.

For vlan, it's possible to do filtering down the
stack simply by using bridge per guest and binding said bridge
to vlan device, which some people do.

Since qemu has no easy way to check IGMP snooping
support in bridge or how it's connected, add options
to disable rx filtering, so that management can set it
as appropriate.
Use these options to optimise virtio-net rx path.
We still ask guest for the list of vlans/macs for
migration.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Cc: Alex Williamson <alex.williamson@hp.com>
Cc: Andreas Plesner Jacobsen <apj@mutt.dk>
---
 hw/virtio-net.c |   10 +++++++++-
 net.h           |   12 +++++++++++-
 2 files changed, 20 insertions(+), 2 deletions(-)

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;
+    }
+
     if (ptr[0] & 1) { // multicast
         if (!memcmp(ptr, bcast, sizeof(bcast))) {
             return !n->nobcast;
@@ -863,6 +869,8 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf)
 
     n->mac_table.macs = qemu_mallocz(MAC_TABLE_ENTRIES * ETH_ALEN);
 
+    n->filtering = conf->filtering;
+
     n->vlans = qemu_mallocz(MAX_VLAN >> 3);
 
     register_savevm("virtio-net", virtio_net_id++, VIRTIO_NET_VM_VERSION,
diff --git a/net.h b/net.h
index 33a1eaf..459ede5 100644
--- a/net.h
+++ b/net.h
@@ -18,12 +18,22 @@ typedef struct NICConf {
     MACAddr macaddr;
     VLANState *vlan;
     VLANClientState *peer;
+    uint32_t filtering;
 } NICConf;
 
+enum {
+    NICCONF_F_MAC_FILTERING = 0,
+    NICCONF_F_VLAN_FILTERING = 1
+};
+
 #define DEFINE_NIC_PROPERTIES(_state, _conf)                            \
     DEFINE_PROP_MACADDR("mac",   _state, _conf.macaddr),                \
     DEFINE_PROP_VLAN("vlan",     _state, _conf.vlan),                   \
-    DEFINE_PROP_NETDEV("netdev", _state, _conf.peer)
+    DEFINE_PROP_NETDEV("netdev", _state, _conf.peer),                    \
+    DEFINE_PROP_BIT("mac_filtering", _state, _conf.filtering,           \
+                     NICCONF_F_MAC_FILTERING, true)                     \
+    DEFINE_PROP_BIT("vlan_filtering", _state, _conf.filtering,          \
+                     NICCONF_F_VLAN_FILTERING, true)                    \
 
 /* VLANs support */
 
-- 
1.7.0.18.g0d53a5

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

[Anthony Liguori]

On 03/09/2010 07:15 AM, Michael S. Tsirkin wrote:
> New bridge in linux 2.6.34 adds IGMP snooping support,
> after which bridge should not normally flood any packets.
> While we still need mac table to arm forwarding tables
> after migration, we can thus ignore it for rx datapath.
>
> For vlan, it's possible to do filtering down the
> stack simply by using bridge per guest and binding said bridge
> to vlan device, which some people do.
>
> Since qemu has no easy way to check IGMP snooping
> support in bridge or how it's connected, add options
> to disable rx filtering, so that management can set it
> as appropriate.
> Use these options to optimise virtio-net rx path.
> We still ask guest for the list of vlans/macs for
> migration.
>
> Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
>    

Can't this be achieved by just disabling the feature bits?  IOW,

ctrl_vq=0,ctrl_vlan=0?

Regards,

Anthony Liguori

> Cc: Alex Williamson<alex.williamson@hp.com>
> Cc: Andreas Plesner Jacobsen<apj@mutt.dk>
> ---
>   hw/virtio-net.c |   10 +++++++++-
>   net.h           |   12 +++++++++++-
>   2 files changed, 20 insertions(+), 2 deletions(-)
>
> 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;
> +    }
> +
>       if (ptr[0]&  1) { // multicast
>           if (!memcmp(ptr, bcast, sizeof(bcast))) {
>               return !n->nobcast;
> @@ -863,6 +869,8 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf)
>
>       n->mac_table.macs = qemu_mallocz(MAC_TABLE_ENTRIES * ETH_ALEN);
>
> +    n->filtering = conf->filtering;
> +
>       n->vlans = qemu_mallocz(MAX_VLAN>>  3);
>
>       register_savevm("virtio-net", virtio_net_id++, VIRTIO_NET_VM_VERSION,
> diff --git a/net.h b/net.h
> index 33a1eaf..459ede5 100644
> --- a/net.h
> +++ b/net.h
> @@ -18,12 +18,22 @@ typedef struct NICConf {
>       MACAddr macaddr;
>       VLANState *vlan;
>       VLANClientState *peer;
> +    uint32_t filtering;
>   } NICConf;
>
> +enum {
> +    NICCONF_F_MAC_FILTERING = 0,
> +    NICCONF_F_VLAN_FILTERING = 1
> +};
> +
>   #define DEFINE_NIC_PROPERTIES(_state, _conf)                            \
>       DEFINE_PROP_MACADDR("mac",   _state, _conf.macaddr),                \
>       DEFINE_PROP_VLAN("vlan",     _state, _conf.vlan),                   \
> -    DEFINE_PROP_NETDEV("netdev", _state, _conf.peer)
> +    DEFINE_PROP_NETDEV("netdev", _state, _conf.peer),                    \
> +    DEFINE_PROP_BIT("mac_filtering", _state, _conf.filtering,           \
> +                     NICCONF_F_MAC_FILTERING, true)                     \
> +    DEFINE_PROP_BIT("vlan_filtering", _state, _conf.filtering,          \
> +                     NICCONF_F_VLAN_FILTERING, true)                    \
>
>   /* VLANs support */
>
>    

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

[Alex Williamson]

On Tue, 2010-03-09 at 08:43 -0600, Anthony Liguori wrote:
> On 03/09/2010 07:15 AM, Michael S. Tsirkin wrote:
> > New bridge in linux 2.6.34 adds IGMP snooping support,
> > after which bridge should not normally flood any packets.
> > While we still need mac table to arm forwarding tables
> > after migration, we can thus ignore it for rx datapath.
> >
> > For vlan, it's possible to do filtering down the
> > stack simply by using bridge per guest and binding said bridge
> > to vlan device, which some people do.
> >
> > Since qemu has no easy way to check IGMP snooping
> > support in bridge or how it's connected, add options
> > to disable rx filtering, so that management can set it
> > as appropriate.
> > Use these options to optimise virtio-net rx path.
> > We still ask guest for the list of vlans/macs for
> > migration.
> >
> > Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
> >    
> 
> Can't this be achieved by just disabling the feature bits?  IOW,
> 
> ctrl_vq=0,ctrl_vlan=0?

Michael still wants the guest to populate the MAC filter table so that
we can use it to announce MACs on the bridge, especially after a
migration.

Alex

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

[Michael S. Tsirkin]

On Tue, Mar 09, 2010 at 08:43:12AM -0600, Anthony Liguori wrote:
> On 03/09/2010 07:15 AM, Michael S. Tsirkin wrote:
>> New bridge in linux 2.6.34 adds IGMP snooping support,
>> after which bridge should not normally flood any packets.
>> While we still need mac table to arm forwarding tables
>> after migration, we can thus ignore it for rx datapath.
>>
>> For vlan, it's possible to do filtering down the
>> stack simply by using bridge per guest and binding said bridge
>> to vlan device, which some people do.
>>
>> Since qemu has no easy way to check IGMP snooping
>> support in bridge or how it's connected, add options
>> to disable rx filtering, so that management can set it
>> as appropriate.
>> Use these options to optimise virtio-net rx path.
>> We still ask guest for the list of vlans/macs for
>> migration.
>>
>> Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
>>    
>
> Can't this be achieved by just disabling the feature bits?  IOW,
>
> ctrl_vq=0,ctrl_vlan=0?
>
> Regards,
>
> Anthony Liguori

It can, but then we won't be able to migrate to a host
that does not do the filtering in host kernel.

>> Cc: Alex Williamson<alex.williamson@hp.com>
>> Cc: Andreas Plesner Jacobsen<apj@mutt.dk>
>> ---
>>   hw/virtio-net.c |   10 +++++++++-
>>   net.h           |   12 +++++++++++-
>>   2 files changed, 20 insertions(+), 2 deletions(-)
>>
>> 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;
>> +    }
>> +
>>       if (ptr[0]&  1) { // multicast
>>           if (!memcmp(ptr, bcast, sizeof(bcast))) {
>>               return !n->nobcast;
>> @@ -863,6 +869,8 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf)
>>
>>       n->mac_table.macs = qemu_mallocz(MAC_TABLE_ENTRIES * ETH_ALEN);
>>
>> +    n->filtering = conf->filtering;
>> +
>>       n->vlans = qemu_mallocz(MAX_VLAN>>  3);
>>
>>       register_savevm("virtio-net", virtio_net_id++, VIRTIO_NET_VM_VERSION,
>> diff --git a/net.h b/net.h
>> index 33a1eaf..459ede5 100644
>> --- a/net.h
>> +++ b/net.h
>> @@ -18,12 +18,22 @@ typedef struct NICConf {
>>       MACAddr macaddr;
>>       VLANState *vlan;
>>       VLANClientState *peer;
>> +    uint32_t filtering;
>>   } NICConf;
>>
>> +enum {
>> +    NICCONF_F_MAC_FILTERING = 0,
>> +    NICCONF_F_VLAN_FILTERING = 1
>> +};
>> +
>>   #define DEFINE_NIC_PROPERTIES(_state, _conf)                            \
>>       DEFINE_PROP_MACADDR("mac",   _state, _conf.macaddr),                \
>>       DEFINE_PROP_VLAN("vlan",     _state, _conf.vlan),                   \
>> -    DEFINE_PROP_NETDEV("netdev", _state, _conf.peer)
>> +    DEFINE_PROP_NETDEV("netdev", _state, _conf.peer),                    \
>> +    DEFINE_PROP_BIT("mac_filtering", _state, _conf.filtering,           \
>> +                     NICCONF_F_MAC_FILTERING, true)                     \
>> +    DEFINE_PROP_BIT("vlan_filtering", _state, _conf.filtering,          \
>> +                     NICCONF_F_VLAN_FILTERING, true)                    \
>>
>>   /* VLANs support */
>>
>>    

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

[Alex Williamson]

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.

>      if (ptr[0] & 1) { // multicast
>          if (!memcmp(ptr, bcast, sizeof(bcast))) {
>              return !n->nobcast;
> @@ -863,6 +869,8 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf)
>  
>      n->mac_table.macs = qemu_mallocz(MAC_TABLE_ENTRIES * ETH_ALEN);
>  
> +    n->filtering = conf->filtering;
> +

Since we're not touching this in the savevm code, I assume the intention
is that we can transparently migrate between filtered bridges and
non-filtered bridges, right?  Thanks,

Alex

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

[Michael S. Tsirkin]

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?

> >      if (ptr[0] & 1) { // multicast
> >          if (!memcmp(ptr, bcast, sizeof(bcast))) {
> >              return !n->nobcast;
> > @@ -863,6 +869,8 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf)
> >  
> >      n->mac_table.macs = qemu_mallocz(MAC_TABLE_ENTRIES * ETH_ALEN);
> >  
> > +    n->filtering = conf->filtering;
> > +
> 
> Since we're not touching this in the savevm code, I assume the intention
> is that we can transparently migrate between filtered bridges and
> non-filtered bridges, right?

Right.

>  Thanks,
> 
> Alex

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

[Michael S. Tsirkin]

On Tue, Mar 09, 2010 at 05:30:31PM +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?

Might not be a bad idea: e.g. tap filtering table in kernel
is limited in size, so we might win from only downloading
necessary mac addresses there.

> > >      if (ptr[0] & 1) { // multicast
> > >          if (!memcmp(ptr, bcast, sizeof(bcast))) {
> > >              return !n->nobcast;
> > > @@ -863,6 +869,8 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf)
> > >  
> > >      n->mac_table.macs = qemu_mallocz(MAC_TABLE_ENTRIES * ETH_ALEN);
> > >  
> > > +    n->filtering = conf->filtering;
> > > +
> > 
> > Since we're not touching this in the savevm code, I assume the intention
> > is that we can transparently migrate between filtered bridges and
> > non-filtered bridges, right?
> 
> Right.
> 
> >  Thanks,
> > 
> > Alex

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

[Paul Brook]

> > Can't this be achieved by just disabling the feature bits?  IOW,
> >
> > ctrl_vq=0,ctrl_vlan=0?
> 
> Michael still wants the guest to populate the MAC filter table so that
> we can use it to announce MACs on the bridge, especially after a
> migration.

We don't do that to start with.  We only set gratuitous ARPs for the MAC 
address of the emulated NIC.  I'm not convinced anything else is useful.

Paul

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

[Alex Williamson]

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.  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

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

[Michael S. Tsirkin]

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

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

[Alex Williamson]

On Tue, 2010-03-09 at 18:18 +0200, Michael S. Tsirkin wrote:
> 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:
> > > > 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.

Sorry, I must have been thinking of a runtime vs compile time shift.

Alex