[dpdk-stable] [PATCH 19.11 1/2] net/netvsc: allow setting Rx and Tx copy break

[dpdk-stable] [PATCH 19.11 1/2] net/netvsc: allow setting Rx and Tx copy break

[Long Li]

From: Stephen Hemminger <stephen@networkplumber.org>

[ upstream commit 74a5a6663b5820211b8f4c0a9610ae1f0f8bd497 ]

The values for Rx and Tx copy break should be tunable rather
than hard coded constants.

The rx_copybreak sets the threshold where the driver uses an
external mbuf to avoid having to copy data. Setting 0 for copybreak
will cause driver to always create an external mbuf. Setting
a value greater than the MTU would prevent it from ever making
an external mbuf and always copy. The default value is 256 (bytes).

Likewise the tx_copybreak sets the threshold where the driver
aggregates multiple small packets into one request. If tx_copybreak
is 0 then each packet goes as a VMBus request (no copying).
If tx_copybreak is set larger than the MTU, then all packets smaller
than the chunk size of the VMBus send buffer will be copied; larger
packets always have to go as a single direct request. The default
value is 512 (bytes).

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Long Li <longli@microsoft.com>
---
 doc/guides/nics/netvsc.rst     | 17 +++++++++++++
 drivers/net/netvsc/hn_ethdev.c | 45 +++++++++++++++++++++++-----------
 drivers/net/netvsc/hn_rxtx.c   |  8 +++---
 drivers/net/netvsc/hn_var.h    |  5 ++++
 4 files changed, 56 insertions(+), 19 deletions(-)

diff --git a/doc/guides/nics/netvsc.rst b/doc/guides/nics/netvsc.rst
index 6dbb9a5513..5a68ffa8a3 100644
--- a/doc/guides/nics/netvsc.rst
+++ b/doc/guides/nics/netvsc.rst
@@ -116,3 +116,20 @@ The user can specify below argument in devargs.
     values save CPU cycles. This parameter is in microseconds.
     If the value is too large or too small it will be
     ignored by the host. (Default: 50)
+
+#.  ``rx_copybreak``:
+
+    The rx_copybreak sets the threshold where the driver uses an external
+    mbuf to avoid having to copy data. Setting 0 for copybreak will cause
+    driver to always create an external mbuf. Setting a value greater than
+    the MTU would prevent it from ever making an external mbuf and always
+    copy. The default value is 256 (bytes).
+
+#.  ``tx_copybreak``:
+
+    The tx_copybreak sets the threshold where the driver aggregates
+    multiple small packets into one request. If tx_copybreak is 0 then
+    each packet goes as a VMBus request (no copying). If tx_copybreak is
+    set larger than the MTU, then all packets smaller than the chunk size
+    of the VMBus send buffer will be copied; larger packets always have to
+    go as a single direct request. The default value is 512 (bytes).
diff --git a/drivers/net/netvsc/hn_ethdev.c b/drivers/net/netvsc/hn_ethdev.c
index 20cf1cc902..e2f5adc498 100644
--- a/drivers/net/netvsc/hn_ethdev.c
+++ b/drivers/net/netvsc/hn_ethdev.c
@@ -47,6 +47,9 @@
 
 int hn_logtype_init;
 int hn_logtype_driver;
+#define NETVSC_ARG_LATENCY "latency"
+#define NETVSC_ARG_RXBREAK "rx_copybreak"
+#define NETVSC_ARG_TXBREAK "tx_copybreak"
 
 struct hn_xstats_name_off {
 	char name[RTE_ETH_XSTATS_NAME_SIZE];
@@ -141,24 +144,32 @@ eth_dev_vmbus_release(struct rte_eth_dev *eth_dev)
 	eth_dev->intr_handle = NULL;
 }
 
-/* handle "latency=X" from devargs */
-static int hn_set_latency(const char *key, const char *value, void *opaque)
+static int hn_set_parameter(const char *key, const char *value, void *opaque)
 {
 	struct hn_data *hv = opaque;
 	char *endp = NULL;
-	unsigned long lat;
-
-	errno = 0;
-	lat = strtoul(value, &endp, 0);
+	unsigned long v;
 
+	v = strtoul(value, &endp, 0);
 	if (*value == '\0' || *endp != '\0') {
 		PMD_DRV_LOG(ERR, "invalid parameter %s=%s", key, value);
 		return -EINVAL;
 	}
 
-	PMD_DRV_LOG(DEBUG, "set latency %lu usec", lat);
+	if (!strcmp(key, NETVSC_ARG_LATENCY)) {
+		/* usec to nsec */
+		hv->latency = v * 1000;
+		PMD_DRV_LOG(DEBUG, "set latency %u usec", hv->latency);
+	} else if (!strcmp(key, NETVSC_ARG_RXBREAK)) {
+		hv->rx_copybreak = v;
+		PMD_DRV_LOG(DEBUG, "rx copy break set to %u",
+			    hv->rx_copybreak);
+	} else if (!strcmp(key, NETVSC_ARG_TXBREAK)) {
+		hv->tx_copybreak = v;
+		PMD_DRV_LOG(DEBUG, "tx copy break set to %u",
+			    hv->tx_copybreak);
+	}
 
-	hv->latency = lat * 1000;	/* usec to nsec */
 	return 0;
 }
 
@@ -168,7 +179,9 @@ static int hn_parse_args(const struct rte_eth_dev *dev)
 	struct hn_data *hv = dev->data->dev_private;
 	struct rte_devargs *devargs = dev->device->devargs;
 	static const char * const valid_keys[] = {
-		"latency",
+		NETVSC_ARG_LATENCY,
+		NETVSC_ARG_RXBREAK,
+		NETVSC_ARG_TXBREAK,
 		NULL
 	};
 	struct rte_kvargs *kvlist;
@@ -182,15 +195,13 @@ static int hn_parse_args(const struct rte_eth_dev *dev)
 
 	kvlist = rte_kvargs_parse(devargs->args, valid_keys);
 	if (!kvlist) {
-		PMD_DRV_LOG(NOTICE, "invalid parameters");
+		PMD_DRV_LOG(ERR, "invalid parameters");
 		return -EINVAL;
 	}
 
-	ret = rte_kvargs_process(kvlist, "latency", hn_set_latency, hv);
-	if (ret)
-		PMD_DRV_LOG(ERR, "Unable to process latency arg\n");
-
+	ret = rte_kvargs_process(kvlist, NULL, hn_set_parameter, hv);
 	rte_kvargs_free(kvlist);
+
 	return ret;
 }
 
@@ -957,6 +968,8 @@ eth_hn_dev_init(struct rte_eth_dev *eth_dev)
 	hv->chim_res  = &vmbus->resource[HV_SEND_BUF_MAP];
 	hv->port_id = eth_dev->data->port_id;
 	hv->latency = HN_CHAN_LATENCY_NS;
+	hv->rx_copybreak = HN_RXCOPY_THRESHOLD;
+	hv->tx_copybreak = HN_TXCOPY_THRESHOLD;
 	hv->max_queues = 1;
 	rte_spinlock_init(&hv->vf_lock);
 	hv->vf_port = HN_INVALID_PORT;
@@ -1125,3 +1138,7 @@ RTE_INIT(hn_init_log)
 	if (hn_logtype_driver >= 0)
 		rte_log_set_level(hn_logtype_driver, RTE_LOG_NOTICE);
 }
+RTE_PMD_REGISTER_PARAM_STRING(net_netvsc,
+			      NETVSC_ARG_LATENCY "=<uint32> "
+			      NETVSC_ARG_RXBREAK "=<uint32> "
+			      NETVSC_ARG_TXBREAK "=<uint32>");
diff --git a/drivers/net/netvsc/hn_rxtx.c b/drivers/net/netvsc/hn_rxtx.c
index c0965ab420..1cbb2ccee5 100644
--- a/drivers/net/netvsc/hn_rxtx.c
+++ b/drivers/net/netvsc/hn_rxtx.c
@@ -40,9 +40,6 @@
 	(sizeof(struct vmbus_chanpkt_hdr) + sizeof(struct hn_nvs_rndis))
 
 #define HN_TXD_CACHE_SIZE	32 /* per cpu tx_descriptor pool cache */
-#define HN_TXCOPY_THRESHOLD	512
-
-#define HN_RXCOPY_THRESHOLD	256
 #define HN_RXQ_EVENT_DEFAULT	2048
 
 struct hn_rxinfo {
@@ -541,7 +538,7 @@ static void hn_rxpkt(struct hn_rx_queue *rxq, struct hn_rx_bufinfo *rxb,
 	 * For large packets, avoid copy if possible but need to keep
 	 * some space available in receive area for later packets.
 	 */
-	if (dlen >= HN_RXCOPY_THRESHOLD &&
+	if (dlen > hv->rx_copybreak &&
 	    (uint32_t)rte_atomic32_read(&rxq->rxbuf_outstanding) <
 			hv->rxbuf_section_cnt / 2) {
 		struct rte_mbuf_ext_shared_info *shinfo;
@@ -1420,7 +1417,8 @@ hn_xmit_pkts(void *ptxq, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
 			break;
 
 		/* For small packets aggregate them in chimney buffer */
-		if (m->pkt_len < HN_TXCOPY_THRESHOLD && pkt_size <= txq->agg_szmax) {
+		if (m->pkt_len <= hv->tx_copybreak &&
+		    pkt_size <= txq->agg_szmax) {
 			/* If this packet will not fit, then flush  */
 			if (txq->agg_pktleft == 0 ||
 			    RTE_ALIGN(pkt_size, txq->agg_align) > txq->agg_szleft) {
diff --git a/drivers/net/netvsc/hn_var.h b/drivers/net/netvsc/hn_var.h
index 44c9db95a6..50ca1e56ca 100644
--- a/drivers/net/netvsc/hn_var.h
+++ b/drivers/net/netvsc/hn_var.h
@@ -23,6 +23,9 @@
 /* Host monitor interval */
 #define HN_CHAN_LATENCY_NS	50000
 
+#define HN_TXCOPY_THRESHOLD	512
+#define HN_RXCOPY_THRESHOLD	256
+
 /* Buffers need to be aligned */
 #ifndef PAGE_SIZE
 #define PAGE_SIZE 4096
@@ -115,6 +118,7 @@ struct hn_data {
 
 	struct rte_mem_resource *rxbuf_res;	/* UIO resource for Rx */
 	uint32_t	rxbuf_section_cnt;	/* # of Rx sections */
+	uint32_t	rx_copybreak;
 	uint16_t	max_queues;		/* Max available queues */
 	uint16_t	num_queues;
 	uint64_t	rss_offloads;
@@ -123,6 +127,7 @@ struct hn_data {
 	struct rte_mem_resource *chim_res;	/* UIO resource for Tx */
 	struct rte_bitmap *chim_bmap;		/* Send buffer map */
 	void		*chim_bmem;
+	uint32_t	tx_copybreak;
 	uint32_t	chim_szmax;		/* Max size per buffer */
 	uint32_t	chim_cnt;		/* Max packets per buffer */
 
-- 
2.27.0

[dpdk-stable] [PATCH 19.11 2/2] net/netvsc: control use of external mbuf on Rx

[Long Li]

From: Long Li <longli@microsoft.com>

[ upstream commit 096b31fc0d8c989cc455c35f4d1def24a4ed6dee ]

When receiving packets, netvsp puts data in a buffer mapped through UIO.
Depending on packet size, netvsc may attach the buffer as an external
mbuf. This is not a problem if this mbuf is consumed in the application,
and the application can correctly read data out of an external mbuf.

However, there are two problems with data in an external mbuf.
1. Due to the limitation of the kernel UIO implementation, physical
   address of this external buffer is not exposed to the user-mode. If
   this mbuf is passed to another driver, the other driver is unable to
   map this buffer to iova.
2. Some DPDK applications are not aware of external mbuf, and may bug
   when they receive an mbuf with external buffer attached.

Introduce a driver parameter "rx_extmbuf_enable" to control if netvsc
should use external mbuf for receiving packets. The default value is 0.
(netvsc doesn't use external mbuf, it always allocates mbuf and copy
data to mbuf) A non-zero value tells netvsc to attach external buffers
to mbuf on receiving packets, thus avoid copying memory.

Signed-off-by: Long Li <longli@microsoft.com>
---
 doc/guides/nics/netvsc.rst     |  8 ++++++++
 drivers/net/netvsc/hn_ethdev.c | 10 +++++++++-
 drivers/net/netvsc/hn_rxtx.c   |  2 +-
 drivers/net/netvsc/hn_var.h    |  3 +++
 4 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/doc/guides/nics/netvsc.rst b/doc/guides/nics/netvsc.rst
index 5a68ffa8a3..19f9940fe6 100644
--- a/doc/guides/nics/netvsc.rst
+++ b/doc/guides/nics/netvsc.rst
@@ -133,3 +133,11 @@ The user can specify below argument in devargs.
     set larger than the MTU, then all packets smaller than the chunk size
     of the VMBus send buffer will be copied; larger packets always have to
     go as a single direct request. The default value is 512 (bytes).
+
+#.  ``rx_extmbuf_enable``:
+    The rx_extmbuf_enable is used to control if netvsc should use external
+    mbuf for receiving packets. The default value is 0. (netvsc doesn't use
+    external mbuf, it always allocates mbuf and copy received data to mbuf)
+    A non-zero value tells netvsc to attach external buffers to mbuf on
+    receiving packets, thus avoid copying memory. Use of external buffers
+    requires the application is able to read data from external mbuf.
diff --git a/drivers/net/netvsc/hn_ethdev.c b/drivers/net/netvsc/hn_ethdev.c
index e2f5adc498..596bec4860 100644
--- a/drivers/net/netvsc/hn_ethdev.c
+++ b/drivers/net/netvsc/hn_ethdev.c
@@ -50,6 +50,7 @@ int hn_logtype_driver;
 #define NETVSC_ARG_LATENCY "latency"
 #define NETVSC_ARG_RXBREAK "rx_copybreak"
 #define NETVSC_ARG_TXBREAK "tx_copybreak"
+#define NETVSC_ARG_RX_EXTMBUF_ENABLE "rx_extmbuf_enable"
 
 struct hn_xstats_name_off {
 	char name[RTE_ETH_XSTATS_NAME_SIZE];
@@ -168,6 +169,10 @@ static int hn_set_parameter(const char *key, const char *value, void *opaque)
 		hv->tx_copybreak = v;
 		PMD_DRV_LOG(DEBUG, "tx copy break set to %u",
 			    hv->tx_copybreak);
+	} else if (!strcmp(key, NETVSC_ARG_RX_EXTMBUF_ENABLE)) {
+		hv->rx_extmbuf_enable = v;
+		PMD_DRV_LOG(DEBUG, "rx extmbuf enable set to %u",
+			    hv->rx_extmbuf_enable);
 	}
 
 	return 0;
@@ -182,6 +187,7 @@ static int hn_parse_args(const struct rte_eth_dev *dev)
 		NETVSC_ARG_LATENCY,
 		NETVSC_ARG_RXBREAK,
 		NETVSC_ARG_TXBREAK,
+		NETVSC_ARG_RX_EXTMBUF_ENABLE,
 		NULL
 	};
 	struct rte_kvargs *kvlist;
@@ -970,6 +976,7 @@ eth_hn_dev_init(struct rte_eth_dev *eth_dev)
 	hv->latency = HN_CHAN_LATENCY_NS;
 	hv->rx_copybreak = HN_RXCOPY_THRESHOLD;
 	hv->tx_copybreak = HN_TXCOPY_THRESHOLD;
+	hv->rx_extmbuf_enable = HN_RX_EXTMBUF_ENABLE;
 	hv->max_queues = 1;
 	rte_spinlock_init(&hv->vf_lock);
 	hv->vf_port = HN_INVALID_PORT;
@@ -1141,4 +1148,5 @@ RTE_INIT(hn_init_log)
 RTE_PMD_REGISTER_PARAM_STRING(net_netvsc,
 			      NETVSC_ARG_LATENCY "=<uint32> "
 			      NETVSC_ARG_RXBREAK "=<uint32> "
-			      NETVSC_ARG_TXBREAK "=<uint32>");
+			      NETVSC_ARG_TXBREAK "=<uint32> "
+			      NETVSC_ARG_RX_EXTMBUF_ENABLE "=<0|1>");
diff --git a/drivers/net/netvsc/hn_rxtx.c b/drivers/net/netvsc/hn_rxtx.c
index 1cbb2ccee5..e1cf15c090 100644
--- a/drivers/net/netvsc/hn_rxtx.c
+++ b/drivers/net/netvsc/hn_rxtx.c
@@ -538,7 +538,7 @@ static void hn_rxpkt(struct hn_rx_queue *rxq, struct hn_rx_bufinfo *rxb,
 	 * For large packets, avoid copy if possible but need to keep
 	 * some space available in receive area for later packets.
 	 */
-	if (dlen > hv->rx_copybreak &&
+	if (hv->rx_extmbuf_enable && dlen > hv->rx_copybreak &&
 	    (uint32_t)rte_atomic32_read(&rxq->rxbuf_outstanding) <
 			hv->rxbuf_section_cnt / 2) {
 		struct rte_mbuf_ext_shared_info *shinfo;
diff --git a/drivers/net/netvsc/hn_var.h b/drivers/net/netvsc/hn_var.h
index 50ca1e56ca..61593827af 100644
--- a/drivers/net/netvsc/hn_var.h
+++ b/drivers/net/netvsc/hn_var.h
@@ -26,6 +26,8 @@
 #define HN_TXCOPY_THRESHOLD	512
 #define HN_RXCOPY_THRESHOLD	256
 
+#define HN_RX_EXTMBUF_ENABLE	0
+
 /* Buffers need to be aligned */
 #ifndef PAGE_SIZE
 #define PAGE_SIZE 4096
@@ -119,6 +121,7 @@ struct hn_data {
 	struct rte_mem_resource *rxbuf_res;	/* UIO resource for Rx */
 	uint32_t	rxbuf_section_cnt;	/* # of Rx sections */
 	uint32_t	rx_copybreak;
+	uint32_t	rx_extmbuf_enable;
 	uint16_t	max_queues;		/* Max available queues */
 	uint16_t	num_queues;
 	uint64_t	rss_offloads;
-- 
2.27.0

Re: [dpdk-stable] [PATCH 19.11 2/2] net/netvsc: control use of external mbuf on Rx

[Luca Boccassi]

On Wed, 2020-12-09 at 16:27 -0800, Long Li wrote:
> From: Long Li <longli@microsoft.com>
> 
> [ upstream commit 096b31fc0d8c989cc455c35f4d1def24a4ed6dee ]
> 
> When receiving packets, netvsp puts data in a buffer mapped through UIO.
> Depending on packet size, netvsc may attach the buffer as an external
> mbuf. This is not a problem if this mbuf is consumed in the application,
> and the application can correctly read data out of an external mbuf.
> 
> However, there are two problems with data in an external mbuf.
> 1. Due to the limitation of the kernel UIO implementation, physical
>    address of this external buffer is not exposed to the user-mode. If
>    this mbuf is passed to another driver, the other driver is unable to
>    map this buffer to iova.
> 2. Some DPDK applications are not aware of external mbuf, and may bug
>    when they receive an mbuf with external buffer attached.
> 
> Introduce a driver parameter "rx_extmbuf_enable" to control if netvsc
> should use external mbuf for receiving packets. The default value is 0.
> (netvsc doesn't use external mbuf, it always allocates mbuf and copy
> data to mbuf) A non-zero value tells netvsc to attach external buffers
> to mbuf on receiving packets, thus avoid copying memory.
> 
> Signed-off-by: Long Li <longli@microsoft.com>
> ---
>  doc/guides/nics/netvsc.rst     |  8 ++++++++
>  drivers/net/netvsc/hn_ethdev.c | 10 +++++++++-
>  drivers/net/netvsc/hn_rxtx.c   |  2 +-
>  drivers/net/netvsc/hn_var.h    |  3 +++
>  4 files changed, 21 insertions(+), 2 deletions(-)

Correct me if I'm wrong, but these 2 patches look a bit more like new
features than bug fixes? It's new options for the PMD, right?

In general, we do not take new features in LTS releases. Stable means
stable - we make very few exceptions.

Is the PMD broken/unusable without these options?

-- 
Kind regards,
Luca Boccassi

Re: [dpdk-stable] [PATCH 19.11 2/2] net/netvsc: control use of external mbuf on Rx

[Long Li]

> Subject: Re: [dpdk-stable] [PATCH 19.11 2/2] net/netvsc: control use of
> external mbuf on Rx
> 
> On Wed, 2020-12-09 at 16:27 -0800, Long Li wrote:
> > From: Long Li <longli@microsoft.com>
> >
> > [ upstream commit 096b31fc0d8c989cc455c35f4d1def24a4ed6dee ]
> >
> > When receiving packets, netvsp puts data in a buffer mapped through UIO.
> > Depending on packet size, netvsc may attach the buffer as an external
> > mbuf. This is not a problem if this mbuf is consumed in the
> > application, and the application can correctly read data out of an external
> mbuf.
> >
> > However, there are two problems with data in an external mbuf.
> > 1. Due to the limitation of the kernel UIO implementation, physical
> >    address of this external buffer is not exposed to the user-mode. If
> >    this mbuf is passed to another driver, the other driver is unable to
> >    map this buffer to iova.
> > 2. Some DPDK applications are not aware of external mbuf, and may bug
> >    when they receive an mbuf with external buffer attached.
> >
> > Introduce a driver parameter "rx_extmbuf_enable" to control if netvsc
> > should use external mbuf for receiving packets. The default value is 0.
> > (netvsc doesn't use external mbuf, it always allocates mbuf and copy
> > data to mbuf) A non-zero value tells netvsc to attach external buffers
> > to mbuf on receiving packets, thus avoid copying memory.
> >
> > Signed-off-by: Long Li <longli@microsoft.com>
> > ---
> >  doc/guides/nics/netvsc.rst     |  8 ++++++++
> >  drivers/net/netvsc/hn_ethdev.c | 10 +++++++++-
> >  drivers/net/netvsc/hn_rxtx.c   |  2 +-
> >  drivers/net/netvsc/hn_var.h    |  3 +++
> >  4 files changed, 21 insertions(+), 2 deletions(-)
> 
> Correct me if I'm wrong, but these 2 patches look a bit more like new
> features than bug fixes? It's new options for the PMD, right?
> 
> In general, we do not take new features in LTS releases. Stable means stable
> - we make very few exceptions.
> 
> Is the PMD broken/unusable without these options?

This patch changes the default behavior of netvsc PMD to not use external mbufs on receiving data.

Using external mbufs has shown problems in many applications. Changing the default behavior will fix those.

Thanks,
Long

> 
> --
> Kind regards,
> Luca Boccassi

Re: [dpdk-stable] [PATCH 19.11 2/2] net/netvsc: control use of external mbuf on Rx

[Luca Boccassi]

On Thu, 2020-12-10 at 19:44 +0000, Long Li wrote:
> > Subject: Re: [dpdk-stable] [PATCH 19.11 2/2] net/netvsc: control use of
> > external mbuf on Rx
> > 
> > On Wed, 2020-12-09 at 16:27 -0800, Long Li wrote:
> > > From: Long Li <longli@microsoft.com>
> > > 
> > > [ upstream commit 096b31fc0d8c989cc455c35f4d1def24a4ed6dee ]
> > > 
> > > When receiving packets, netvsp puts data in a buffer mapped through UIO.
> > > Depending on packet size, netvsc may attach the buffer as an external
> > > mbuf. This is not a problem if this mbuf is consumed in the
> > > application, and the application can correctly read data out of an external
> > mbuf.
> > > However, there are two problems with data in an external mbuf.
> > > 1. Due to the limitation of the kernel UIO implementation, physical
> > >    address of this external buffer is not exposed to the user-mode. If
> > >    this mbuf is passed to another driver, the other driver is unable to
> > >    map this buffer to iova.
> > > 2. Some DPDK applications are not aware of external mbuf, and may bug
> > >    when they receive an mbuf with external buffer attached.
> > > 
> > > Introduce a driver parameter "rx_extmbuf_enable" to control if netvsc
> > > should use external mbuf for receiving packets. The default value is 0.
> > > (netvsc doesn't use external mbuf, it always allocates mbuf and copy
> > > data to mbuf) A non-zero value tells netvsc to attach external buffers
> > > to mbuf on receiving packets, thus avoid copying memory.
> > > 
> > > Signed-off-by: Long Li <longli@microsoft.com>
> > > ---
> > >  doc/guides/nics/netvsc.rst     |  8 ++++++++
> > >  drivers/net/netvsc/hn_ethdev.c | 10 +++++++++-
> > >  drivers/net/netvsc/hn_rxtx.c   |  2 +-
> > >  drivers/net/netvsc/hn_var.h    |  3 +++
> > >  4 files changed, 21 insertions(+), 2 deletions(-)
> > 
> > Correct me if I'm wrong, but these 2 patches look a bit more like new
> > features than bug fixes? It's new options for the PMD, right?
> > 
> > In general, we do not take new features in LTS releases. Stable means stable
> > - we make very few exceptions.
> > 
> > Is the PMD broken/unusable without these options?
> 
> This patch changes the default behavior of netvsc PMD to not use external mbufs on receiving data.
> 
> Using external mbufs has shown problems in many applications. Changing the default behavior will fix those.

Sorry but I don't think adding new options is appropriate for an LTS
release.
The expectations is that everything works as-is without any need to
even recompile, let alone changing options.

-- 
Kind regards,
Luca Boccassi

Re: [dpdk-stable] [PATCH 19.11 2/2] net/netvsc: control use of external mbuf on Rx

[Stephen Hemminger]

On Tue, 15 Dec 2020 13:48:57 +0000
Luca Boccassi <bluca@debian.org> wrote:

> On Thu, 2020-12-10 at 19:44 +0000, Long Li wrote:
> > > Subject: Re: [dpdk-stable] [PATCH 19.11 2/2] net/netvsc: control use of
> > > external mbuf on Rx
> > > 
> > > On Wed, 2020-12-09 at 16:27 -0800, Long Li wrote:  
> > > > From: Long Li <longli@microsoft.com>
> > > > 
> > > > [ upstream commit 096b31fc0d8c989cc455c35f4d1def24a4ed6dee ]
> > > > 
> > > > When receiving packets, netvsp puts data in a buffer mapped through UIO.
> > > > Depending on packet size, netvsc may attach the buffer as an external
> > > > mbuf. This is not a problem if this mbuf is consumed in the
> > > > application, and the application can correctly read data out of an external  
> > > mbuf.  
> > > > However, there are two problems with data in an external mbuf.
> > > > 1. Due to the limitation of the kernel UIO implementation, physical
> > > >    address of this external buffer is not exposed to the user-mode. If
> > > >    this mbuf is passed to another driver, the other driver is unable to
> > > >    map this buffer to iova.
> > > > 2. Some DPDK applications are not aware of external mbuf, and may bug
> > > >    when they receive an mbuf with external buffer attached.
> > > > 
> > > > Introduce a driver parameter "rx_extmbuf_enable" to control if netvsc
> > > > should use external mbuf for receiving packets. The default value is 0.
> > > > (netvsc doesn't use external mbuf, it always allocates mbuf and copy
> > > > data to mbuf) A non-zero value tells netvsc to attach external buffers
> > > > to mbuf on receiving packets, thus avoid copying memory.
> > > > 
> > > > Signed-off-by: Long Li <longli@microsoft.com>
> > > > ---
> > > >  doc/guides/nics/netvsc.rst     |  8 ++++++++
> > > >  drivers/net/netvsc/hn_ethdev.c | 10 +++++++++-
> > > >  drivers/net/netvsc/hn_rxtx.c   |  2 +-
> > > >  drivers/net/netvsc/hn_var.h    |  3 +++
> > > >  4 files changed, 21 insertions(+), 2 deletions(-)  
> > > 
> > > Correct me if I'm wrong, but these 2 patches look a bit more like new
> > > features than bug fixes? It's new options for the PMD, right?
> > > 
> > > In general, we do not take new features in LTS releases. Stable means stable
> > > - we make very few exceptions.
> > > 
> > > Is the PMD broken/unusable without these options?  
> > 
> > This patch changes the default behavior of netvsc PMD to not use external mbufs on receiving data.
> > 
> > Using external mbufs has shown problems in many applications. Changing the default behavior will fix those.  
> 
> Sorry but I don't think adding new options is appropriate for an LTS
> release.
> The expectations is that everything works as-is without any need to
> even recompile, let alone changing options.
> 

Can we just turn external mbuf usage in this driver off for the stable version instead?

Re: [dpdk-stable] [PATCH 19.11 2/2] net/netvsc: control use of external mbuf on Rx

[Long Li]

> Subject: Re: [dpdk-stable] [PATCH 19.11 2/2] net/netvsc: control use of
> external mbuf on Rx
> 
> On Tue, 15 Dec 2020 13:48:57 +0000
> Luca Boccassi <bluca@debian.org> wrote:
> 
> > On Thu, 2020-12-10 at 19:44 +0000, Long Li wrote:
> > > > Subject: Re: [dpdk-stable] [PATCH 19.11 2/2] net/netvsc: control
> > > > use of external mbuf on Rx
> > > >
> > > > On Wed, 2020-12-09 at 16:27 -0800, Long Li wrote:
> > > > > From: Long Li <longli@microsoft.com>
> > > > >
> > > > > [ upstream commit 096b31fc0d8c989cc455c35f4d1def24a4ed6dee ]
> > > > >
> > > > > When receiving packets, netvsp puts data in a buffer mapped through
> UIO.
> > > > > Depending on packet size, netvsc may attach the buffer as an
> > > > > external mbuf. This is not a problem if this mbuf is consumed in
> > > > > the application, and the application can correctly read data out
> > > > > of an external
> > > > mbuf.
> > > > > However, there are two problems with data in an external mbuf.
> > > > > 1. Due to the limitation of the kernel UIO implementation, physical
> > > > >    address of this external buffer is not exposed to the user-mode. If
> > > > >    this mbuf is passed to another driver, the other driver is unable to
> > > > >    map this buffer to iova.
> > > > > 2. Some DPDK applications are not aware of external mbuf, and may
> bug
> > > > >    when they receive an mbuf with external buffer attached.
> > > > >
> > > > > Introduce a driver parameter "rx_extmbuf_enable" to control if
> > > > > netvsc should use external mbuf for receiving packets. The default
> value is 0.
> > > > > (netvsc doesn't use external mbuf, it always allocates mbuf and
> > > > > copy data to mbuf) A non-zero value tells netvsc to attach
> > > > > external buffers to mbuf on receiving packets, thus avoid copying
> memory.
> > > > >
> > > > > Signed-off-by: Long Li <longli@microsoft.com>
> > > > > ---
> > > > >  doc/guides/nics/netvsc.rst     |  8 ++++++++
> > > > >  drivers/net/netvsc/hn_ethdev.c | 10 +++++++++-
> > > > >  drivers/net/netvsc/hn_rxtx.c   |  2 +-
> > > > >  drivers/net/netvsc/hn_var.h    |  3 +++
> > > > >  4 files changed, 21 insertions(+), 2 deletions(-)
> > > >
> > > > Correct me if I'm wrong, but these 2 patches look a bit more like
> > > > new features than bug fixes? It's new options for the PMD, right?
> > > >
> > > > In general, we do not take new features in LTS releases. Stable
> > > > means stable
> > > > - we make very few exceptions.
> > > >
> > > > Is the PMD broken/unusable without these options?
> > >
> > > This patch changes the default behavior of netvsc PMD to not use
> external mbufs on receiving data.
> > >
> > > Using external mbufs has shown problems in many applications. Changing
> the default behavior will fix those.
> >
> > Sorry but I don't think adding new options is appropriate for an LTS
> > release.
> > The expectations is that everything works as-is without any need to
> > even recompile, let alone changing options.

This patch effectively disables external mbuf without any changes to application.

> >
> 
> Can we just turn external mbuf usage in this driver off for the stable version
> instead?

That will require a separate patch not in upstream. If that's acceptable I can submit another patch. But this will create some confusion to someone doing bisecting in 19.11.