> From: yang_y_yi <yang_y...@163.com> > Sent: Wednesday, October 14, 2020 3:56 AM > To: Hu, Jiayu <jiayu...@intel.com> > Cc: Ananyev, Konstantin <konstantin.anan...@intel.com>; dev@dpdk.org; > olivier.m...@6wind.com; tho...@monjalon.net; > yangy...@inspur.com > Subject: Re:RE: [PATCH] gso: fix free issue of mbuf gso segments attach to > > I think it isn't a good idea to free it in rte_gso_segment, maybe application > will continue to use this pkt for other purpose, rte_gso_segment > can't make decision for application without any notice, it is better to > return this decision right backt to application. >
I think, if user wants to keep original packet, he can always call rte_pktmbuf_refcnt_update(pkt, 1) just before calling gso function. Also, as I remember in some cases it is not safe to do free() for input packet (as pkt_out[] can contain input pkt itself). Would it also be user responsibility to determine such situations? P.S. Please don't reply on the top. Konstantin > > > > > At 2020-10-14 09:00:12, "Hu, Jiayu" <mailto:jiayu...@intel.com> wrote: > > > > > >> -----Original Message----- > >> From: Ananyev, Konstantin <mailto:konstantin.anan...@intel.com> > >> Sent: Tuesday, October 13, 2020 11:39 PM > >> To: Hu, Jiayu <mailto:jiayu...@intel.com>; mailto:yang_y...@163.com; > >> mailto:dev@dpdk.org > >> Cc: mailto:mark.b.kavan...@intel.com; mailto:olivier.m...@6wind.com; > >> mailto:tho...@monjalon.net; mailto:yangy...@inspur.com > >> Subject: RE: [PATCH] gso: fix free issue of mbuf gso segments attach to > >> > >> > >> > > rte_gso_segment decreased refcnt of pkt by one, but > >> > > it is wrong if pkt is external mbuf, pkt won't be > >> > > freed because of incorrect refcnt, the result is > >> > > application can't allocate mbuf from mempool because > >> > > mbufs in mempool are run out of. > >> > > > >> > > One correct way is application should call > >> > > rte_pktmbuf_free after calling rte_gso_segment to free > >> > > pkt explicitly. rte_gso_segment mustn't handle it, this > >> > > should be responsibility of application. > >> > > >> > GSO doesn't support the input pktmbuf has external buffer. > >> > Indeed, requiring users to free the input pktmbuf can avoid > >> > memory leak, but I'm afraid that it also changes the semantic > >> > of rte_gso_segment() which is defined in rte_gso.h. > >> > > >> > @Konstantin, any suggestions? > >> > >> Probably, a stupid question, but why can't we call mbuf_free() > >> here instead fo decrementing refcnt manually: > >> if (ret > 1) > >> rte_pktmbuf_free(pkt); > >> else if ... > >> ? > > > >You are right. Freeing mbuf inside GSO is a better way to solve > >the problem. > > > >Thanks, > >Jiayu > >> > >> > >> > >> > > >> > Thanks, > >> > Jiayu > >> > > > >> > > Fixes: 119583797b6a ("gso: support TCP/IPv4 GSO") > >> > > Signed-off-by: Yi Yang <mailto:yangy...@inspur.com> > >> > > --- > >> > > doc/guides/prog_guide/generic_segmentation_offload_lib.rst | 7 +++++- > >> - > >> > > lib/librte_gso/rte_gso.c | 9 > >> > > +-------- > >> > > 2 files changed, 6 insertions(+), 10 deletions(-) > >> > > > >> > > diff --git a/doc/guides/prog_guide/generic_segmentation_offload_lib.rst > >> > > b/doc/guides/prog_guide/generic_segmentation_offload_lib.rst > >> > > index 205cb8a..8577572 100644 > >> > > --- a/doc/guides/prog_guide/generic_segmentation_offload_lib.rst > >> > > +++ b/doc/guides/prog_guide/generic_segmentation_offload_lib.rst > >> > > @@ -25,8 +25,9 @@ Bearing that in mind, the GSO library enables DPDK > >> > > applications to segment > >> > > packets in software. Note however, that GSO is implemented as a > >> > > standalone > >> > > library, and not via a 'fallback' mechanism (i.e. for when TSO is > >> unsupported > >> > > in the underlying hardware); that is, applications must explicitly > >> > > invoke > >> the > >> > > -GSO library to segment packets. The size of GSO segments ``(segsz)`` > >> > > is > >> > > -configurable by the application. > >> > > +GSO library to segment packets, they also must call > >> ``rte_pktmbuf_free()`` to > >> > > +free mbuf GSO segments attach to after calling ``rte_gso_segment()``. > >> The > >> > > size > >> > > +of GSO segments ``(segsz)`` is configurable by the application. > >> > > > >> > > Limitations > >> > > ----------- > >> > > @@ -233,6 +234,8 @@ To segment an outgoing packet, an application > >> must: > >> > > > >> > > #. Invoke the GSO segmentation API, ``rte_gso_segment()``. > >> > > > >> > > +#. Call ``rte_pktmbuf_free()`` to free mbuf ``rte_gso_segment()`` > >> segments. > >> > > + > >> > > #. If required, update the L3 and L4 checksums of the newly-created > >> > > segments. > >> > > For tunneled packets, the outer IPv4 headers' checksums should also > >> be > >> > > updated. Alternatively, the application may offload checksum > >> calculation > >> > > diff --git a/lib/librte_gso/rte_gso.c b/lib/librte_gso/rte_gso.c > >> > > index 751b5b6..0d6cae5 100644 > >> > > --- a/lib/librte_gso/rte_gso.c > >> > > +++ b/lib/librte_gso/rte_gso.c > >> > > @@ -30,7 +30,6 @@ > >> > > uint16_t nb_pkts_out) > >> > > { > >> > > struct rte_mempool *direct_pool, *indirect_pool; > >> > > -struct rte_mbuf *pkt_seg; > >> > > uint64_t ol_flags; > >> > > uint16_t gso_size; > >> > > uint8_t ipid_delta; > >> > > @@ -80,13 +79,7 @@ > >> > > return 1; > >> > > } > >> > > > >> > > -if (ret > 1) { > >> > > -pkt_seg = pkt; > >> > > -while (pkt_seg) { > >> > > -rte_mbuf_refcnt_update(pkt_seg, -1); > >> > > -pkt_seg = pkt_seg->next; > >> > > -} > >> > > -} else if (ret < 0) { > >> > > +if (ret < 0) { > >> > > /* Revert the ol_flags in the event of failure. */ > >> > > pkt->ol_flags = ol_flags; > >> > > } > >> > > -- > >> > > 1.8.3.1 > >> > > >> > >
