| Message ID | 1457401848-9191-1-git-send-email-yuanhan.liu@linux.intel.com |
|---|---|
| State | Accepted |
| Headers | show |
Thanks for the patch! Were you able to hit this bug in your setup or did you just find this by code inspection? I'm asking because I'm wondering whether we should backport the fix. In any case I've applied this to master and added your name to the AUTHORS file, thanks! On 07/03/2016 17:50, "dev on behalf of Yuanhan Liu" <dev-bounces@openvswitch.org on behalf of yuanhan.liu@linux.intel.com> wrote: >mbufs could be chained (by the "next" field of rte_mbuf struct), when >an mbuf is not big enough to hold a big packet, say when TSO is enabled. > >rte_pktmbuf_free_seg() frees the head mbuf only, leading mbuf leaks. >This patch fix it by invoking the right API rte_pktmbuf_free(), to >free all mbufs in the chain. > >Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> >--- > lib/netdev-dpdk.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > >diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >index 0233b3c..f402354 100644 >--- a/lib/netdev-dpdk.c >+++ b/lib/netdev-dpdk.c >@@ -396,7 +396,7 @@ free_dpdk_buf(struct dp_packet *p) > { > struct rte_mbuf *pkt = (struct rte_mbuf *) p; > >- rte_pktmbuf_free_seg(pkt); >+ rte_pktmbuf_free(pkt); > } > > static void >@@ -1089,7 +1089,7 @@ dpdk_queue_flush__(struct netdev_dpdk *dev, int qid) > int i; > > for (i = nb_tx; i < txq->count; i++) { >- rte_pktmbuf_free_seg(txq->burst_pkts[i]); >+ rte_pktmbuf_free(txq->burst_pkts[i]); > } > rte_spinlock_lock(&dev->stats_lock); > dev->stats.tx_dropped += txq->count-nb_tx; >-- >1.9.0 > >_______________________________________________ >dev mailing list >dev@openvswitch.org >http://openvswitch.org/mailman/listinfo/dev
On Thu, Mar 10, 2016 at 01:33:03AM +0000, Daniele Di Proietto wrote: > Thanks for the patch! > > Were you able to hit this bug in your setup or did you just > find this by code inspection? Yes, it's a 100% reproducible bug if you have TSO enabled, which is a feature has been merged for DPDK v2.3 (well, it's been renamed to v16.04). > I'm asking because I'm wondering whether we should backport > the fix. I would say yes. I mean, you should really use rte_pktmbuf_free(), the right API to free a mbuf. rte_pktmbuf_free_segs() is more like an internal helper function used by rte_pktmbuf_free(). > In any case I've applied this to master and added your name > to the AUTHORS file, thanks! Thanks. --yliu > > On 07/03/2016 17:50, "dev on behalf of Yuanhan Liu" > <dev-bounces@openvswitch.org on behalf of yuanhan.liu@linux.intel.com> > wrote: > > >mbufs could be chained (by the "next" field of rte_mbuf struct), when > >an mbuf is not big enough to hold a big packet, say when TSO is enabled. > > > >rte_pktmbuf_free_seg() frees the head mbuf only, leading mbuf leaks. > >This patch fix it by invoking the right API rte_pktmbuf_free(), to > >free all mbufs in the chain. > > > >Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> > >--- > > lib/netdev-dpdk.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > >diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > >index 0233b3c..f402354 100644 > >--- a/lib/netdev-dpdk.c > >+++ b/lib/netdev-dpdk.c > >@@ -396,7 +396,7 @@ free_dpdk_buf(struct dp_packet *p) > > { > > struct rte_mbuf *pkt = (struct rte_mbuf *) p; > > > >- rte_pktmbuf_free_seg(pkt); > >+ rte_pktmbuf_free(pkt); > > } > > > > static void > >@@ -1089,7 +1089,7 @@ dpdk_queue_flush__(struct netdev_dpdk *dev, int qid) > > int i; > > > > for (i = nb_tx; i < txq->count; i++) { > >- rte_pktmbuf_free_seg(txq->burst_pkts[i]); > >+ rte_pktmbuf_free(txq->burst_pkts[i]); > > } > > rte_spinlock_lock(&dev->stats_lock); > > dev->stats.tx_dropped += txq->count-nb_tx; > >-- > >1.9.0 > > > >_______________________________________________ > >dev mailing list > >dev@openvswitch.org > >http://openvswitch.org/mailman/listinfo/dev
Thanks for clarifying, I've back ported this up to branch-2.4 On 09/03/2016 17:46, "Yuanhan Liu" <yuanhan.liu@linux.intel.com> wrote: >On Thu, Mar 10, 2016 at 01:33:03AM +0000, Daniele Di Proietto wrote: >> Thanks for the patch! >> >> Were you able to hit this bug in your setup or did you just >> find this by code inspection? > >Yes, it's a 100% reproducible bug if you have TSO enabled, which is a >feature has been merged for DPDK v2.3 (well, it's been renamed to v16.04). > >> I'm asking because I'm wondering whether we should backport >> the fix. > >I would say yes. I mean, you should really use rte_pktmbuf_free(), >the right API to free a mbuf. rte_pktmbuf_free_segs() is more like >an internal helper function used by rte_pktmbuf_free(). > >> In any case I've applied this to master and added your name >> to the AUTHORS file, thanks! > >Thanks. > > --yliu >> >> On 07/03/2016 17:50, "dev on behalf of Yuanhan Liu" >> <dev-bounces@openvswitch.org on behalf of yuanhan.liu@linux.intel.com> >> wrote: >> >> >mbufs could be chained (by the "next" field of rte_mbuf struct), when >> >an mbuf is not big enough to hold a big packet, say when TSO is >>enabled. >> > >> >rte_pktmbuf_free_seg() frees the head mbuf only, leading mbuf leaks. >> >This patch fix it by invoking the right API rte_pktmbuf_free(), to >> >free all mbufs in the chain. >> > >> >Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> >> >--- >> > lib/netdev-dpdk.c | 4 ++-- >> > 1 file changed, 2 insertions(+), 2 deletions(-) >> > >> >diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> >index 0233b3c..f402354 100644 >> >--- a/lib/netdev-dpdk.c >> >+++ b/lib/netdev-dpdk.c >> >@@ -396,7 +396,7 @@ free_dpdk_buf(struct dp_packet *p) >> > { >> > struct rte_mbuf *pkt = (struct rte_mbuf *) p; >> > >> >- rte_pktmbuf_free_seg(pkt); >> >+ rte_pktmbuf_free(pkt); >> > } >> > >> > static void >> >@@ -1089,7 +1089,7 @@ dpdk_queue_flush__(struct netdev_dpdk *dev, int >>qid) >> > int i; >> > >> > for (i = nb_tx; i < txq->count; i++) { >> >- rte_pktmbuf_free_seg(txq->burst_pkts[i]); >> >+ rte_pktmbuf_free(txq->burst_pkts[i]); >> > } >> > rte_spinlock_lock(&dev->stats_lock); >> > dev->stats.tx_dropped += txq->count-nb_tx; >> >-- >> >1.9.0 >> > >> >_______________________________________________ >> >dev mailing list >> >dev@openvswitch.org >> >http://openvswitch.org/mailman/listinfo/dev
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 0233b3c..f402354 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -396,7 +396,7 @@ free_dpdk_buf(struct dp_packet *p) { struct rte_mbuf *pkt = (struct rte_mbuf *) p; - rte_pktmbuf_free_seg(pkt); + rte_pktmbuf_free(pkt); } static void @@ -1089,7 +1089,7 @@ dpdk_queue_flush__(struct netdev_dpdk *dev, int qid) int i; for (i = nb_tx; i < txq->count; i++) { - rte_pktmbuf_free_seg(txq->burst_pkts[i]); + rte_pktmbuf_free(txq->burst_pkts[i]); } rte_spinlock_lock(&dev->stats_lock); dev->stats.tx_dropped += txq->count-nb_tx;
mbufs could be chained (by the "next" field of rte_mbuf struct), when an mbuf is not big enough to hold a big packet, say when TSO is enabled. rte_pktmbuf_free_seg() frees the head mbuf only, leading mbuf leaks. This patch fix it by invoking the right API rte_pktmbuf_free(), to free all mbufs in the chain. Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> --- lib/netdev-dpdk.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)