[RFC PATCH net-next 0/3] Extend macvtap with multiple qdiscs

[RFC PATCH net-next 0/3] Extend macvtap with multiple qdiscs

[Vladislav Yasevich]

This is an RFC series to extend macvtap with multiple qdisc.  Right now
multiqueue macvtap setups suffer from lock contention.  Macvtap sets
the queue index and thus gets a default qdisc allocated to it.  Since
it later users dev_queue_xmit() call to the macvlan type device (so that
we can packet captures and other filtering on macvtap itself) we end up
with qdisc lock contention since what we have is multiple file descriptors
writing to the same qdisc.

With this series, the macvtap device now becomes a true multi-queue device
that defaults to 1 queue.  Every time the user opens the device (this is
how multiqueue macvtap is used), we update the number of real queues for
the device.  When the user writes to the device, we record the queue index
associted with the file descriptor to the skb, and that ends up translating
to the device queue index.  This is one transmit only.  Receive side
is left alone and will prefer skb hash if available. 

Macvlan through this all is left with lockless transmit path.

Thanks
-vlad

Vladislav Yasevich (3):
  macvtap: mutiple qdiscs support
  macvlan: add queue selection functionality
  macvtap: Record the rx queue based on the user tap queue

 drivers/net/macvlan.c | 20 ++++++++++++++++++++
 drivers/net/macvtap.c | 19 +++++++++++++++++++
 2 files changed, 39 insertions(+)

-- 
2.1.0

[RFC PATCH net-next 1/3] macvtap: mutiple qdiscs support

[Vladislav Yasevich]

Since commit 6acf54f1cf0a6747bac9fea26f34cfc5a9029523
    macvtap: Add support of packet capture on macvtap device.

macvtap has been using dev_queue_xmit() call.  Since macvtap
set the queue length, it ended up using a qdisc and that caused
lock contention when multi-queue macvtap was used.

This patch extends the macvtap multi-queue support that adds
multiple qudiscs.   If macvtap is crated without a spcified
number of queues, we default to MAX_MACVTAP_QUEUES.  We always
start with one real queue.  After the that, every time the user
opens a macvtap device (for every open after the first one), we'll
update the number of real queues.  We also update the number of
queues upon queue disables and enables.  This is different then tap
bacuase, unlike tap device, macvtap will actually completely unlink
the tap queue  from the vlan and change the queue index.  As a result,
a disable/enable sequence will cause the queue index change and we
account for this by updating real queues.

Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
 drivers/net/macvtap.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index d636d05..810e7d3 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -161,6 +161,12 @@ static struct macvlan_dev *macvtap_get_vlan_rcu(const struct net_device *dev)
  * when both our references and any pending SKBs are gone.
  */
 
+static void macvtap_set_real_num_queues(struct net_device *dev, int q)
+{
+	netif_set_real_num_tx_queues(dev, q);
+	netif_set_real_num_rx_queues(dev, q);
+}
+
 static int macvtap_enable_queue(struct net_device *dev, struct file *file,
 				struct macvtap_queue *q)
 {
@@ -178,6 +184,7 @@ static int macvtap_enable_queue(struct net_device *dev, struct file *file,
 	q->enabled = true;
 
 	vlan->numvtaps++;
+	macvtap_set_real_num_queues(dev, vlan->numvtaps);
 out:
 	return err;
 }
@@ -204,6 +211,7 @@ static int macvtap_set_queue(struct net_device *dev, struct file *file,
 	vlan->numvtaps++;
 	vlan->numqueues++;
 
+	macvtap_set_real_num_queues(dev, vlan->numvtaps);
 	return 0;
 }
 
@@ -229,6 +237,7 @@ static int macvtap_disable_queue(struct macvtap_queue *q)
 		q->enabled = false;
 
 		vlan->numvtaps--;
+		macvtap_set_real_num_queues(vlan->dev, vlan->numvtaps);
 	}
 
 	return 0;
@@ -481,15 +490,24 @@ static void macvtap_dellink(struct net_device *dev,
 
 static void macvtap_setup(struct net_device *dev)
 {
+	/* Start with 1 queue and will update as user adds them */
+	macvtap_set_real_num_queues(dev, 1);
 	macvlan_common_setup(dev);
 	dev->tx_queue_len = TUN_READQ_SIZE;
 }
 
+static unsigned int macvtap_get_num_queues(void)
+{
+	return MAX_MACVTAP_QUEUES;
+}
+
 static struct rtnl_link_ops macvtap_link_ops __read_mostly = {
 	.kind		= "macvtap",
 	.setup		= macvtap_setup,
 	.newlink	= macvtap_newlink,
 	.dellink	= macvtap_dellink,
+	.get_num_tx_queues = macvtap_get_num_queues,
+	.get_num_rx_queues = macvtap_get_num_queues,
 };
 
 
-- 
2.1.0

[RFC PATCH net-next 2/3] macvlan: add queue selection functionality

[Vladislav Yasevich]

This patch adds a simple queue selection function to macvlan
layer.  In most cases, this will just use the standard fallback
fuction, but when rx-queue has been recoreded we'll try to use
that value.

Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
 drivers/net/macvlan.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 94e6888..213d587 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -1018,6 +1018,25 @@ static int macvlan_dev_get_iflink(const struct net_device *dev)
 	return vlan->lowerdev->ifindex;
 }
 
+static u16 macvlan_select_queue(struct net_device *dev,
+				struct sk_buff *skb,
+				void *accel_priv,
+				select_queue_fallback_t fallback)
+{
+	u16 txq = skb_rx_queue_recorded(skb) ? skb_get_rx_queue(skb) : 0;
+
+	if (!txq)
+		return fallback(dev, skb);
+
+	if (unlikely(txq >= dev->real_num_tx_queues)) {
+		do {
+			txq -= dev->real_num_tx_queues;
+		} while (txq >= dev->real_num_tx_queues);
+	}
+	return txq;
+}
+
+
 static const struct ethtool_ops macvlan_ethtool_ops = {
 	.get_link		= ethtool_op_get_link,
 	.get_settings		= macvlan_ethtool_get_settings,
@@ -1048,6 +1067,7 @@ static const struct net_device_ops macvlan_netdev_ops = {
 	.ndo_netpoll_setup	= macvlan_dev_netpoll_setup,
 	.ndo_netpoll_cleanup	= macvlan_dev_netpoll_cleanup,
 #endif
+	.ndo_select_queue	= macvlan_select_queue,
 	.ndo_get_iflink		= macvlan_dev_get_iflink,
 	.ndo_features_check	= passthru_features_check,
 };
-- 
2.1.0

Re: [RFC PATCH net-next 2/3] macvlan: add queue selection functionality

[Eric Dumazet]

On Sun, 2016-02-07 at 22:14 -0500, Vladislav Yasevich wrote:
> This patch adds a simple queue selection function to macvlan
> layer.  In most cases, this will just use the standard fallback
> fuction, but when rx-queue has been recoreded we'll try to use
> that value.
> 
> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> ---
>  drivers/net/macvlan.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index 94e6888..213d587 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -1018,6 +1018,25 @@ static int macvlan_dev_get_iflink(const struct net_device *dev)
>  	return vlan->lowerdev->ifindex;
>  }
>  
> +static u16 macvlan_select_queue(struct net_device *dev,
> +				struct sk_buff *skb,
> +				void *accel_priv,
> +				select_queue_fallback_t fallback)
> +{
> +	u16 txq = skb_rx_queue_recorded(skb) ? skb_get_rx_queue(skb) : 0;
> +
> +	if (!txq)
> +		return fallback(dev, skb);
> +
> +	if (unlikely(txq >= dev->real_num_tx_queues)) {
> +		do {
> +			txq -= dev->real_num_tx_queues;
> +		} while (txq >= dev->real_num_tx_queues);
> +	}
> +	return txq;
> +}
> +
> +
>  static const struct ethtool_ops macvlan_ethtool_ops = {
>  	.get_link		= ethtool_op_get_link,
>  	.get_settings		= macvlan_ethtool_get_settings,
> @@ -1048,6 +1067,7 @@ static const struct net_device_ops macvlan_netdev_ops = {
>  	.ndo_netpoll_setup	= macvlan_dev_netpoll_setup,
>  	.ndo_netpoll_cleanup	= macvlan_dev_netpoll_cleanup,
>  #endif
> +	.ndo_select_queue	= macvlan_select_queue,
>  	.ndo_get_iflink		= macvlan_dev_get_iflink,
>  	.ndo_features_check	= passthru_features_check,
>  };

Should not it be the default behavior ?

This patch looks not needed to me.

Re: [RFC PATCH net-next 2/3] macvlan: add queue selection functionality

[Pankaj Gupta]

> 
> This patch adds a simple queue selection function to macvlan
> layer.  In most cases, this will just use the standard fallback
> fuction, but when rx-queue has been recoreded we'll try to use
> that value.
> 
> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> ---
>  drivers/net/macvlan.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index 94e6888..213d587 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -1018,6 +1018,25 @@ static int macvlan_dev_get_iflink(const struct
> net_device *dev)
>  	return vlan->lowerdev->ifindex;
>  }
>  
> +static u16 macvlan_select_queue(struct net_device *dev,
> +				struct sk_buff *skb,
> +				void *accel_priv,
> +				select_queue_fallback_t fallback)
> +{
> +	u16 txq = skb_rx_queue_recorded(skb) ? skb_get_rx_queue(skb) : 0;
> +
> +	if (!txq)
> +		return fallback(dev, skb);
> +
> +	if (unlikely(txq >= dev->real_num_tx_queues)) {
> +		do {
> +			txq -= dev->real_num_tx_queues;
> +		} while (txq >= dev->real_num_tx_queues);
                  if this is just to bring number down, %(mod) would be better
> +	}
> +	return txq;
> +}
> +
> +
>  static const struct ethtool_ops macvlan_ethtool_ops = {
>  	.get_link		= ethtool_op_get_link,
>  	.get_settings		= macvlan_ethtool_get_settings,
> @@ -1048,6 +1067,7 @@ static const struct net_device_ops macvlan_netdev_ops =
> {
>  	.ndo_netpoll_setup	= macvlan_dev_netpoll_setup,
>  	.ndo_netpoll_cleanup	= macvlan_dev_netpoll_cleanup,
>  #endif
> +	.ndo_select_queue	= macvlan_select_queue,
>  	.ndo_get_iflink		= macvlan_dev_get_iflink,
>  	.ndo_features_check	= passthru_features_check,
>  };
> --
> 2.1.0
> 
> 

[RFC PATCH net-next 3/3] macvtap: Record the rx queue based on the user tap queue

[Vladislav Yasevich]

Now that macvlan has a basic queue selection and macvtap has
multiple qdiscs, we can record the the queue index into the skb
based on the index of the macvtap user queue.  This index will
then be used to select the right qdisc.

Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
 drivers/net/macvtap.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index 810e7d3..650a374 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -821,6 +821,7 @@ static ssize_t macvtap_get_user(struct macvtap_queue *q, struct msghdr *m,
 			goto err_kfree;
 	}
 
+	skb_record_rx_queue(skb, q->queue_index);
 	skb_probe_transport_header(skb, ETH_HLEN);
 
 	/* Move network header to the right position for VLAN tagged packets */
-- 
2.1.0

Re: [RFC PATCH net-next 0/3] Extend macvtap with multiple qdiscs

[Jason Wang]

On 02/08/2016 11:14 AM, Vladislav Yasevich wrote:
> This is an RFC series to extend macvtap with multiple qdisc.  Right now
> multiqueue macvtap setups suffer from lock contention.  Macvtap sets
> the queue index and thus gets a default qdisc allocated to it.  Since
> it later users dev_queue_xmit() call to the macvlan type device (so that
> we can packet captures and other filtering on macvtap itself) we end up
> with qdisc lock contention since what we have is multiple file descriptors
> writing to the same qdisc.
>
> With this series, the macvtap device now becomes a true multi-queue device
> that defaults to 1 queue.  Every time the user opens the device (this is
> how multiqueue macvtap is used), we update the number of real queues for
> the device.  When the user writes to the device, we record the queue index
> associted with the file descriptor to the skb, and that ends up translating
> to the device queue index.  This is one transmit only.  Receive side
> is left alone and will prefer skb hash if available. 
>
> Macvlan through this all is left with lockless transmit path.
>
> Thanks
> -vlad

A question is do we really want a qdisc for macvtap (and other pseudo
device)? If yes, the "problem" may not be specific to macvtap. Consider
you may get contention after place a qdisc on vlan device. If not,
probably a one line patch with IFF_NO_QUEUE is sufficient?

Thanks

>
> Vladislav Yasevich (3):
>   macvtap: mutiple qdiscs support
>   macvlan: add queue selection functionality
>   macvtap: Record the rx queue based on the user tap queue
>
>  drivers/net/macvlan.c | 20 ++++++++++++++++++++
>  drivers/net/macvtap.c | 19 +++++++++++++++++++
>  2 files changed, 39 insertions(+)
>