| Message ID | 1592961106-82820-1-git-send-email-longli@linuxonhyperv.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Ferruh Yigit |
| Headers | show |
| Series | [1/2] net/netvsc: fix underflow error when external mbuf are used in the receive path | expand |
| Context | Check | Description |
|---|---|---|
| ci/checkpatch | warning | coding style issues |
| ci/iol-broadcom-Performance | success | Performance Testing PASS |
| ci/iol-intel-Performance | success | Performance Testing PASS |
| ci/Intel-compilation | success | Compilation OK |
| ci/iol-nxp-Performance | success | Performance Testing PASS |
| ci/iol-mellanox-Performance | success | Performance Testing PASS |
| ci/iol-testing | success | Testing PASS |
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>
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.
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;