Linux Virtualization - Jun 2016 - [PATCH -next 2/2] virtio_net: Read the advised MTU

The following series adds the ability for a hypervisor to set an MTU on the
guest during feature negotiation phase. This is useful for VM orchestration
when, for instance, tunneling is involved and the MTU of the various systems
should be homogenous.

The first patch adds the feature bit as described in the proposed VFIO spec
addition found at
https://lists.oasis-open.org/archives/virtio-dev/201603/msg00001.html

The second patch adds a user of the bit, and a warning when the guest changes
the MTU from the hypervisor advised MTU. Future patches may add more thorough
error handling.

Aaron Conole (2):
  virtio: Start feature MTU support
  virtio_net: Read the advised MTU

 drivers/net/virtio_net.c        | 7 +++++++
 include/uapi/linux/virtio_net.h | 2 ++
 2 files changed, 9 insertions(+)

-- 
2.5.5

This commit adds the feature bit and associated mtu device entry for the
virtio network device. Future commits will make use of these bits to
support negotiated MTU.

Signed-off-by: Aaron Conole <aconole at redhat.com>
---
 include/uapi/linux/virtio_net.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
index ec32293..751ff59 100644
--- a/include/uapi/linux/virtio_net.h
+++ b/include/uapi/linux/virtio_net.h
@@ -73,6 +73,8 @@ struct virtio_net_config {
 	 * Legal values are between 1 and 0x8000
 	 */
 	__u16 max_virtqueue_pairs;
+	/* Default maximum transmit unit advice */
+	__u16 mtu;
 } __attribute__((packed));
 
 /*
-- 
2.5.5

This patch checks the feature bit for the VIRTIO_NET_F_MTU feature. If it
exists, read the advised MTU and use it.

No proper error handling is provided for the case where a user changes the
negotiated MTU. A future commit will add proper error handling. Instead, a
warning is emitted if the guest changes the device MTU after previously
being given advice.

Signed-off-by: Aaron Conole <aconole at redhat.com>
---
 drivers/net/virtio_net.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index e0638e5..ef5ee01 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1896,6 +1896,12 @@ static int virtnet_probe(struct virtio_device *vdev)
 	if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
 		vi->has_cvq = true;
 
+	if (virtio_has_feature(vdev, VIRTIO_NET_F_MTU)) {
+		dev->mtu = virtio_cread16(vdev,
+					  offsetof(struct virtio_net_config,
+						   mtu));
+	}
+
 	if (vi->any_header_sg)
 		dev->needed_headroom = vi->hdr_len;
 
@@ -2067,6 +2073,7 @@ static unsigned int features[] = {
 	VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ,
 	VIRTIO_NET_F_CTRL_MAC_ADDR,
 	VIRTIO_F_ANY_LAYOUT,
+	VIRTIO_NET_F_MTU,
 };
 
 static struct virtio_driver virtio_net_driver = {
-- 
2.5.5

On Thu, Jun 02, 2016 at 11:43:31AM -0400, Aaron Conole
wrote:
> This patch checks the feature bit for the VIRTIO_NET_F_MTU feature. If it
> exists, read the advised MTU and use it.
> 
> No proper error handling is provided for the case where a user changes the
> negotiated MTU. A future commit will add proper error handling. Instead, a
> warning is emitted if the guest changes the device MTU after previously
> being given advice.
I don't see a warning and I don't think it's needed.
Patch is ok commit log isn't.
> Signed-off-by: Aaron Conole <aconole at redhat.com>
> ---
>  drivers/net/virtio_net.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index e0638e5..ef5ee01 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1896,6 +1896,12 @@ static int virtnet_probe(struct virtio_device *vdev)
>  	if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
>  		vi->has_cvq = true;
>  
> +	if (virtio_has_feature(vdev, VIRTIO_NET_F_MTU)) {
> +		dev->mtu = virtio_cread16(vdev,
> +					  offsetof(struct virtio_net_config,
> +						   mtu));
> +	}
> +
>  	if (vi->any_header_sg)
>  		dev->needed_headroom = vi->hdr_len;
>  
> @@ -2067,6 +2073,7 @@ static unsigned int features[] = {
>  	VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ,
>  	VIRTIO_NET_F_CTRL_MAC_ADDR,
>  	VIRTIO_F_ANY_LAYOUT,
> +	VIRTIO_NET_F_MTU,
>  };
>  
>  static struct virtio_driver virtio_net_driver = {
> -- 
> 2.5.5

On Thu, Jun 02, 2016 at 11:43:30AM -0400, Aaron Conole
wrote:
> This commit adds the feature bit and associated mtu device entry for the
> virtio network device. Future commits will make use of these bits to
> support negotiated MTU.
why split it out? Pls squash with the next patch.
> Signed-off-by: Aaron Conole <aconole at redhat.com>
> ---
>  include/uapi/linux/virtio_net.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/uapi/linux/virtio_net.h
b/include/uapi/linux/virtio_net.h
> index ec32293..751ff59 100644
> --- a/include/uapi/linux/virtio_net.h
> +++ b/include/uapi/linux/virtio_net.h
> @@ -73,6 +73,8 @@ struct virtio_net_config {
>  	 * Legal values are between 1 and 0x8000
>  	 */
>  	__u16 max_virtqueue_pairs;
> +	/* Default maximum transmit unit advice */
> +	__u16 mtu;
>  } __attribute__((packed));
>  
>  /*
> -- 
> 2.5.5

Hi Rick,

In the future, please don't cut the list.

Rick Jones <rick.jones2 at hpe.com> writes:
> On 06/02/2016 08:43 AM, Aaron Conole wrote:
>> This patch checks the feature bit for the VIRTIO_NET_F_MTU feature. If
it
>> exists, read the advised MTU and use it.
>>
>> No proper error handling is provided for the case where a user changes
the
>> negotiated MTU. A future commit will add proper error handling.
Instead, a
>> warning is emitted if the guest changes the device MTU after previously
>> being given advice.
>
> One of the things I've been doing has been setting-up a cluster
> (OpenStack) with JumboFrames, and then setting MTUs on instance vNICs
> by hand to measure different MTU sizes.  It would be a shame if such a
> thing were not possible in the future.  Keeping a warning if shrinking
> the MTU would be good, leave the error (perhaps) to if an attempt is
> made to go beyond the advised value.
This was cut because it didn't make sense for such a warning to
be issued, but it seems like perhaps you may want such a feature?  I
agree with Michael, after thinking about it, that I don't know what sort
of use the warning would serve.  After all, if you're changing the MTU,
you must have wanted such a change to occur?

-Aaron
> happy benchmarking,
>
> rick jones

Hi,

[auto build test ERROR on next-20160602]

url:   
https://github.com/0day-ci/linux/commits/Aaron-Conole/virtio-net-Advised-MTU-feature/20160603-000714
config: i386-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

Note: the
linux-review/Aaron-Conole/virtio-net-Advised-MTU-feature/20160603-000714 HEAD
d909da4df3c52f78b4f5fcccd89aea5e38722d10 builds fine.
      It only hurts bisectibility.

All errors (new ones prefixed by >>):

   drivers/net/virtio_net.c: In function
'virtnet_probe':
>> drivers/net/virtio_net.c:1899:31: error: 'VIRTIO_NET_F_MTU'
undeclared (first use in this function)
     if (virtio_has_feature(vdev, VIRTIO_NET_F_MTU)) {
                                  ^~~~~~~~~~~~~~~~
   drivers/net/virtio_net.c:1899:31: note: each undeclared identifier is
reported only once for each function it appears in
   drivers/net/virtio_net.c: At top level:
>> drivers/net/virtio_net.c:2076:2: error: 'VIRTIO_NET_F_MTU'
undeclared here (not in a function)
     VIRTIO_NET_F_MTU,
     ^~~~~~~~~~~~~~~~

vim +/VIRTIO_NET_F_MTU +1899 drivers/net/virtio_net.c

  1893		    virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
  1894			vi->any_header_sg = true;
  1895	
  1896		if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
  1897			vi->has_cvq = true;
  1898	
> 1899		if (virtio_has_feature(vdev, VIRTIO_NET_F_MTU)) {
  1900			dev->mtu = virtio_cread16(vdev,
  1901						  offsetof(struct virtio_net_config,
  1902							   mtu));
  1903		}
  1904	
  1905		if (vi->any_header_sg)
  1906			dev->needed_headroom = vi->hdr_len;
  1907	
  1908		/* Use single tx/rx queue pair as default */
  1909		vi->curr_queue_pairs = 1;
  1910		vi->max_queue_pairs = max_queue_pairs;
  1911	
  1912		/* Allocate/initialize the rx/tx queues, and invoke find_vqs */
  1913		err = init_vqs(vi);
  1914		if (err)
  1915			goto free_stats;
  1916	
  1917	#ifdef CONFIG_SYSFS
  1918		if (vi->mergeable_rx_bufs)
  1919			dev->sysfs_rx_queue_group = &virtio_net_mrg_rx_group;
  1920	#endif
  1921		netif_set_real_num_tx_queues(dev, vi->curr_queue_pairs);
  1922		netif_set_real_num_rx_queues(dev, vi->curr_queue_pairs);
  1923	
  1924		virtnet_init_settings(dev);
  1925	
  1926		err = register_netdev(dev);
  1927		if (err) {
  1928			pr_debug("virtio_net: registering device failed\n");
  1929			goto free_vqs;
  1930		}
  1931	
  1932		virtio_device_ready(vdev);
  1933	
  1934		vi->nb.notifier_call = &virtnet_cpu_callback;
  1935		err = register_hotcpu_notifier(&vi->nb);
  1936		if (err) {
  1937			pr_debug("virtio_net: registering cpu notifier failed\n");
  1938			goto free_unregister_netdev;
  1939		}
  1940	
  1941		/* Assume link up if device can't report link status,
  1942		   otherwise get link status from config. */
  1943		if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS)) {
  1944			netif_carrier_off(dev);
  1945			schedule_work(&vi->config_work);
  1946		} else {
  1947			vi->status = VIRTIO_NET_S_LINK_UP;
  1948			netif_carrier_on(dev);
  1949		}
  1950	
  1951		pr_debug("virtnet: registered device %s with %d RX and TX
vq's\n",
  1952			 dev->name, max_queue_pairs);
  1953	
  1954		return 0;
  1955	
  1956	free_unregister_netdev:
  1957		vi->vdev->config->reset(vdev);
  1958	
  1959		unregister_netdev(dev);
  1960	free_vqs:
  1961		cancel_delayed_work_sync(&vi->refill);
  1962		free_receive_page_frags(vi);
  1963		virtnet_del_vqs(vi);
  1964	free_stats:
  1965		free_percpu(vi->stats);
  1966	free:
  1967		free_netdev(dev);
  1968		return err;
  1969	}
  1970	
  1971	static void remove_vq_common(struct virtnet_info *vi)
  1972	{
  1973		vi->vdev->config->reset(vi->vdev);
  1974	
  1975		/* Free unused buffers in both send and recv, if any. */
  1976		free_unused_bufs(vi);
  1977	
  1978		free_receive_bufs(vi);
  1979	
  1980		free_receive_page_frags(vi);
  1981	
  1982		virtnet_del_vqs(vi);
  1983	}
  1984	
  1985	static void virtnet_remove(struct virtio_device *vdev)
  1986	{
  1987		struct virtnet_info *vi = vdev->priv;
  1988	
  1989		unregister_hotcpu_notifier(&vi->nb);
  1990	
  1991		/* Make sure no work handler is accessing the device. */
  1992		flush_work(&vi->config_work);
  1993	
  1994		unregister_netdev(vi->dev);
  1995	
  1996		remove_vq_common(vi);
  1997	
  1998		free_percpu(vi->stats);
  1999		free_netdev(vi->dev);
  2000	}
  2001	
  2002	#ifdef CONFIG_PM_SLEEP
  2003	static int virtnet_freeze(struct virtio_device *vdev)
  2004	{
  2005		struct virtnet_info *vi = vdev->priv;
  2006		int i;
  2007	
  2008		unregister_hotcpu_notifier(&vi->nb);
  2009	
  2010		/* Make sure no work handler is accessing the device */
  2011		flush_work(&vi->config_work);
  2012	
  2013		netif_device_detach(vi->dev);
  2014		cancel_delayed_work_sync(&vi->refill);
  2015	
  2016		if (netif_running(vi->dev)) {
  2017			for (i = 0; i < vi->max_queue_pairs; i++)
  2018				napi_disable(&vi->rq[i].napi);
  2019		}
  2020	
  2021		remove_vq_common(vi);
  2022	
  2023		return 0;
  2024	}
  2025	
  2026	static int virtnet_restore(struct virtio_device *vdev)
  2027	{
  2028		struct virtnet_info *vi = vdev->priv;
  2029		int err, i;
  2030	
  2031		err = init_vqs(vi);
  2032		if (err)
  2033			return err;
  2034	
  2035		virtio_device_ready(vdev);
  2036	
  2037		if (netif_running(vi->dev)) {
  2038			for (i = 0; i < vi->curr_queue_pairs; i++)
  2039				if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL))
  2040					schedule_delayed_work(&vi->refill, 0);
  2041	
  2042			for (i = 0; i < vi->max_queue_pairs; i++)
  2043				virtnet_napi_enable(&vi->rq[i]);
  2044		}
  2045	
  2046		netif_device_attach(vi->dev);
  2047	
  2048		rtnl_lock();
  2049		virtnet_set_queues(vi, vi->curr_queue_pairs);
  2050		rtnl_unlock();
  2051	
  2052		err = register_hotcpu_notifier(&vi->nb);
  2053		if (err)
  2054			return err;
  2055	
  2056		return 0;
  2057	}
  2058	#endif
  2059	
  2060	static struct virtio_device_id id_table[] = {
  2061		{ VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID },
  2062		{ 0 },
  2063	};
  2064	
  2065	static unsigned int features[] = {
  2066		VIRTIO_NET_F_CSUM, VIRTIO_NET_F_GUEST_CSUM,
  2067		VIRTIO_NET_F_GSO, VIRTIO_NET_F_MAC,
  2068		VIRTIO_NET_F_HOST_TSO4, VIRTIO_NET_F_HOST_UFO, VIRTIO_NET_F_HOST_TSO6,
  2069		VIRTIO_NET_F_HOST_ECN, VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6,
  2070		VIRTIO_NET_F_GUEST_ECN, VIRTIO_NET_F_GUEST_UFO,
  2071		VIRTIO_NET_F_MRG_RXBUF, VIRTIO_NET_F_STATUS, VIRTIO_NET_F_CTRL_VQ,
  2072		VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN,
  2073		VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ,
  2074		VIRTIO_NET_F_CTRL_MAC_ADDR,
  2075		VIRTIO_F_ANY_LAYOUT,
> 2076		VIRTIO_NET_F_MTU,
  2077	};
  2078	
  2079	static struct virtio_driver virtio_net_driver = {

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/octet-stream
Size: 55156 bytes
Desc: not available
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20160603/17a37f8c/attachment-0001.obj>

On 06/02/2016 10:06 AM, Aaron Conole wrote:
> Rick Jones <rick.jones2 at hpe.com> writes:
>> One of the things I've been doing has been setting-up a cluster
>> (OpenStack) with JumboFrames, and then setting MTUs on instance vNICs
>> by hand to measure different MTU sizes.  It would be a shame if such a
>> thing were not possible in the future.  Keeping a warning if shrinking
>> the MTU would be good, leave the error (perhaps) to if an attempt is
>> made to go beyond the advised value.
>
> This was cut because it didn't make sense for such a warning to
> be issued, but it seems like perhaps you may want such a feature?  I
> agree with Michael, after thinking about it, that I don't know what
sort
> of use the warning would serve.  After all, if you're changing the MTU,
> you must have wanted such a change to occur?
I don't need a warning, was simply willing to live with one when 
shrinking the MTU.  Didn't want an error.

happy benchmarking,

rick jones