[v2] gso: fix free issue of mbuf gso segments attach to
diff mbox series

Message ID 20201022065151.10108-1-yang_y_yi@163.com
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show
Series
  • [v2] gso: fix free issue of mbuf gso segments attach to
Related show

Checks

Context Check Description
ci/travis-robot success Travis build: passed
ci/Intel-compilation success Compilation OK
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/checkpatch warning coding style issues

Commit Message

yang_y_yi Oct. 22, 2020, 6:51 a.m. UTC
From: Yi Yang <yangyi01@inspur.com>

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.

Fixes: 119583797b6a ("gso: support TCP/IPv4 GSO")
Signed-off-by: Yi Yang <yangyi01@inspur.com>
---
Changelog:

v1->v2:
  - update description of rte_gso_segment().
  - change code which calls rte_gso_segment() to
    fix free issue.

---
 app/test-pmd/csumonly.c                                    | 3 ++-
 doc/guides/prog_guide/generic_segmentation_offload_lib.rst | 7 +++++--
 lib/librte_gso/rte_gso.c                                   | 9 +--------
 lib/librte_gso/rte_gso.h                                   | 7 +++++--
 4 files changed, 13 insertions(+), 13 deletions(-)

Comments

Ananyev, Konstantin Oct. 22, 2020, 1:16 p.m. UTC | #1
> 
> 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.

Probably needs to be stated clearly:
It is a change in functional behaviour.
Without deprecation note in advance.
TB members: please provide your opinion on that patch.

> 
> Fixes: 119583797b6a ("gso: support TCP/IPv4 GSO")
> Signed-off-by: Yi Yang <yangyi01@inspur.com>
> ---
> Changelog:
> 
> v1->v2:
>   - update description of rte_gso_segment().
>   - change code which calls rte_gso_segment() to
>     fix free issue.
> 
> ---
>  app/test-pmd/csumonly.c                                    | 3 ++-
>  doc/guides/prog_guide/generic_segmentation_offload_lib.rst | 7 +++++--

I think release notes also have to be updated.

>  lib/librte_gso/rte_gso.c                                   | 9 +--------
>  lib/librte_gso/rte_gso.h                                   | 7 +++++--
>  4 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
> index 3d7d244..829e07f 100644
> --- a/app/test-pmd/csumonly.c
> +++ b/app/test-pmd/csumonly.c
> @@ -1080,11 +1080,12 @@ struct simple_gre_hdr {
>  			ret = rte_gso_segment(pkts_burst[i], gso_ctx,
>  					&gso_segments[nb_segments],
>  					GSO_MAX_PKT_BURST - nb_segments);
> +			/* pkts_burst[i] can be freed safely here. */
> +			rte_pktmbuf_free(pkts_burst[i]);

It doesn't look correct to me.
I think it should be:
If (ret > 1) rte_pktmbuf_free(pkts_burst[i]);

>  			if (ret >= 0)
>  				nb_segments += ret;
>  			else {
>  				TESTPMD_LOG(DEBUG, "Unable to segment packet");
> -				rte_pktmbuf_free(pkts_burst[i]);
>  			}
>  		}


About drivers/net/tap/rte_eth_tap.c:
I think it has to be modified too, as here:

/* free original mbuf */
                rte_pktmbuf_free(mbuf_in);
                /* free tso mbufs */
                if (num_tso_mbufs > 0)
                        rte_pktmbuf_free_bulk(mbuf, num_tso_mbufs);

if mbuf[0] == mbuf_in
Will have a double free() for the same mbuf.

> 
> 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()``.

Probably worth to mention that if return code == 1, then
output mbuf will point to input mbuf and extra care with free() is required.

> 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;
>  	}
> diff --git a/lib/librte_gso/rte_gso.h b/lib/librte_gso/rte_gso.h
> index 3aab297..f6694f9 100644
> --- a/lib/librte_gso/rte_gso.h
> +++ b/lib/librte_gso/rte_gso.h
> @@ -89,8 +89,11 @@ struct rte_gso_ctx {
>   * the GSO segments are sent to should support transmission of multi-segment
>   * packets.
>   *
> - * If the input packet is GSO'd, its mbuf refcnt reduces by 1. Therefore,
> - * when all GSO segments are freed, the input packet is freed automatically.
> + * If the input packet is GSO'd, all the indirect segments are attached to the
> + * input packet.
> + *
> + * rte_gso_segment() will not free the input packet no matter whether it is
> + * GSO'd or not, the application should free it after call rte_gso_segment().
>   *
>   * If the memory space in pkts_out or MBUF pools is insufficient, this
>   * function fails, and it returns (-1) * errno. Otherwise, GSO succeeds,
> --
> 1.8.3.1
Ananyev, Konstantin Oct. 22, 2020, 3:33 p.m. UTC | #2
> >
> > 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.
> 
> Probably needs to be stated clearly:
> It is a change in functional behaviour.
> Without deprecation note in advance.
> TB members: please provide your opinion on that patch.
> 
> >
> > Fixes: 119583797b6a ("gso: support TCP/IPv4 GSO")
> > Signed-off-by: Yi Yang <yangyi01@inspur.com>
> > ---
> > Changelog:
> >
> > v1->v2:
> >   - update description of rte_gso_segment().
> >   - change code which calls rte_gso_segment() to
> >     fix free issue.
> >
> > ---
> >  app/test-pmd/csumonly.c                                    | 3 ++-
> >  doc/guides/prog_guide/generic_segmentation_offload_lib.rst | 7 +++++--
> 
> I think release notes also have to be updated.
> 
> >  lib/librte_gso/rte_gso.c                                   | 9 +--------
> >  lib/librte_gso/rte_gso.h                                   | 7 +++++--
> >  4 files changed, 13 insertions(+), 13 deletions(-)
> >
> > diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
> > index 3d7d244..829e07f 100644
> > --- a/app/test-pmd/csumonly.c
> > +++ b/app/test-pmd/csumonly.c
> > @@ -1080,11 +1080,12 @@ struct simple_gre_hdr {
> >  			ret = rte_gso_segment(pkts_burst[i], gso_ctx,
> >  					&gso_segments[nb_segments],
> >  					GSO_MAX_PKT_BURST - nb_segments);
> > +			/* pkts_burst[i] can be freed safely here. */
> > +			rte_pktmbuf_free(pkts_burst[i]);
> 
> It doesn't look correct to me.
> I think it should be:
> If (ret > 1) rte_pktmbuf_free(pkts_burst[i]);
> 
> >  			if (ret >= 0)
> >  				nb_segments += ret;
> >  			else {
> >  				TESTPMD_LOG(DEBUG, "Unable to segment packet");
> > -				rte_pktmbuf_free(pkts_burst[i]);
> >  			}
> >  		}
> 
> 
> About drivers/net/tap/rte_eth_tap.c:
> I think it has to be modified too, as here:
> 
> /* free original mbuf */
>                 rte_pktmbuf_free(mbuf_in);
>                 /* free tso mbufs */
>                 if (num_tso_mbufs > 0)
>                         rte_pktmbuf_free_bulk(mbuf, num_tso_mbufs);
> 
> if mbuf[0] == mbuf_in
> Will have a double free() for the same mbuf.
> 
> >
> > 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()``.
> 
> Probably worth to mention that if return code == 1, then
> output mbuf will point to input mbuf and extra care with free() is required.

Another possibility - change gso_segment() behaviour even further,
and don't put input_pkt into pkt_out[] when no segmentation happened.
Might be even a bit cleaner and less error prone. 

> 
> > 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;
> >  	}
> > diff --git a/lib/librte_gso/rte_gso.h b/lib/librte_gso/rte_gso.h
> > index 3aab297..f6694f9 100644
> > --- a/lib/librte_gso/rte_gso.h
> > +++ b/lib/librte_gso/rte_gso.h
> > @@ -89,8 +89,11 @@ struct rte_gso_ctx {
> >   * the GSO segments are sent to should support transmission of multi-segment
> >   * packets.
> >   *
> > - * If the input packet is GSO'd, its mbuf refcnt reduces by 1. Therefore,
> > - * when all GSO segments are freed, the input packet is freed automatically.
> > + * If the input packet is GSO'd, all the indirect segments are attached to the
> > + * input packet.
> > + *
> > + * rte_gso_segment() will not free the input packet no matter whether it is
> > + * GSO'd or not, the application should free it after call rte_gso_segment().
> >   *
> >   * If the memory space in pkts_out or MBUF pools is insufficient, this
> >   * function fails, and it returns (-1) * errno. Otherwise, GSO succeeds,
> > --
> > 1.8.3.1
Hu, Jiayu Oct. 23, 2020, 12:57 a.m. UTC | #3
> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Thursday, October 22, 2020 11:34 PM
> To: yang_y_yi@163.com; dev@dpdk.org
> Cc: Hu, Jiayu <jiayu.hu@intel.com>; techboard@dpdk.org;
> thomas@monjalon.net; yangyi01@inspur.com
> Subject: RE: [PATCH v2] 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.
> >
> > Probably needs to be stated clearly:
> > It is a change in functional behaviour.
> > Without deprecation note in advance.
> > TB members: please provide your opinion on that patch.
> >
> > >
> > > Fixes: 119583797b6a ("gso: support TCP/IPv4 GSO")
> > > Signed-off-by: Yi Yang <yangyi01@inspur.com>
> > > ---
> > > Changelog:
> > >
> > > v1->v2:
> > >   - update description of rte_gso_segment().
> > >   - change code which calls rte_gso_segment() to
> > >     fix free issue.
> > >
> > > ---
> > >  app/test-pmd/csumonly.c                                    | 3 ++-
> > >  doc/guides/prog_guide/generic_segmentation_offload_lib.rst | 7 +++++-
> -
> >
> > I think release notes also have to be updated.
> >
> > >  lib/librte_gso/rte_gso.c                                   | 9 +--------
> > >  lib/librte_gso/rte_gso.h                                   | 7 +++++--
> > >  4 files changed, 13 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
> > > index 3d7d244..829e07f 100644
> > > --- a/app/test-pmd/csumonly.c
> > > +++ b/app/test-pmd/csumonly.c
> > > @@ -1080,11 +1080,12 @@ struct simple_gre_hdr {
> > >  ret = rte_gso_segment(pkts_burst[i], gso_ctx,
> > >  &gso_segments[nb_segments],
> > >  GSO_MAX_PKT_BURST - nb_segments);
> > > +/* pkts_burst[i] can be freed safely here. */
> > > +rte_pktmbuf_free(pkts_burst[i]);
> >
> > It doesn't look correct to me.
> > I think it should be:
> > If (ret > 1) rte_pktmbuf_free(pkts_burst[i]);
> >
> > >  if (ret >= 0)
> > >  nb_segments += ret;
> > >  else {
> > >  TESTPMD_LOG(DEBUG, "Unable to segment packet");
> > > -rte_pktmbuf_free(pkts_burst[i]);
> > >  }
> > >  }
> >
> >
> > About drivers/net/tap/rte_eth_tap.c:
> > I think it has to be modified too, as here:
> >
> > /* free original mbuf */
> >                 rte_pktmbuf_free(mbuf_in);
> >                 /* free tso mbufs */
> >                 if (num_tso_mbufs > 0)
> >                         rte_pktmbuf_free_bulk(mbuf, num_tso_mbufs);
> >
> > if mbuf[0] == mbuf_in
> > Will have a double free() for the same mbuf.
> >
> > >
> > > 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()``.
> >
> > Probably worth to mention that if return code == 1, then
> > output mbuf will point to input mbuf and extra care with free() is required.
> 
> Another possibility - change gso_segment() behaviour even further,
> and don't put input_pkt into pkt_out[] when no segmentation happened.
> Might be even a bit cleaner and less error prone.

Agree. Pkts_out[] better not to contain input_pkt when GSO doesn't happen.

BTW, @Yi, you also need to update the comment of describing what rte_gso_segment()
returns in rte_gso.h.

Thanks,
Jiayu
> 
> >
> > > 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;
> > >  }
> > > diff --git a/lib/librte_gso/rte_gso.h b/lib/librte_gso/rte_gso.h
> > > index 3aab297..f6694f9 100644
> > > --- a/lib/librte_gso/rte_gso.h
> > > +++ b/lib/librte_gso/rte_gso.h
> > > @@ -89,8 +89,11 @@ struct rte_gso_ctx {
> > >   * the GSO segments are sent to should support transmission of multi-
> segment
> > >   * packets.
> > >   *
> > > - * If the input packet is GSO'd, its mbuf refcnt reduces by 1. Therefore,
> > > - * when all GSO segments are freed, the input packet is freed
> automatically.
> > > + * If the input packet is GSO'd, all the indirect segments are attached to
> the
> > > + * input packet.
> > > + *
> > > + * rte_gso_segment() will not free the input packet no matter whether it
> is
> > > + * GSO'd or not, the application should free it after call
> rte_gso_segment().
> > >   *
> > >   * If the memory space in pkts_out or MBUF pools is insufficient, this
> > >   * function fails, and it returns (-1) * errno. Otherwise, GSO succeeds,
> > > --
> > > 1.8.3.1
>
yang_y_yi Oct. 23, 2020, 1:18 p.m. UTC | #4
Konstantin, thank you so much for comments, my replies inline, please check them.
At 2020-10-22 21:16:43, "Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:
>
>> 
>> 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.
>
>Probably needs to be stated clearly:
>It is a change in functional behaviour.
>Without deprecation note in advance.

Ok, I'll add such statement in next version.

>TB members: please provide your opinion on that patch.
>
>> 
>> Fixes: 119583797b6a ("gso: support TCP/IPv4 GSO")
>> Signed-off-by: Yi Yang <yangyi01@inspur.com>
>> ---
>> Changelog:
>> 
>> v1->v2:
>>   - update description of rte_gso_segment().
>>   - change code which calls rte_gso_segment() to
>>     fix free issue.
>> 
>> ---
>>  app/test-pmd/csumonly.c                                    | 3 ++-
>>  doc/guides/prog_guide/generic_segmentation_offload_lib.rst | 7 +++++--
>
>I think release notes also have to be updated.

Ok, also will update it to reflect this change.

>
>>  lib/librte_gso/rte_gso.c                                   | 9 +--------
>>  lib/librte_gso/rte_gso.h                                   | 7 +++++--
>>  4 files changed, 13 insertions(+), 13 deletions(-)
>> 
>> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
>> index 3d7d244..829e07f 100644
>> --- a/app/test-pmd/csumonly.c
>> +++ b/app/test-pmd/csumonly.c
>> @@ -1080,11 +1080,12 @@ struct simple_gre_hdr {
>>  			ret = rte_gso_segment(pkts_burst[i], gso_ctx,
>>  					&gso_segments[nb_segments],
>>  					GSO_MAX_PKT_BURST - nb_segments);
>> +			/* pkts_burst[i] can be freed safely here. */
>> +			rte_pktmbuf_free(pkts_burst[i]);
>
>It doesn't look correct to me.
>I think it should be:
>If (ret > 1) rte_pktmbuf_free(pkts_burst[i]);

No, in original implementation, if gso failed, application will free it, otherwise rte_gso_segment will free it (i.e. refcnt update -1 in rte_gso_segment), this change will change previous behavior. application will free it for both cases.

>
>>  			if (ret >= 0)
>>  				nb_segments += ret;
>>  			else {
>>  				TESTPMD_LOG(DEBUG, "Unable to segment packet");
>> -				rte_pktmbuf_free(pkts_burst[i]);
>>  			}
>>  		}
>
>
>About drivers/net/tap/rte_eth_tap.c:
>I think it has to be modified too, as here:
>
>/* free original mbuf */
>                rte_pktmbuf_free(mbuf_in);
>                /* free tso mbufs */
>                if (num_tso_mbufs > 0)
>                        rte_pktmbuf_free_bulk(mbuf, num_tso_mbufs);
>
>if mbuf[0] == mbuf_in
>Will have a double free() for the same mbuf.

Here I'm not clear, is this code ok before I change? It looks like there is same issue there before. 

>
>> 
>> 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()``.
>
>Probably worth to mention that if return code == 1, then
>output mbuf will point to input mbuf and extra care with free() is required.

Ok. 

>
>> 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;
>>  	}
>> diff --git a/lib/librte_gso/rte_gso.h b/lib/librte_gso/rte_gso.h
>> index 3aab297..f6694f9 100644
>> --- a/lib/librte_gso/rte_gso.h
>> +++ b/lib/librte_gso/rte_gso.h
>> @@ -89,8 +89,11 @@ struct rte_gso_ctx {
>>   * the GSO segments are sent to should support transmission of multi-segment
>>   * packets.
>>   *
>> - * If the input packet is GSO'd, its mbuf refcnt reduces by 1. Therefore,
>> - * when all GSO segments are freed, the input packet is freed automatically.
>> + * If the input packet is GSO'd, all the indirect segments are attached to the
>> + * input packet.
>> + *
>> + * rte_gso_segment() will not free the input packet no matter whether it is
>> + * GSO'd or not, the application should free it after call rte_gso_segment().
>>   *
>>   * If the memory space in pkts_out or MBUF pools is insufficient, this
>>   * function fails, and it returns (-1) * errno. Otherwise, GSO succeeds,
>> --
>> 1.8.3.1
yang_y_yi Oct. 23, 2020, 1:21 p.m. UTC | #5
At 2020-10-22 23:33:42, "Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:
>
>> >
>> > 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.
>> 
>> Probably needs to be stated clearly:
>> It is a change in functional behaviour.
>> Without deprecation note in advance.
>> TB members: please provide your opinion on that patch.
>> 
>> >
>> > Fixes: 119583797b6a ("gso: support TCP/IPv4 GSO")
>> > Signed-off-by: Yi Yang <yangyi01@inspur.com>
>> > ---
>> > Changelog:
>> >
>> > v1->v2:
>> >   - update description of rte_gso_segment().
>> >   - change code which calls rte_gso_segment() to
>> >     fix free issue.
>> >
>> > ---
>> >  app/test-pmd/csumonly.c                                    | 3 ++-
>> >  doc/guides/prog_guide/generic_segmentation_offload_lib.rst | 7 +++++--
>> 
>> I think release notes also have to be updated.
>> 
>> >  lib/librte_gso/rte_gso.c                                   | 9 +--------
>> >  lib/librte_gso/rte_gso.h                                   | 7 +++++--
>> >  4 files changed, 13 insertions(+), 13 deletions(-)
>> >
>> > diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
>> > index 3d7d244..829e07f 100644
>> > --- a/app/test-pmd/csumonly.c
>> > +++ b/app/test-pmd/csumonly.c
>> > @@ -1080,11 +1080,12 @@ struct simple_gre_hdr {
>> >  			ret = rte_gso_segment(pkts_burst[i], gso_ctx,
>> >  					&gso_segments[nb_segments],
>> >  					GSO_MAX_PKT_BURST - nb_segments);
>> > +			/* pkts_burst[i] can be freed safely here. */
>> > +			rte_pktmbuf_free(pkts_burst[i]);
>> 
>> It doesn't look correct to me.
>> I think it should be:
>> If (ret > 1) rte_pktmbuf_free(pkts_burst[i]);
>> 
>> >  			if (ret >= 0)
>> >  				nb_segments += ret;
>> >  			else {
>> >  				TESTPMD_LOG(DEBUG, "Unable to segment packet");
>> > -				rte_pktmbuf_free(pkts_burst[i]);
>> >  			}
>> >  		}
>> 
>> 
>> About drivers/net/tap/rte_eth_tap.c:
>> I think it has to be modified too, as here:
>> 
>> /* free original mbuf */
>>                 rte_pktmbuf_free(mbuf_in);
>>                 /* free tso mbufs */
>>                 if (num_tso_mbufs > 0)
>>                         rte_pktmbuf_free_bulk(mbuf, num_tso_mbufs);
>> 
>> if mbuf[0] == mbuf_in
>> Will have a double free() for the same mbuf.
>> 
>> >
>> > 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()``.
>> 
>> Probably worth to mention that if return code == 1, then
>> output mbuf will point to input mbuf and extra care with free() is required.
>
>Another possibility - change gso_segment() behaviour even further,
>and don't put input_pkt into pkt_out[] when no segmentation happened.
>Might be even a bit cleaner and less error prone.

Agree, I didn't notice it before.
 
>
>> 
>> > 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;
>> >  	}
>> > diff --git a/lib/librte_gso/rte_gso.h b/lib/librte_gso/rte_gso.h
>> > index 3aab297..f6694f9 100644
>> > --- a/lib/librte_gso/rte_gso.h
>> > +++ b/lib/librte_gso/rte_gso.h
>> > @@ -89,8 +89,11 @@ struct rte_gso_ctx {
>> >   * the GSO segments are sent to should support transmission of multi-segment
>> >   * packets.
>> >   *
>> > - * If the input packet is GSO'd, its mbuf refcnt reduces by 1. Therefore,
>> > - * when all GSO segments are freed, the input packet is freed automatically.
>> > + * If the input packet is GSO'd, all the indirect segments are attached to the
>> > + * input packet.
>> > + *
>> > + * rte_gso_segment() will not free the input packet no matter whether it is
>> > + * GSO'd or not, the application should free it after call rte_gso_segment().
>> >   *
>> >   * If the memory space in pkts_out or MBUF pools is insufficient, this
>> >   * function fails, and it returns (-1) * errno. Otherwise, GSO succeeds,
>> > --
>> > 1.8.3.1
yang_y_yi Oct. 23, 2020, 1:23 p.m. UTC | #6
At 2020-10-23 08:57:15, "Hu, Jiayu" <jiayu.hu@intel.com> wrote:
>
>
>> -----Original Message-----
>> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
>> Sent: Thursday, October 22, 2020 11:34 PM
>> To: yang_y_yi@163.com; dev@dpdk.org
>> Cc: Hu, Jiayu <jiayu.hu@intel.com>; techboard@dpdk.org;
>> thomas@monjalon.net; yangyi01@inspur.com
>> Subject: RE: [PATCH v2] 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.
>> >
>> > Probably needs to be stated clearly:
>> > It is a change in functional behaviour.
>> > Without deprecation note in advance.
>> > TB members: please provide your opinion on that patch.
>> >
>> > >
>> > > Fixes: 119583797b6a ("gso: support TCP/IPv4 GSO")
>> > > Signed-off-by: Yi Yang <yangyi01@inspur.com>
>> > > ---
>> > > Changelog:
>> > >
>> > > v1->v2:
>> > >   - update description of rte_gso_segment().
>> > >   - change code which calls rte_gso_segment() to
>> > >     fix free issue.
>> > >
>> > > ---
>> > >  app/test-pmd/csumonly.c                                    | 3 ++-
>> > >  doc/guides/prog_guide/generic_segmentation_offload_lib.rst | 7 +++++-
>> -
>> >
>> > I think release notes also have to be updated.
>> >
>> > >  lib/librte_gso/rte_gso.c                                   | 9 +--------
>> > >  lib/librte_gso/rte_gso.h                                   | 7 +++++--
>> > >  4 files changed, 13 insertions(+), 13 deletions(-)
>> > >
>> > > diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
>> > > index 3d7d244..829e07f 100644
>> > > --- a/app/test-pmd/csumonly.c
>> > > +++ b/app/test-pmd/csumonly.c
>> > > @@ -1080,11 +1080,12 @@ struct simple_gre_hdr {
>> > >  ret = rte_gso_segment(pkts_burst[i], gso_ctx,
>> > >  &gso_segments[nb_segments],
>> > >  GSO_MAX_PKT_BURST - nb_segments);
>> > > +/* pkts_burst[i] can be freed safely here. */
>> > > +rte_pktmbuf_free(pkts_burst[i]);
>> >
>> > It doesn't look correct to me.
>> > I think it should be:
>> > If (ret > 1) rte_pktmbuf_free(pkts_burst[i]);
>> >
>> > >  if (ret >= 0)
>> > >  nb_segments += ret;
>> > >  else {
>> > >  TESTPMD_LOG(DEBUG, "Unable to segment packet");
>> > > -rte_pktmbuf_free(pkts_burst[i]);
>> > >  }
>> > >  }
>> >
>> >
>> > About drivers/net/tap/rte_eth_tap.c:
>> > I think it has to be modified too, as here:
>> >
>> > /* free original mbuf */
>> >                 rte_pktmbuf_free(mbuf_in);
>> >                 /* free tso mbufs */
>> >                 if (num_tso_mbufs > 0)
>> >                         rte_pktmbuf_free_bulk(mbuf, num_tso_mbufs);
>> >
>> > if mbuf[0] == mbuf_in
>> > Will have a double free() for the same mbuf.
>> >
>> > >
>> > > 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()``.
>> >
>> > Probably worth to mention that if return code == 1, then
>> > output mbuf will point to input mbuf and extra care with free() is required.
>> 
>> Another possibility - change gso_segment() behaviour even further,
>> and don't put input_pkt into pkt_out[] when no segmentation happened.
>> Might be even a bit cleaner and less error prone.
>
>Agree. Pkts_out[] better not to contain input_pkt when GSO doesn't happen.
>
>BTW, @Yi, you also need to update the comment of describing what rte_gso_segment()
>returns in rte_gso.h.
>
>Thanks,
>Jiayu

Thanks Jiayu, I'll add this in  next version.

>> 
>> >
>> > > 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;
>> > >  }
>> > > diff --git a/lib/librte_gso/rte_gso.h b/lib/librte_gso/rte_gso.h
>> > > index 3aab297..f6694f9 100644
>> > > --- a/lib/librte_gso/rte_gso.h
>> > > +++ b/lib/librte_gso/rte_gso.h
>> > > @@ -89,8 +89,11 @@ struct rte_gso_ctx {
>> > >   * the GSO segments are sent to should support transmission of multi-
>> segment
>> > >   * packets.
>> > >   *
>> > > - * If the input packet is GSO'd, its mbuf refcnt reduces by 1. Therefore,
>> > > - * when all GSO segments are freed, the input packet is freed
>> automatically.
>> > > + * If the input packet is GSO'd, all the indirect segments are attached to
>> the
>> > > + * input packet.
>> > > + *
>> > > + * rte_gso_segment() will not free the input packet no matter whether it
>> is
>> > > + * GSO'd or not, the application should free it after call
>> rte_gso_segment().
>> > >   *
>> > >   * If the memory space in pkts_out or MBUF pools is insufficient, this
>> > >   * function fails, and it returns (-1) * errno. Otherwise, GSO succeeds,
>> > > --
>> > > 1.8.3.1
>>
Ananyev, Konstantin Oct. 23, 2020, 2:46 p.m. UTC | #7
> From: yang_y_yi <yang_y_yi@163.com>
> Sent: Friday, October 23, 2020 2:18 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; techboard@dpdk.org; thomas@monjalon.net; yangyi01@inspur.com
> Subject: Re:RE: [PATCH v2] gso: fix free issue of mbuf gso segments attach to
> 
> Konstantin, thank you so much for comments, my replies inline, please check them.
> At 2020-10-22 21:16:43, "Ananyev, Konstantin" <mailto:konstantin.ananyev@intel.com> wrote:
> >
> >>
> >> 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.
> >
> >Probably needs to be stated clearly:
> >It is a change in functional behaviour.
> >Without deprecation note in advance.
> 
> Ok, I'll add such statement in next version.
> 
> >TB members: please provide your opinion on that patch.
> >
> >>
> >> Fixes: 119583797b6a ("gso: support TCP/IPv4 GSO")
> >> Signed-off-by: Yi Yang <mailto:yangyi01@inspur.com>
> >> ---
> >> Changelog:
> >>
> >> v1->v2:
> >>   - update description of rte_gso_segment().
> >>   - change code which calls rte_gso_segment() to
> >>     fix free issue.
> >>
> >> ---
> >>  app/test-pmd/csumonly.c                                    | 3 ++-
> >>  doc/guides/prog_guide/generic_segmentation_offload_lib.rst | 7 +++++--
> >
> >I think release notes also have to be updated.
> 
> Ok, also will update it to reflect this change.
> 
> >
> >>  lib/librte_gso/rte_gso.c                                   | 9 +--------
> >>  lib/librte_gso/rte_gso.h                                   | 7 +++++--
> >>  4 files changed, 13 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
> >> index 3d7d244..829e07f 100644
> >> --- a/app/test-pmd/csumonly.c
> >> +++ b/app/test-pmd/csumonly.c
> >> @@ -1080,11 +1080,12 @@ struct simple_gre_hdr {
> >>  			ret = rte_gso_segment(pkts_burst[i], gso_ctx,
> >>  					&gso_segments[nb_segments],
> >>  					GSO_MAX_PKT_BURST - nb_segments);
> >> +			/* pkts_burst[i] can be freed safely here. */
> >> +			rte_pktmbuf_free(pkts_burst[i]);
> >
> >It doesn't look correct to me.
> >I think it should be:
> >If (ret > 1) rte_pktmbuf_free(pkts_burst[i]);
> 
> No, in original implementation, if gso failed, application will free it, otherwise rte_gso_segment will free it (i.e. refcnt update -1 in
> rte_gso_segment), this change will change previous behavior. application will free it for both cases.


That's the point - with current implementation:
If ret == 1, then you shouldn't free input packet.
Because in that case:
input_pkt == output_pkt[0]

And if you'll free it, you can't use it after it.
In that particular case, you can't TX it.
 
> 
> >
> >>  			if (ret >= 0)
> >>  				nb_segments += ret;
> >>  			else {
> >>  				TESTPMD_LOG(DEBUG, "Unable to segment packet");
> >> -				rte_pktmbuf_free(pkts_burst[i]);
> >>  			}
> >>  		}
> >
> >
> >About drivers/net/tap/rte_eth_tap.c:
> >I think it has to be modified too, as here:
> >
> >/* free original mbuf */
> >                rte_pktmbuf_free(mbuf_in);
> >                /* free tso mbufs */
> >                if (num_tso_mbufs > 0)
> >                        rte_pktmbuf_free_bulk(mbuf, num_tso_mbufs);
> >
> >if mbuf[0] == mbuf_in
> >Will have a double free() for the same mbuf.
> 
> Here I'm not clear, is this code ok before I change? It looks like there is same issue there before.
> 
> >
> >>
> >> 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()``.
> >
> >Probably worth to mention that if return code == 1, then
> >output mbuf will point to input mbuf and extra care with free() is required.
> 
> Ok.
> 
> >
> >> 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;
> >>  	}
> >> diff --git a/lib/librte_gso/rte_gso.h b/lib/librte_gso/rte_gso.h
> >> index 3aab297..f6694f9 100644
> >> --- a/lib/librte_gso/rte_gso.h
> >> +++ b/lib/librte_gso/rte_gso.h
> >> @@ -89,8 +89,11 @@ struct rte_gso_ctx {
> >>   * the GSO segments are sent to should support transmission of multi-segment
> >>   * packets.
> >>   *
> >> - * If the input packet is GSO'd, its mbuf refcnt reduces by 1. Therefore,
> >> - * when all GSO segments are freed, the input packet is freed automatically.
> >> + * If the input packet is GSO'd, all the indirect segments are attached to the
> >> + * input packet.
> >> + *
> >> + * rte_gso_segment() will not free the input packet no matter whether it is
> >> + * GSO'd or not, the application should free it after call rte_gso_segment().
> >>   *
> >>   * If the memory space in pkts_out or MBUF pools is insufficient, this
> >>   * function fails, and it returns (-1) * errno. Otherwise, GSO succeeds,
> >> --
> >> 1.8.3.1
> 
> 
>
yang_y_yi Oct. 26, 2020, 12:57 a.m. UTC | #8
At 2020-10-23 22:46:42, "Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:
>> From: yang_y_yi <yang_y_yi@163.com>
>> Sent: Friday, October 23, 2020 2:18 PM
>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
>> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; techboard@dpdk.org; thomas@monjalon.net; yangyi01@inspur.com
>> Subject: Re:RE: [PATCH v2] gso: fix free issue of mbuf gso segments attach to
>> 
>> Konstantin, thank you so much for comments, my replies inline, please check them.
>> At 2020-10-22 21:16:43, "Ananyev, Konstantin" <mailto:konstantin.ananyev@intel.com> wrote:
>> >
>> >>
>> >> 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.
>> >
>> >Probably needs to be stated clearly:
>> >It is a change in functional behaviour.
>> >Without deprecation note in advance.
>> 
>> Ok, I'll add such statement in next version.
>> 
>> >TB members: please provide your opinion on that patch.
>> >
>> >>
>> >> Fixes: 119583797b6a ("gso: support TCP/IPv4 GSO")
>> >> Signed-off-by: Yi Yang <mailto:yangyi01@inspur.com>
>> >> ---
>> >> Changelog:
>> >>
>> >> v1->v2:
>> >>   - update description of rte_gso_segment().
>> >>   - change code which calls rte_gso_segment() to
>> >>     fix free issue.
>> >>
>> >> ---
>> >>  app/test-pmd/csumonly.c                                    | 3 ++-
>> >>  doc/guides/prog_guide/generic_segmentation_offload_lib.rst | 7 +++++--
>> >
>> >I think release notes also have to be updated.
>> 
>> Ok, also will update it to reflect this change.
>> 
>> >
>> >>  lib/librte_gso/rte_gso.c                                   | 9 +--------
>> >>  lib/librte_gso/rte_gso.h                                   | 7 +++++--
>> >>  4 files changed, 13 insertions(+), 13 deletions(-)
>> >>
>> >> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
>> >> index 3d7d244..829e07f 100644
>> >> --- a/app/test-pmd/csumonly.c
>> >> +++ b/app/test-pmd/csumonly.c
>> >> @@ -1080,11 +1080,12 @@ struct simple_gre_hdr {
>> >>  			ret = rte_gso_segment(pkts_burst[i], gso_ctx,
>> >>  					&gso_segments[nb_segments],
>> >>  					GSO_MAX_PKT_BURST - nb_segments);
>> >> +			/* pkts_burst[i] can be freed safely here. */
>> >> +			rte_pktmbuf_free(pkts_burst[i]);
>> >
>> >It doesn't look correct to me.
>> >I think it should be:
>> >If (ret > 1) rte_pktmbuf_free(pkts_burst[i]);
>> 
>> No, in original implementation, if gso failed, application will free it, otherwise rte_gso_segment will free it (i.e. refcnt update -1 in
>> rte_gso_segment), this change will change previous behavior. application will free it for both cases.
>
>
>That's the point - with current implementation:
>If ret == 1, then you shouldn't free input packet.
>Because in that case:
>input_pkt == output_pkt[0]
>
>And if you'll free it, you can't use it after it.
>In that particular case, you can't TX it.

I checked gso code again, there are two cases even if ret == 1, one case is it isn't segmented, the other is it is segmented but gso_do_segment returns 1, for case #1, we can handle it as you said, but for case #2, we can't handle it as you said because it has been segmented in fact. So I think we should return 0 foe case #1 and don't do assignment  "pkts_out[0] = pkt;", we should handle case #2 as before, right?

> 
>> 
>> >
>> >>  			if (ret >= 0)
>> >>  				nb_segments += ret;
>> >>  			else {
>> >>  				TESTPMD_LOG(DEBUG, "Unable to segment packet");
>> >> -				rte_pktmbuf_free(pkts_burst[i]);
>> >>  			}
>> >>  		}
>> >
>> >
>> >About drivers/net/tap/rte_eth_tap.c:
>> >I think it has to be modified too, as here:
>> >
>> >/* free original mbuf */
>> >                rte_pktmbuf_free(mbuf_in);
>> >                /* free tso mbufs */
>> >                if (num_tso_mbufs > 0)
>> >                        rte_pktmbuf_free_bulk(mbuf, num_tso_mbufs);
>> >
>> >if mbuf[0] == mbuf_in
>> >Will have a double free() for the same mbuf.
>> 
>> Here I'm not clear, is this code ok before I change? It looks like there is same issue there before.
>> 
>> >
>> >>
>> >> 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()``.
>> >
>> >Probably worth to mention that if return code == 1, then
>> >output mbuf will point to input mbuf and extra care with free() is required.
>> 
>> Ok.
>> 
>> >
>> >> 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;
>> >>  	}
>> >> diff --git a/lib/librte_gso/rte_gso.h b/lib/librte_gso/rte_gso.h
>> >> index 3aab297..f6694f9 100644
>> >> --- a/lib/librte_gso/rte_gso.h
>> >> +++ b/lib/librte_gso/rte_gso.h
>> >> @@ -89,8 +89,11 @@ struct rte_gso_ctx {
>> >>   * the GSO segments are sent to should support transmission of multi-segment
>> >>   * packets.
>> >>   *
>> >> - * If the input packet is GSO'd, its mbuf refcnt reduces by 1. Therefore,
>> >> - * when all GSO segments are freed, the input packet is freed automatically.
>> >> + * If the input packet is GSO'd, all the indirect segments are attached to the
>> >> + * input packet.
>> >> + *
>> >> + * rte_gso_segment() will not free the input packet no matter whether it is
>> >> + * GSO'd or not, the application should free it after call rte_gso_segment().
>> >>   *
>> >>   * If the memory space in pkts_out or MBUF pools is insufficient, this
>> >>   * function fails, and it returns (-1) * errno. Otherwise, GSO succeeds,
>> >> --
>> >> 1.8.3.1
>> 
>> 
>>
Hu, Jiayu Oct. 26, 2020, 2:06 a.m. UTC | #9
On Mon, Oct 26, 2020 at 08:57:30AM +0800, yang_y_yi wrote:
> At 2020-10-23 22:46:42, "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
> wrote:
> 
> >> From: yang_y_yi <yang_y_yi@163.com>
> >> Sent: Friday, October 23, 2020 2:18 PM
> >> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> >> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; techboard@dpdk.org; thomas@monjalon.net; yangyi01@inspur.com
> >> Subject: Re:RE: [PATCH v2] gso: fix free issue of mbuf gso segments attach to
> >>
> >> Konstantin, thank you so much for comments, my replies inline, please check them.
> >> At 2020-10-22 21:16:43, "Ananyev, Konstantin" <mailto:konstantin.ananyev@intel.com> wrote:
> >> >
> >> >>
> >> >> 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.
> >> >
> >> >Probably needs to be stated clearly:
> >> >It is a change in functional behaviour.
> >> >Without deprecation note in advance.
> >>
> >> Ok, I'll add such statement in next version.
> >>
> >> >TB members: please provide your opinion on that patch.
> >> >
> >> >>
> >> >> Fixes: 119583797b6a ("gso: support TCP/IPv4 GSO")
> >> >> Signed-off-by: Yi Yang <mailto:yangyi01@inspur.com>
> >> >> ---
> >> >> Changelog:
> >> >>
> >> >> v1->v2:
> >> >>   - update description of rte_gso_segment().
> >> >>   - change code which calls rte_gso_segment() to
> >> >>     fix free issue.
> >> >>
> >> >> ---
> >> >>  app/test-pmd/csumonly.c                                    | 3 ++-
> >> >>  doc/guides/prog_guide/generic_segmentation_offload_lib.rst | 7 +++++--
> >> >
> >> >I think release notes also have to be updated.
> >>
> >> Ok, also will update it to reflect this change.
> >>
> >> >
> >> >>  lib/librte_gso/rte_gso.c                                   | 9 +--------
> >> >>  lib/librte_gso/rte_gso.h                                   | 7 +++++--
> >> >>  4 files changed, 13 insertions(+), 13 deletions(-)
> >> >>
> >> >> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
> >> >> index 3d7d244..829e07f 100644
> >> >> --- a/app/test-pmd/csumonly.c
> >> >> +++ b/app/test-pmd/csumonly.c
> >> >> @@ -1080,11 +1080,12 @@ struct simple_gre_hdr {
> >> >>                   ret = rte_gso_segment(pkts_burst[i], gso_ctx,
> >> >>                                   &gso_segments[nb_segments],
> >> >>                                   GSO_MAX_PKT_BURST - nb_segments);
> >> >> +                 /* pkts_burst[i] can be freed safely here. */
> >> >> +                 rte_pktmbuf_free(pkts_burst[i]);
> >> >
> >> >It doesn't look correct to me.
> >> >I think it should be:
> >> >If (ret > 1) rte_pktmbuf_free(pkts_burst[i]);
> >>
> >> No, in original implementation, if gso failed, application will free it, otherwise rte_gso_segment will free it (i.e. refcnt update -1 in
> >> rte_gso_segment), this change will change previous behavior. application will free it for both cases.
> >
> >
> >That's the point - with current implementation:
> >If ret == 1, then you shouldn't free input packet.
> >Because in that case:
> >input_pkt == output_pkt[0]
> >
> >And if you'll free it, you can't use it after it.
> >In that particular case, you can't TX it.
> 
> I checked gso code again, there are two cases even if ret == 1, one case is it isn't segmented, the other is it is segmented but gso_do_segment returns 1, for case #1, we can handle it as you said, but for case #2, we can't handle it as you said because it has been segmented in fact. So I think we should return 0 foe case #1 and don't do assignment  "pkts_out[0] = pkt;", we should handle case #2 as before, right?
> 

When will #2 case happen? In current implementation,
ret of gso_do_segment() is > 1, when GSO happens; otherwise,
ret is negative. It doesn't return 1. If you mean the
case that pkt_len is smaller than gso_size, gso_do_segment()
will not be called, since rte_gso_segment() will compare
pkt_len and gso_size before.

Thanks,
Jiayu
> >
> >>
> 
> 
> 
>  
>
yang_y_yi Oct. 26, 2020, 2:12 a.m. UTC | #10
At 2020-10-26 10:06:09, "Jiayu Hu" <jiayu.hu@intel.com> wrote:
>On Mon, Oct 26, 2020 at 08:57:30AM +0800, yang_y_yi wrote:
>> At 2020-10-23 22:46:42, "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
>> wrote:
>> 
>> >> From: yang_y_yi <yang_y_yi@163.com>
>> >> Sent: Friday, October 23, 2020 2:18 PM
>> >> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
>> >> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; techboard@dpdk.org; thomas@monjalon.net; yangyi01@inspur.com
>> >> Subject: Re:RE: [PATCH v2] gso: fix free issue of mbuf gso segments attach to
>> >>
>> >> Konstantin, thank you so much for comments, my replies inline, please check them.
>> >> At 2020-10-22 21:16:43, "Ananyev, Konstantin" <mailto:konstantin.ananyev@intel.com> wrote:
>> >> >
>> >> >>
>> >> >> 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.
>> >> >
>> >> >Probably needs to be stated clearly:
>> >> >It is a change in functional behaviour.
>> >> >Without deprecation note in advance.
>> >>
>> >> Ok, I'll add such statement in next version.
>> >>
>> >> >TB members: please provide your opinion on that patch.
>> >> >
>> >> >>
>> >> >> Fixes: 119583797b6a ("gso: support TCP/IPv4 GSO")
>> >> >> Signed-off-by: Yi Yang <mailto:yangyi01@inspur.com>
>> >> >> ---
>> >> >> Changelog:
>> >> >>
>> >> >> v1->v2:
>> >> >>   - update description of rte_gso_segment().
>> >> >>   - change code which calls rte_gso_segment() to
>> >> >>     fix free issue.
>> >> >>
>> >> >> ---
>> >> >>  app/test-pmd/csumonly.c                                    | 3 ++-
>> >> >>  doc/guides/prog_guide/generic_segmentation_offload_lib.rst | 7 +++++--
>> >> >
>> >> >I think release notes also have to be updated.
>> >>
>> >> Ok, also will update it to reflect this change.
>> >>
>> >> >
>> >> >>  lib/librte_gso/rte_gso.c                                   | 9 +--------
>> >> >>  lib/librte_gso/rte_gso.h                                   | 7 +++++--
>> >> >>  4 files changed, 13 insertions(+), 13 deletions(-)
>> >> >>
>> >> >> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
>> >> >> index 3d7d244..829e07f 100644
>> >> >> --- a/app/test-pmd/csumonly.c
>> >> >> +++ b/app/test-pmd/csumonly.c
>> >> >> @@ -1080,11 +1080,12 @@ struct simple_gre_hdr {
>> >> >>                   ret = rte_gso_segment(pkts_burst[i], gso_ctx,
>> >> >>                                   &gso_segments[nb_segments],
>> >> >>                                   GSO_MAX_PKT_BURST - nb_segments);
>> >> >> +                 /* pkts_burst[i] can be freed safely here. */
>> >> >> +                 rte_pktmbuf_free(pkts_burst[i]);
>> >> >
>> >> >It doesn't look correct to me.
>> >> >I think it should be:
>> >> >If (ret > 1) rte_pktmbuf_free(pkts_burst[i]);
>> >>
>> >> No, in original implementation, if gso failed, application will free it, otherwise rte_gso_segment will free it (i.e. refcnt update -1 in
>> >> rte_gso_segment), this change will change previous behavior. application will free it for both cases.
>> >
>> >
>> >That's the point - with current implementation:
>> >If ret == 1, then you shouldn't free input packet.
>> >Because in that case:
>> >input_pkt == output_pkt[0]
>> >
>> >And if you'll free it, you can't use it after it.
>> >In that particular case, you can't TX it.
>> 
>> I checked gso code again, there are two cases even if ret == 1, one case is it isn't segmented, the other is it is segmented but gso_do_segment returns 1, for case #1, we can handle it as you said, but for case #2, we can't handle it as you said because it has been segmented in fact. So I think we should return 0 foe case #1 and don't do assignment  "pkts_out[0] = pkt;", we should handle case #2 as before, right?
>> 
>
>When will #2 case happen? In current implementation,
>ret of gso_do_segment() is > 1, when GSO happens; otherwise,
>ret is negative. It doesn't return 1. If you mean the
>case that pkt_len is smaller than gso_size, gso_do_segment()
>will not be called, since rte_gso_segment() will compare
>pkt_len and gso_size before.
>
>Thanks,
>Jiayu

Got it, thanks Jiayu, I'll send out v3 to fix all the comments. Is "return 0 for the case ret == 1" ok? Before, this is still transmitted as a normal packet, but it is dropped/freed if ret <0.

>> >
>> >>
>> 
>> 
>> 
>>  
>>
Hu, Jiayu Oct. 26, 2020, 6:16 a.m. UTC | #11
On Mon, Oct 26, 2020 at 10:12:24AM +0800, yang_y_yi wrote:
> At 2020-10-26 10:06:09, "Jiayu Hu" <jiayu.hu@intel.com> wrote:
> 
> >On Mon, Oct 26, 2020 at 08:57:30AM +0800, yang_y_yi wrote:
> >> At 2020-10-23 22:46:42, "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
> >> wrote:
> >>
> >> >> From: yang_y_yi <yang_y_yi@163.com>
> >> >> Sent: Friday, October 23, 2020 2:18 PM
> >> >> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> >> >> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; techboard@dpdk.org; thomas@monjalon.net; yangyi01@inspur.com
> >> >> Subject: Re:RE: [PATCH v2] gso: fix free issue of mbuf gso segments attach to
> >> >>
> >> >> Konstantin, thank you so much for comments, my replies inline, please check them.
> >> >> At 2020-10-22 21:16:43, "Ananyev, Konstantin" <mailto:konstantin.ananyev@intel.com> wrote:
> >> >> >
> >> >> >>
> >> >> >> 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.
> >> >> >
> >> >> >Probably needs to be stated clearly:
> >> >> >It is a change in functional behaviour.
> >> >> >Without deprecation note in advance.
> >> >>
> >> >> Ok, I'll add such statement in next version.
> >> >>
> >> >> >TB members: please provide your opinion on that patch.
> >> >> >
> >> >> >>
> >> >> >> Fixes: 119583797b6a ("gso: support TCP/IPv4 GSO")
> >> >> >> Signed-off-by: Yi Yang <mailto:yangyi01@inspur.com>
> >> >> >> ---
> >> >> >> Changelog:
> >> >> >>
> >> >> >> v1->v2:
> >> >> >>   - update description of rte_gso_segment().
> >> >> >>   - change code which calls rte_gso_segment() to
> >> >> >>     fix free issue.
> >> >> >>
> >> >> >> ---
> >> >> >>  app/test-pmd/csumonly.c                                    | 3 ++-
> >> >> >>  doc/guides/prog_guide/generic_segmentation_offload_lib.rst | 7 +++++--
> >> >> >
> >> >> >I think release notes also have to be updated.
> >> >>
> >> >> Ok, also will update it to reflect this change.
> >> >>
> >> >> >
> >> >> >>  lib/librte_gso/rte_gso.c                                   | 9 +--------
> >> >> >>  lib/librte_gso/rte_gso.h                                   | 7 +++++--
> >> >> >>  4 files changed, 13 insertions(+), 13 deletions(-)
> >> >> >>
> >> >> >> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
> >> >> >> index 3d7d244..829e07f 100644
> >> >> >> --- a/app/test-pmd/csumonly.c
> >> >> >> +++ b/app/test-pmd/csumonly.c
> >> >> >> @@ -1080,11 +1080,12 @@ struct simple_gre_hdr {
> >> >> >>                   ret = rte_gso_segment(pkts_burst[i], gso_ctx,
> >> >> >>                                   &gso_segments[nb_segments],
> >> >> >>                                   GSO_MAX_PKT_BURST - nb_segments);
> >> >> >> +                 /* pkts_burst[i] can be freed safely here. */
> >> >> >> +                 rte_pktmbuf_free(pkts_burst[i]);
> >> >> >
> >> >> >It doesn't look correct to me.
> >> >> >I think it should be:
> >> >> >If (ret > 1) rte_pktmbuf_free(pkts_burst[i]);
> >> >>
> >> >> No, in original implementation, if gso failed, application will free it, otherwise rte_gso_segment will free it (i.e. refcnt update -1 in
> >> >> rte_gso_segment), this change will change previous behavior. application will free it for both cases.
> >> >
> >> >
> >> >That's the point - with current implementation:
> >> >If ret == 1, then you shouldn't free input packet.
> >> >Because in that case:
> >> >input_pkt == output_pkt[0]
> >> >
> >> >And if you'll free it, you can't use it after it.
> >> >In that particular case, you can't TX it.
> >>
> >> I checked gso code again, there are two cases even if ret == 1, one case is it isn't segmented, the other is it is segmented but gso_do_segment returns 1, for case #1, we can handle it as you said, but for case #2, we can't handle it as you said because it has been segmented in fact. So I think we should return 0 foe case #1 and don't do assignment  "pkts_out[0] = pkt;", we should handle case #2 as before, right?
> >>
> >
> >When will #2 case happen? In current implementation,
> >ret of gso_do_segment() is > 1, when GSO happens; otherwise,
> >ret is negative. It doesn't return 1. If you mean the
> >case that pkt_len is smaller than gso_size, gso_do_segment()
> >will not be called, since rte_gso_segment() will compare
> >pkt_len and gso_size before.
> >
> >Thanks,
> >Jiayu
> 
> Got it, thanks Jiayu, I'll send out v3 to fix all the comments. Is "return 0 for the case ret == 1" ok? Before, this is still transmitted as a normal packet, but it is dropped/freed if ret <0.

When no GSO happens, return 0 and pkts_out[] doesn't
store input pkt. This design is OK for me.

Thanks,
Jiayu
> 
> >> >
> >> >>
> >>
> >>
> >>
> >>
> >>
> 
> 
> 
> 
>  
>

Patch
diff mbox series

diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
index 3d7d244..829e07f 100644
--- a/app/test-pmd/csumonly.c
+++ b/app/test-pmd/csumonly.c
@@ -1080,11 +1080,12 @@  struct simple_gre_hdr {
 			ret = rte_gso_segment(pkts_burst[i], gso_ctx,
 					&gso_segments[nb_segments],
 					GSO_MAX_PKT_BURST - nb_segments);
+			/* pkts_burst[i] can be freed safely here. */
+			rte_pktmbuf_free(pkts_burst[i]);
 			if (ret >= 0)
 				nb_segments += ret;
 			else {
 				TESTPMD_LOG(DEBUG, "Unable to segment packet");
-				rte_pktmbuf_free(pkts_burst[i]);
 			}
 		}
 
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;
 	}
diff --git a/lib/librte_gso/rte_gso.h b/lib/librte_gso/rte_gso.h
index 3aab297..f6694f9 100644
--- a/lib/librte_gso/rte_gso.h
+++ b/lib/librte_gso/rte_gso.h
@@ -89,8 +89,11 @@  struct rte_gso_ctx {
  * the GSO segments are sent to should support transmission of multi-segment
  * packets.
  *
- * If the input packet is GSO'd, its mbuf refcnt reduces by 1. Therefore,
- * when all GSO segments are freed, the input packet is freed automatically.
+ * If the input packet is GSO'd, all the indirect segments are attached to the
+ * input packet.
+ *
+ * rte_gso_segment() will not free the input packet no matter whether it is
+ * GSO'd or not, the application should free it after call rte_gso_segment().
  *
  * If the memory space in pkts_out or MBUF pools is insufficient, this
  * function fails, and it returns (-1) * errno. Otherwise, GSO succeeds,