[1/2] net/netvsc: fix underflow error when external mbuf are used in the receive path
diff mbox series

Message ID 1592961106-82820-1-git-send-email-longli@linuxonhyperv.com
State Accepted
Delegated to: Ferruh Yigit
Headers show
Series
  • [1/2] net/netvsc: fix underflow error when external mbuf are used in the receive path
Related show

Checks

Context Check Description
ci/iol-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-nxp-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/checkpatch warning coding style issues

Commit Message

Long Li June 24, 2020, 1:11 a.m. UTC
From: Long Li <longli@microsoft.com>

When rte_pktmbuf_attach_extbuf() is used, the driver should not decrease the
reference count in its callback function hn_rx_buf_free_cb, because the
reference count is already decreased by rte_pktmbuf. Doing it twice may result
in underflow and driver may never send an ack packet over vmbus to host.

Also declares rxbuf_outstanding as atomic, because this value is shared
among all receive queues.

Signed-off-by: Long Li <longli@microsoft.com>
---
 drivers/net/netvsc/hn_nvs.c  |  1 -
 drivers/net/netvsc/hn_rxtx.c | 30 +++++++++++-------------------
 drivers/net/netvsc/hn_var.h  |  2 +-
 3 files changed, 12 insertions(+), 21 deletions(-)

Comments

Stephen Hemminger July 2, 2020, 12:06 a.m. UTC | #1
On Tue, 23 Jun 2020 18:11:45 -0700
longli@linuxonhyperv.com wrote:

> From: Long Li <longli@microsoft.com>
> 
> When rte_pktmbuf_attach_extbuf() is used, the driver should not decrease the
> reference count in its callback function hn_rx_buf_free_cb, because the
> reference count is already decreased by rte_pktmbuf. Doing it twice may result
> in underflow and driver may never send an ack packet over vmbus to host.
> 
> Also declares rxbuf_outstanding as atomic, because this value is shared
> among all receive queues.
> 
> Signed-off-by: Long Li <longli@microsoft.com>

Good catch. DPDK ought to have a real refcnt type like the kernel.
One that traps on underflow.

Acked-by: Stephen Hemminger <stephen@networkplumber.org>
Ferruh Yigit July 9, 2020, 3:28 p.m. UTC | #2
On 7/2/2020 1:06 AM, Stephen Hemminger wrote:
> On Tue, 23 Jun 2020 18:11:45 -0700
> longli@linuxonhyperv.com wrote:
> 
>> From: Long Li <longli@microsoft.com>
>>
>> When rte_pktmbuf_attach_extbuf() is used, the driver should not decrease the
>> reference count in its callback function hn_rx_buf_free_cb, because the
>> reference count is already decreased by rte_pktmbuf. Doing it twice may result
>> in underflow and driver may never send an ack packet over vmbus to host.
>>
>> Also declares rxbuf_outstanding as atomic, because this value is shared
>> among all receive queues.
>>
>> Signed-off-by: Long Li <longli@microsoft.com>
> 
> Good catch. DPDK ought to have a real refcnt type like the kernel.
> One that traps on underflow.
> 
> Acked-by: Stephen Hemminger <stephen@networkplumber.org>
> 

Series applied to dpdk-next-net/master, thanks.

Patch
diff mbox series

diff --git a/drivers/net/netvsc/hn_nvs.c b/drivers/net/netvsc/hn_nvs.c
index 0fa74cd46..466b1948e 100644
--- a/drivers/net/netvsc/hn_nvs.c
+++ b/drivers/net/netvsc/hn_nvs.c
@@ -99,7 +99,6 @@  __hn_nvs_execute(struct hn_data *hv,
 	/* Silently drop received packets while waiting for response */
 	if (hdr->type == NVS_TYPE_RNDIS) {
 		hn_nvs_ack_rxbuf(chan, xactid);
-		--hv->rxbuf_outstanding;
 		goto retry;
 	}
 
diff --git a/drivers/net/netvsc/hn_rxtx.c b/drivers/net/netvsc/hn_rxtx.c
index a4f6d1b6a..aece38e2d 100644
--- a/drivers/net/netvsc/hn_rxtx.c
+++ b/drivers/net/netvsc/hn_rxtx.c
@@ -519,24 +519,13 @@  hn_rndis_rxinfo(const void *info_data, unsigned int info_dlen,
 	return 0;
 }
 
-/*
- * Ack the consumed RXBUF associated w/ this channel packet,
- * so that this RXBUF can be recycled by the hypervisor.
- */
-static void hn_rx_buf_release(struct hn_rx_bufinfo *rxb)
+static void hn_rx_buf_free_cb(void *buf __rte_unused, void *opaque)
 {
-	struct rte_mbuf_ext_shared_info *shinfo = &rxb->shinfo;
+	struct hn_rx_bufinfo *rxb = opaque;
 	struct hn_data *hv = rxb->hv;
 
-	if (rte_mbuf_ext_refcnt_update(shinfo, -1) == 0) {
-		hn_nvs_ack_rxbuf(rxb->chan, rxb->xactid);
-		--hv->rxbuf_outstanding;
-	}
-}
-
-static void hn_rx_buf_free_cb(void *buf __rte_unused, void *opaque)
-{
-	hn_rx_buf_release(opaque);
+	rte_atomic32_dec(&hv->rxbuf_outstanding);
+	hn_nvs_ack_rxbuf(rxb->chan, rxb->xactid);
 }
 
 static struct hn_rx_bufinfo *hn_rx_buf_init(const struct hn_rx_queue *rxq,
@@ -576,7 +565,8 @@  static void hn_rxpkt(struct hn_rx_queue *rxq, struct hn_rx_bufinfo *rxb,
 	 * some space available in receive area for later packets.
 	 */
 	if (dlen >= HN_RXCOPY_THRESHOLD &&
-	    hv->rxbuf_outstanding < hv->rxbuf_section_cnt / 2) {
+	    (uint32_t)rte_atomic32_read(&hv->rxbuf_outstanding) <
+			hv->rxbuf_section_cnt / 2) {
 		struct rte_mbuf_ext_shared_info *shinfo;
 		const void *rxbuf;
 		rte_iova_t iova;
@@ -590,8 +580,9 @@  static void hn_rxpkt(struct hn_rx_queue *rxq, struct hn_rx_bufinfo *rxb,
 		iova = rte_mem_virt2iova(rxbuf) + RTE_PTR_DIFF(data, rxbuf);
 		shinfo = &rxb->shinfo;
 
-		if (rte_mbuf_ext_refcnt_update(shinfo, 1) == 1)
-			++hv->rxbuf_outstanding;
+		/* shinfo is already set to 1 by the caller */
+		if (rte_mbuf_ext_refcnt_update(shinfo, 1) == 2)
+			rte_atomic32_inc(&hv->rxbuf_outstanding);
 
 		rte_pktmbuf_attach_extbuf(m, data, iova,
 					  dlen + headroom, shinfo);
@@ -832,7 +823,8 @@  hn_nvs_handle_rxbuf(struct rte_eth_dev *dev,
 	}
 
 	/* Send ACK now if external mbuf not used */
-	hn_rx_buf_release(rxb);
+	if (rte_mbuf_ext_refcnt_update(&rxb->shinfo, -1) == 0)
+		hn_nvs_ack_rxbuf(rxb->chan, rxb->xactid);
 }
 
 /*
diff --git a/drivers/net/netvsc/hn_var.h b/drivers/net/netvsc/hn_var.h
index 881832d85..7cb7713e9 100644
--- a/drivers/net/netvsc/hn_var.h
+++ b/drivers/net/netvsc/hn_var.h
@@ -113,7 +113,7 @@  struct hn_data {
 	struct rte_mem_resource *rxbuf_res;	/* UIO resource for Rx */
 	struct hn_rx_bufinfo *rxbuf_info;
 	uint32_t	rxbuf_section_cnt;	/* # of Rx sections */
-	volatile uint32_t rxbuf_outstanding;
+	rte_atomic32_t	rxbuf_outstanding;
 	uint16_t	max_queues;		/* Max available queues */
 	uint16_t	num_queues;
 	uint64_t	rss_offloads;