diff mbox

[5/5] virtio_net: Add support for VLAN filtering in the hypervisor

Message ID 20090120163636.10034.5080.stgit@debian.lart
State Deferred, archived
Delegated to: David Miller
Headers show

Commit Message

Alex Williamson Jan. 20, 2009, 4:36 p.m. UTC 
VLAN filtering allows the hypervisor to drop packets from VLANs
that we're not a part of, further reducing the number of extraneous
packets recieved.  This makes use of the VLAN virtqueue command class.
The ENABLE command is used both to activate the filter and verify the
existence of the functionality on the backend.

Signed-off-by: Alex Williamson <alex.williamson@hp.com>
---

Updated with suggestions from Mark McLoughlin:
 - VLAN packet length fix split into separate patch
 - Indenting fix
 - Lower priority VLAN enabled printk
 - Renamed VLAN_KILL to VLAN_DEL

 drivers/net/virtio_net.c   |   49 +++++++++++++++++++++++++++++++++++++-------
 include/linux/virtio_net.h |   15 +++++++++++++
 2 files changed, 56 insertions(+), 8 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Mark McLoughlin Jan. 20, 2009, 4:44 p.m. UTC | #1 
On Tue, 2009-01-20 at 09:36 -0700, Alex Williamson wrote:
> VLAN filtering allows the hypervisor to drop packets from VLANs
> that we're not a part of, further reducing the number of extraneous
> packets recieved.  This makes use of the VLAN virtqueue command class.
> The ENABLE command is used both to activate the filter and verify the
> existence of the functionality on the backend.
> 
> Signed-off-by: Alex Williamson <alex.williamson@hp.com>

Cool, looks good.

Acked-by: Mark McLoughlin <markmc@redhat.com>

Cheers,
Mark.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Jan. 26, 2009, 2:08 a.m. UTC | #2 
From: Mark McLoughlin <markmc@redhat.com>
Date: Tue, 20 Jan 2009 16:44:00 +0000

> On Tue, 2009-01-20 at 09:36 -0700, Alex Williamson wrote:
> > VLAN filtering allows the hypervisor to drop packets from VLANs
> > that we're not a part of, further reducing the number of extraneous
> > packets recieved.  This makes use of the VLAN virtqueue command class.
> > The ENABLE command is used both to activate the filter and verify the
> > existence of the functionality on the backend.
> > 
> > Signed-off-by: Alex Williamson <alex.williamson@hp.com>
> 
> Cool, looks good.
> 
> Acked-by: Mark McLoughlin <markmc@redhat.com>

I'd like to make some forward progress on this patch set.
Either apply something, or defer to Rusty.

If you think I should apply it to net-next-2.6, please just
resubmit the serious with accumulated ACK'd etc.

Else I'll let these flow Rusty's way.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alex Williamson Jan. 26, 2009, 5:42 p.m. UTC | #3 
On Sun, 2009-01-25 at 18:08 -0800, David Miller wrote:
> From: Mark McLoughlin <markmc@redhat.com>
> Date: Tue, 20 Jan 2009 16:44:00 +0000
> 
> > On Tue, 2009-01-20 at 09:36 -0700, Alex Williamson wrote:
> > > VLAN filtering allows the hypervisor to drop packets from VLANs
> > > that we're not a part of, further reducing the number of extraneous
> > > packets recieved.  This makes use of the VLAN virtqueue command class.
> > > The ENABLE command is used both to activate the filter and verify the
> > > existence of the functionality on the backend.
> > > 
> > > Signed-off-by: Alex Williamson <alex.williamson@hp.com>
> > 
> > Cool, looks good.
> > 
> > Acked-by: Mark McLoughlin <markmc@redhat.com>
> 
> I'd like to make some forward progress on this patch set.
> Either apply something, or defer to Rusty.
> 
> If you think I should apply it to net-next-2.6, please just
> resubmit the serious with accumulated ACK'd etc.
> 
> Else I'll let these flow Rusty's way.

I'd still like to see if Rusty has any comments now that he's hopefully
catching up after LCA.  I'll resend them in a couple days if not.
Thanks,

Alex
Alex Williamson Jan. 27, 2009, 4:19 a.m. UTC | #4 
Hi Rusty,

On Tue, 2009-01-27 at 14:22 +1030, Rusty Russell wrote:
> > + struct virtnet_info *vi = netdev_priv(dev);
> > + u16 id = vid;
> > +
> > + if (virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
> > + VIRTIO_NET_CTRL_VLAN_ADD, &id, sizeof(id)))
> > + printk(KERN_WARNING "%s: Failed to add VLAN ID %d.\n",
> > + dev->name, id);
> > +}
> 
> The temporary var seems strange here, and in kill_vid.

It does stand out a bit.  I can't think of a valid reason for why I had
this, so I'll drop it.

> > - .ndo_poll_controller = virtnet_netpoll,
> > + .ndo_poll_controller = virtnet_netpoll,
> 
> Pretty lining up is a pet peeve of mine. Just indent them one more and
> be happy :)

Noted ;^)

> 
> > + /* Enable VLAN filtering if supported by the backend */
> > + if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
> > + VIRTIO_NET_CTRL_VLAN_ENABLE,
> > + &vlan_filter, sizeof(vlan_filter))) {
> > + printk(KERN_DEBUG "virtio_net: VLAN filter enabled\n");
> > + dev->features |= NETIF_F_HW_VLAN_FILTER;
> > + }
> 
> OK, several things here. Should probably be a feature bit. What is the
> vlan_filter arg for? Finally, using ! for the "success" case is YA API
> nastiness for me; if we were leaving virtnet_send_command as is I'd
> make it return bool, if it really returned a sensible -errno I'd
> compare against 0 here. 

The VLAN_ENABLE command can either enable or disable the VLAN filter, so
the vlan_filter arg is setting enable to "on".  A feature bit might be
more efficient, but since this happens at device init and the enable
will fail if it's not supported by the backend, it seemed redundant.

I agree the success/failure logic would be more clear w/o the !, I'll
switch this to a == 0, or something better depending on what we decide
to do with send_command.  Thanks for the comments,

Alex
diff mbox

Patch

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index f3849d1..e07019c 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -744,6 +744,28 @@  set_mode:
 		       dev->name, allmulti ? "en" : "dis");
 }
 
+static void virnet_vlan_rx_add_vid(struct net_device *dev, u16 vid)
+{
+	struct virtnet_info *vi = netdev_priv(dev);
+	u16 id = vid;
+
+	if (virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
+				 VIRTIO_NET_CTRL_VLAN_ADD, &id, sizeof(id)))
+		printk(KERN_WARNING "%s: Failed to add VLAN ID %d.\n",
+		       dev->name, id);
+}
+
+static void virnet_vlan_rx_kill_vid(struct net_device *dev, u16 vid)
+{
+	struct virtnet_info *vi = netdev_priv(dev);
+	u16 id = vid;
+
+	if (virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
+				 VIRTIO_NET_CTRL_VLAN_DEL, &id, sizeof(id)))
+		printk(KERN_WARNING "%s: Failed to kill VLAN ID %d.\n",
+		       dev->name, id);
+}
+
 static struct ethtool_ops virtnet_ethtool_ops = {
 	.set_tx_csum = virtnet_set_tx_csum,
 	.set_sg = ethtool_op_set_sg,
@@ -762,15 +784,17 @@  static int virtnet_change_mtu(struct net_device *dev, int new_mtu)
 }
 
 static const struct net_device_ops virtnet_netdev = {
-	.ndo_open            = virtnet_open,
-	.ndo_stop   	     = virtnet_close,
-	.ndo_start_xmit      = start_xmit,
-	.ndo_validate_addr   = eth_validate_addr,
-	.ndo_set_mac_address = virtnet_set_mac_address,
-	.ndo_set_rx_mode     = virtnet_set_rx_mode,
-	.ndo_change_mtu	     = virtnet_change_mtu,
+	.ndo_open             = virtnet_open,
+	.ndo_stop   	      = virtnet_close,
+	.ndo_start_xmit       = start_xmit,
+	.ndo_validate_addr    = eth_validate_addr,
+	.ndo_set_mac_address  = virtnet_set_mac_address,
+	.ndo_set_rx_mode      = virtnet_set_rx_mode,
+	.ndo_change_mtu	      = virtnet_change_mtu,
+	.ndo_vlan_rx_add_vid  = virnet_vlan_rx_add_vid,
+	.ndo_vlan_rx_kill_vid = virnet_vlan_rx_kill_vid,
 #ifdef CONFIG_NET_POLL_CONTROLLER
-	.ndo_poll_controller = virtnet_netpoll,
+	.ndo_poll_controller  = virtnet_netpoll,
 #endif
 };
 
@@ -864,6 +888,7 @@  static int virtnet_probe(struct virtio_device *vdev)
 		vi->cvq = NULL;
 	else {
 		unsigned int entries;
+		u8 vlan_filter = 1;
 
 		/*
 		 * We use a separate stack variable here because the
@@ -879,6 +904,14 @@  static int virtnet_probe(struct virtio_device *vdev)
 			       "MAC filter table allocation failed.\n");
 			mac_entries = 0;
 		}
+
+		/* Enable VLAN filtering if supported by the backend */
+		if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
+					  VIRTIO_NET_CTRL_VLAN_ENABLE,
+					  &vlan_filter, sizeof(vlan_filter))) {
+			printk(KERN_DEBUG "virtio_net: VLAN filter enabled\n");
+			dev->features |= NETIF_F_HW_VLAN_FILTER;
+		}
 	}
 
 	/* Initialize our empty receive and send queues. */
diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index 84086a6..1d7171c 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -99,4 +99,19 @@  typedef __u8 virtio_net_ctrl_ack;
  #define VIRTIO_NET_CTRL_MAC_TABLE_ALLOC      0
  #define VIRTIO_NET_CTRL_MAC_TABLE_SET        1
 
+/*
+ * Control VLAN filtering
+ *
+ * The VLAN filter table is controlled via a simple ADD/DEL interface.
+ * VLAN IDs not added will be dropped.  Del is the opposite of add.
+ * Both commands expect an out entry containing a 2 byte VLAN ID.
+ * The ENABLE command expects an out entry containing a single byte,
+ * zero to disable, non-zero to enable.  The default state is disabled
+ * for compatibility.
+ */
+#define VIRTIO_NET_CTRL_VLAN       2
+ #define VIRTIO_NET_CTRL_VLAN_ENABLE          0
+ #define VIRTIO_NET_CTRL_VLAN_ADD             1
+ #define VIRTIO_NET_CTRL_VLAN_DEL             2
+
 #endif /* _LINUX_VIRTIO_NET_H */