DPDK patches and discussions
 help / color / Atom feed
* [dpdk-dev] [PATCH] gso: fix free issue of mbuf gso segments attach to
@ 2020-10-10  3:10 yang_y_yi
  2020-10-13  7:28 ` Hu, Jiayu
  0 siblings, 1 reply; 16+ messages in thread
From: yang_y_yi @ 2020-10-10  3:10 UTC (permalink / raw)
  To: dev
  Cc: jiayu.hu, mark.b.kavanagh, konstantin.ananyev, olivier.matz,
	thomas, yangyi01, yang_y_yi

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>
---
 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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH] gso: fix free issue of mbuf gso segments attach to
  2020-10-10  3:10 [dpdk-dev] [PATCH] gso: fix free issue of mbuf gso segments attach to yang_y_yi
@ 2020-10-13  7:28 ` Hu, Jiayu
  2020-10-13 15:39   ` Ananyev, Konstantin
  0 siblings, 1 reply; 16+ messages in thread
From: Hu, Jiayu @ 2020-10-13  7:28 UTC (permalink / raw)
  To: yang_y_yi, dev, Ananyev, Konstantin
  Cc: mark.b.kavanagh, olivier.matz, thomas, yangyi01



> -----Original Message-----
> From: yang_y_yi@163.com <yang_y_yi@163.com>
> Sent: Saturday, October 10, 2020 11:10 AM
> To: dev@dpdk.org
> Cc: Hu, Jiayu <jiayu.hu@intel.com>; mark.b.kavanagh@intel.com; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>; olivier.matz@6wind.com;
> thomas@monjalon.net; yangyi01@inspur.com; yang_y_yi@163.com
> Subject: [PATCH] gso: fix free issue of mbuf gso segments attach to
> 
> 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.

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?

Thanks,
Jiayu
> 
> Fixes: 119583797b6a ("gso: support TCP/IPv4 GSO")
> Signed-off-by: Yi Yang <yangyi01@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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH] gso: fix free issue of mbuf gso segments attach to
  2020-10-13  7:28 ` Hu, Jiayu
@ 2020-10-13 15:39   ` Ananyev, Konstantin
  2020-10-14  1:00     ` Hu, Jiayu
  0 siblings, 1 reply; 16+ messages in thread
From: Ananyev, Konstantin @ 2020-10-13 15:39 UTC (permalink / raw)
  To: Hu, Jiayu, yang_y_yi, dev; +Cc: mark.b.kavanagh, olivier.matz, thomas, yangyi01


> > 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 ...
?



> 
> Thanks,
> Jiayu
> >
> > Fixes: 119583797b6a ("gso: support TCP/IPv4 GSO")
> > Signed-off-by: Yi Yang <yangyi01@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
> 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH] gso: fix free issue of mbuf gso segments attach to
  2020-10-13 15:39   ` Ananyev, Konstantin
@ 2020-10-14  1:00     ` Hu, Jiayu
  2020-10-14  2:56       ` yang_y_yi
  0 siblings, 1 reply; 16+ messages in thread
From: Hu, Jiayu @ 2020-10-14  1:00 UTC (permalink / raw)
  To: Ananyev, Konstantin, yang_y_yi, dev; +Cc: olivier.matz, thomas, yangyi01



> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Tuesday, October 13, 2020 11:39 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>; yang_y_yi@163.com; dev@dpdk.org
> Cc: mark.b.kavanagh@intel.com; olivier.matz@6wind.com;
> thomas@monjalon.net; yangyi01@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 <yangyi01@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
> >
> 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH] gso: fix free issue of mbuf gso segments attach to
  2020-10-14  1:00     ` Hu, Jiayu
@ 2020-10-14  2:56       ` yang_y_yi
  2020-10-14 12:05         ` Ananyev, Konstantin
  0 siblings, 1 reply; 16+ messages in thread
From: yang_y_yi @ 2020-10-14  2:56 UTC (permalink / raw)
  To: Hu, Jiayu; +Cc: Ananyev, Konstantin, dev, olivier.matz, thomas, yangyi01

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.





















At 2020-10-14 09:00:12, "Hu, Jiayu" <jiayu.hu@intel.com> wrote:
>
>
>> -----Original Message-----
>> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
>> Sent: Tuesday, October 13, 2020 11:39 PM
>> To: Hu, Jiayu <jiayu.hu@intel.com>; yang_y_yi@163.com; dev@dpdk.org
>> Cc: mark.b.kavanagh@intel.com; olivier.matz@6wind.com;
>> thomas@monjalon.net; yangyi01@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 <yangyi01@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
>> >
>> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH] gso: fix free issue of mbuf gso segments attach to
  2020-10-14  2:56       ` yang_y_yi
@ 2020-10-14 12:05         ` Ananyev, Konstantin
  2020-10-15  5:14           ` Hu, Jiayu
  0 siblings, 1 reply; 16+ messages in thread
From: Ananyev, Konstantin @ 2020-10-14 12:05 UTC (permalink / raw)
  To: yang_y_yi, Hu, Jiayu; +Cc: dev, olivier.matz, thomas, yangyi01


> From: yang_y_yi <yang_y_yi@163.com>
> Sent: Wednesday, October 14, 2020 3:56 AM
> To: Hu, Jiayu <jiayu.hu@intel.com>
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org; olivier.matz@6wind.com; thomas@monjalon.net;
> yangyi01@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.hu@intel.com> wrote:
> >
> >
> >> -----Original Message-----
> >> From: Ananyev, Konstantin <mailto:konstantin.ananyev@intel.com>
> >> Sent: Tuesday, October 13, 2020 11:39 PM
> >> To: Hu, Jiayu <mailto:jiayu.hu@intel.com>; mailto:yang_y_yi@163.com; mailto:dev@dpdk.org
> >> Cc: mailto:mark.b.kavanagh@intel.com; mailto:olivier.matz@6wind.com;
> >> mailto:thomas@monjalon.net; mailto:yangyi01@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:yangyi01@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
> >> >
> >>
> 
>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH] gso: fix free issue of mbuf gso segments attach to
  2020-10-14 12:05         ` Ananyev, Konstantin
@ 2020-10-15  5:14           ` Hu, Jiayu
  2020-10-15 16:16             ` Ananyev, Konstantin
  0 siblings, 1 reply; 16+ messages in thread
From: Hu, Jiayu @ 2020-10-15  5:14 UTC (permalink / raw)
  To: Ananyev, Konstantin, yang_y_yi; +Cc: dev, olivier.matz, thomas, yangyi01



> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Wednesday, October 14, 2020 8:06 PM
> To: yang_y_yi <yang_y_yi@163.com>; Hu, Jiayu <jiayu.hu@intel.com>
> Cc: dev@dpdk.org; olivier.matz@6wind.com; thomas@monjalon.net;
> yangyi01@inspur.com
> Subject: RE: Re:RE: [PATCH] gso: fix free issue of mbuf gso segments attach to
> 
> 
> > From: yang_y_yi <yang_y_yi@163.com>
> > Sent: Wednesday, October 14, 2020 3:56 AM
> > To: Hu, Jiayu <jiayu.hu@intel.com>
> > Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org;
> olivier.matz@6wind.com; thomas@monjalon.net;
> > yangyi01@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?

In what case will pkt_out[] contain the input pkt? Can you give an example?

Thanks,
Jiayu

> 
> P.S. Please don't reply on the top.
> 
> Konstantin
> 
> >
> >
> >
> >


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH] gso: fix free issue of mbuf gso segments attach to
  2020-10-15  5:14           ` Hu, Jiayu
@ 2020-10-15 16:16             ` Ananyev, Konstantin
  2020-10-16  0:53               ` Hu, Jiayu
  2020-10-19  2:20               ` yang_y_yi
  0 siblings, 2 replies; 16+ messages in thread
From: Ananyev, Konstantin @ 2020-10-15 16:16 UTC (permalink / raw)
  To: Hu, Jiayu, yang_y_yi; +Cc: dev, olivier.matz, thomas, yangyi01



> 
> > -----Original Message-----
> > From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> > Sent: Wednesday, October 14, 2020 8:06 PM
> > To: yang_y_yi <yang_y_yi@163.com>; Hu, Jiayu <jiayu.hu@intel.com>
> > Cc: dev@dpdk.org; olivier.matz@6wind.com; thomas@monjalon.net;
> > yangyi01@inspur.com
> > Subject: RE: Re:RE: [PATCH] gso: fix free issue of mbuf gso segments attach to
> >
> >
> > > From: yang_y_yi <yang_y_yi@163.com>
> > > Sent: Wednesday, October 14, 2020 3:56 AM
> > > To: Hu, Jiayu <jiayu.hu@intel.com>
> > > Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org;
> > olivier.matz@6wind.com; thomas@monjalon.net;
> > > yangyi01@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?
> 
> In what case will pkt_out[] contain the input pkt? Can you give an example?

As I can read the code, whenever gso code decides that
no segmentation is not really needed, or it is not capable
of doing it properly.
Let say:

gso_tcp4_segment(struct rte_mbuf *pkt,
                uint16_t gso_size,
                uint8_t ipid_delta,
                struct rte_mempool *direct_pool,
                struct rte_mempool *indirect_pool,
                struct rte_mbuf **pkts_out,
                uint16_t nb_pkts_out)
{
	...
	/* Don't process the fragmented packet */
        ipv4_hdr = (struct rte_ipv4_hdr *)(rte_pktmbuf_mtod(pkt, char *) +
                        pkt->l2_len);
        frag_off = rte_be_to_cpu_16(ipv4_hdr->fragment_offset);
        if (unlikely(IS_FRAGMENTED(frag_off))) {
                pkts_out[0] = pkt;
                return 1;
        }

        /* Don't process the packet without data */
        hdr_offset = pkt->l2_len + pkt->l3_len + pkt->l4_len;
        if (unlikely(hdr_offset >= pkt->pkt_len)) {
                pkts_out[0] = pkt;
                return 1;
        }

That's why in rte_gso_segment() we update refcnt only when ret > 1. 
	



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH] gso: fix free issue of mbuf gso segments attach to
  2020-10-15 16:16             ` Ananyev, Konstantin
@ 2020-10-16  0:53               ` Hu, Jiayu
  2020-10-16  8:31                 ` Ananyev, Konstantin
  2020-10-19  2:29                 ` yang_y_yi
  2020-10-19  2:20               ` yang_y_yi
  1 sibling, 2 replies; 16+ messages in thread
From: Hu, Jiayu @ 2020-10-16  0:53 UTC (permalink / raw)
  To: Ananyev, Konstantin, yang_y_yi; +Cc: dev, olivier.matz, thomas, yangyi01



> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Friday, October 16, 2020 12:16 AM
> To: Hu, Jiayu <jiayu.hu@intel.com>; yang_y_yi <yang_y_yi@163.com>
> Cc: dev@dpdk.org; olivier.matz@6wind.com; thomas@monjalon.net;
> yangyi01@inspur.com
> Subject: RE: Re:RE: [PATCH] gso: fix free issue of mbuf gso segments attach to
> 
> 
> 
> >
> > > -----Original Message-----
> > > From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> > > Sent: Wednesday, October 14, 2020 8:06 PM
> > > To: yang_y_yi <yang_y_yi@163.com>; Hu, Jiayu <jiayu.hu@intel.com>
> > > Cc: dev@dpdk.org; olivier.matz@6wind.com; thomas@monjalon.net;
> > > yangyi01@inspur.com
> > > Subject: RE: Re:RE: [PATCH] gso: fix free issue of mbuf gso segments
> attach to
> > >
> > >
> > > > From: yang_y_yi <yang_y_yi@163.com>
> > > > Sent: Wednesday, October 14, 2020 3:56 AM
> > > > To: Hu, Jiayu <jiayu.hu@intel.com>
> > > > Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> dev@dpdk.org;
> > > olivier.matz@6wind.com; thomas@monjalon.net;
> > > > yangyi01@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?
> >
> > In what case will pkt_out[] contain the input pkt? Can you give an example?
> 
> As I can read the code, whenever gso code decides that
> no segmentation is not really needed, or it is not capable
> of doing it properly.
> Let say:
> 
> gso_tcp4_segment(struct rte_mbuf *pkt,
>                 uint16_t gso_size,
>                 uint8_t ipid_delta,
>                 struct rte_mempool *direct_pool,
>                 struct rte_mempool *indirect_pool,
>                 struct rte_mbuf **pkts_out,
>                 uint16_t nb_pkts_out)
> {
> ...
> /* Don't process the fragmented packet */
>         ipv4_hdr = (struct rte_ipv4_hdr *)(rte_pktmbuf_mtod(pkt, char *) +
>                         pkt->l2_len);
>         frag_off = rte_be_to_cpu_16(ipv4_hdr->fragment_offset);
>         if (unlikely(IS_FRAGMENTED(frag_off))) {
>                 pkts_out[0] = pkt;
>                 return 1;
>         }
> 
>         /* Don't process the packet without data */
>         hdr_offset = pkt->l2_len + pkt->l3_len + pkt->l4_len;
>         if (unlikely(hdr_offset >= pkt->pkt_len)) {
>                 pkts_out[0] = pkt;
>                 return 1;
>         }
> 
> That's why in rte_gso_segment() we update refcnt only when ret > 1.

But in these cases, the value of ret is 1. So we can free input pkt only when
ret > 1. Like:

-       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 > 1)
+               rte_pktmbuf_free(pkt);
+       else if (ret < 0) {
                /* Revert the ol_flags in the event of failure. */
                pkt->ol_flags = ol_flags;
        }

Thanks,
Jiayu
> 
> 
> 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH] gso: fix free issue of mbuf gso segments attach to
  2020-10-16  0:53               ` Hu, Jiayu
@ 2020-10-16  8:31                 ` Ananyev, Konstantin
  2020-10-19  3:17                   ` Hu, Jiayu
  2020-10-19  2:29                 ` yang_y_yi
  1 sibling, 1 reply; 16+ messages in thread
From: Ananyev, Konstantin @ 2020-10-16  8:31 UTC (permalink / raw)
  To: Hu, Jiayu, yang_y_yi; +Cc: dev, olivier.matz, thomas, yangyi01



> 
> > > > >
> > > > > 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?
> > >
> > > In what case will pkt_out[] contain the input pkt? Can you give an example?
> >
> > As I can read the code, whenever gso code decides that
> > no segmentation is not really needed, or it is not capable
> > of doing it properly.
> > Let say:
> >
> > gso_tcp4_segment(struct rte_mbuf *pkt,
> >                 uint16_t gso_size,
> >                 uint8_t ipid_delta,
> >                 struct rte_mempool *direct_pool,
> >                 struct rte_mempool *indirect_pool,
> >                 struct rte_mbuf **pkts_out,
> >                 uint16_t nb_pkts_out)
> > {
> > ...
> > /* Don't process the fragmented packet */
> >         ipv4_hdr = (struct rte_ipv4_hdr *)(rte_pktmbuf_mtod(pkt, char *) +
> >                         pkt->l2_len);
> >         frag_off = rte_be_to_cpu_16(ipv4_hdr->fragment_offset);
> >         if (unlikely(IS_FRAGMENTED(frag_off))) {
> >                 pkts_out[0] = pkt;
> >                 return 1;
> >         }
> >
> >         /* Don't process the packet without data */
> >         hdr_offset = pkt->l2_len + pkt->l3_len + pkt->l4_len;
> >         if (unlikely(hdr_offset >= pkt->pkt_len)) {
> >                 pkts_out[0] = pkt;
> >                 return 1;
> >         }
> >
> > That's why in rte_gso_segment() we update refcnt only when ret > 1.
> 
> But in these cases, the value of ret is 1. So we can free input pkt only when
> ret > 1. Like:
> 
> -       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 > 1)
> +               rte_pktmbuf_free(pkt);
> +       else if (ret < 0) {
>                 /* Revert the ol_flags in the event of failure. */
>                 pkt->ol_flags = ol_flags;
>         }

Yes, definitely. I am not arguing about that.
My question was to the author of the original patch:
He suggests not to free input packet inside gso function and leave it to the user.
So, in his proposition, would it also become user responsibility to determine
when input packet can be freed (it is not present in pkt_out[]) or not?

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH] gso: fix free issue of mbuf gso segments attach to
  2020-10-15 16:16             ` Ananyev, Konstantin
  2020-10-16  0:53               ` Hu, Jiayu
@ 2020-10-19  2:20               ` yang_y_yi
  1 sibling, 0 replies; 16+ messages in thread
From: yang_y_yi @ 2020-10-19  2:20 UTC (permalink / raw)
  To: Ananyev, Konstantin; +Cc: Hu, Jiayu, dev, olivier.matz, thomas, yangyi01

At 2020-10-16 00:16:03, "Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:
>
>
>> 
>> > -----Original Message-----
>> > From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
>> > Sent: Wednesday, October 14, 2020 8:06 PM
>> > To: yang_y_yi <yang_y_yi@163.com>; Hu, Jiayu <jiayu.hu@intel.com>
>> > Cc: dev@dpdk.org; olivier.matz@6wind.com; thomas@monjalon.net;
>> > yangyi01@inspur.com
>> > Subject: RE: Re:RE: [PATCH] gso: fix free issue of mbuf gso segments attach to
>> >
>> >
>> > > From: yang_y_yi <yang_y_yi@163.com>
>> > > Sent: Wednesday, October 14, 2020 3:56 AM
>> > > To: Hu, Jiayu <jiayu.hu@intel.com>
>> > > Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org;
>> > olivier.matz@6wind.com; thomas@monjalon.net;
>> > > yangyi01@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?
>> 
>> In what case will pkt_out[] contain the input pkt? Can you give an example?
>
>As I can read the code, whenever gso code decides that
>no segmentation is not really needed, or it is not capable
>of doing it properly.
>Let say:
>
>gso_tcp4_segment(struct rte_mbuf *pkt,
>                uint16_t gso_size,
>                uint8_t ipid_delta,
>                struct rte_mempool *direct_pool,
>                struct rte_mempool *indirect_pool,
>                struct rte_mbuf **pkts_out,
>                uint16_t nb_pkts_out)
>{
>	...
>	/* Don't process the fragmented packet */
>        ipv4_hdr = (struct rte_ipv4_hdr *)(rte_pktmbuf_mtod(pkt, char *) +
>                        pkt->l2_len);
>        frag_off = rte_be_to_cpu_16(ipv4_hdr->fragment_offset);
>        if (unlikely(IS_FRAGMENTED(frag_off))) {
>                pkts_out[0] = pkt;
>                return 1;
>        }
>
>        /* Don't process the packet without data */
>        hdr_offset = pkt->l2_len + pkt->l3_len + pkt->l4_len;
>        if (unlikely(hdr_offset >= pkt->pkt_len)) {
>                pkts_out[0] = pkt;
>                return 1;
>        }
>
>That's why in rte_gso_segment() we update refcnt only when ret > 1. 
>	
>


This code block 

       if (ret > 1) {
               pkt_seg = pkt;
               while (pkt_seg) {
                       rte_mbuf_refcnt_update(pkt_seg, -1);
                       pkt_seg = pkt_seg->next;
               }
       }

should be equivalent to this

       if (ret > 1) {
               rte_pktmbuf_free(pkt);
       }

So not sure what the issue you're saying is.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH] gso: fix free issue of mbuf gso segments attach to
  2020-10-16  0:53               ` Hu, Jiayu
  2020-10-16  8:31                 ` Ananyev, Konstantin
@ 2020-10-19  2:29                 ` yang_y_yi
  1 sibling, 0 replies; 16+ messages in thread
From: yang_y_yi @ 2020-10-19  2:29 UTC (permalink / raw)
  To: Hu, Jiayu; +Cc: Ananyev, Konstantin, dev, olivier.matz, thomas, yangyi01

At 2020-10-16 08:53:00, "Hu, Jiayu" <jiayu.hu@intel.com> wrote:
>
>
>> -----Original Message-----
>> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
>> Sent: Friday, October 16, 2020 12:16 AM
>> To: Hu, Jiayu <jiayu.hu@intel.com>; yang_y_yi <yang_y_yi@163.com>
>> Cc: dev@dpdk.org; olivier.matz@6wind.com; thomas@monjalon.net;
>> yangyi01@inspur.com
>> Subject: RE: Re:RE: [PATCH] gso: fix free issue of mbuf gso segments attach to
>> 
>> 
>> 
>> >
>> > > -----Original Message-----
>> > > From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
>> > > Sent: Wednesday, October 14, 2020 8:06 PM
>> > > To: yang_y_yi <yang_y_yi@163.com>; Hu, Jiayu <jiayu.hu@intel.com>
>> > > Cc: dev@dpdk.org; olivier.matz@6wind.com; thomas@monjalon.net;
>> > > yangyi01@inspur.com
>> > > Subject: RE: Re:RE: [PATCH] gso: fix free issue of mbuf gso segments
>> attach to
>> > >
>> > >
>> > > > From: yang_y_yi <yang_y_yi@163.com>
>> > > > Sent: Wednesday, October 14, 2020 3:56 AM
>> > > > To: Hu, Jiayu <jiayu.hu@intel.com>
>> > > > Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>;
>> dev@dpdk.org;
>> > > olivier.matz@6wind.com; thomas@monjalon.net;
>> > > > yangyi01@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?
>> >
>> > In what case will pkt_out[] contain the input pkt? Can you give an example?
>> 
>> As I can read the code, whenever gso code decides that
>> no segmentation is not really needed, or it is not capable
>> of doing it properly.
>> Let say:
>> 
>> gso_tcp4_segment(struct rte_mbuf *pkt,
>>                 uint16_t gso_size,
>>                 uint8_t ipid_delta,
>>                 struct rte_mempool *direct_pool,
>>                 struct rte_mempool *indirect_pool,
>>                 struct rte_mbuf **pkts_out,
>>                 uint16_t nb_pkts_out)
>> {
>> ...
>> /* Don't process the fragmented packet */
>>         ipv4_hdr = (struct rte_ipv4_hdr *)(rte_pktmbuf_mtod(pkt, char *) +
>>                         pkt->l2_len);
>>         frag_off = rte_be_to_cpu_16(ipv4_hdr->fragment_offset);
>>         if (unlikely(IS_FRAGMENTED(frag_off))) {
>>                 pkts_out[0] = pkt;
>>                 return 1;
>>         }
>> 
>>         /* Don't process the packet without data */
>>         hdr_offset = pkt->l2_len + pkt->l3_len + pkt->l4_len;
>>         if (unlikely(hdr_offset >= pkt->pkt_len)) {
>>                 pkts_out[0] = pkt;
>>                 return 1;
>>         }
>> 
>> That's why in rte_gso_segment() we update refcnt only when ret > 1.
>
>But in these cases, the value of ret is 1. So we can free input pkt only when
>ret > 1. Like:
>
>-       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 > 1)
>+               rte_pktmbuf_free(pkt);
>+       else if (ret < 0) {
>                /* Revert the ol_flags in the event of failure. */
>                pkt->ol_flags = ol_flags;
>        }
>
>Thanks,
>Jiayu
>> 
>> 
>> 

Jiayu, please help commit the patch you pasted if you think it is ok. I need to update my GSO patch based on this fix, thanks a lot.




^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH] gso: fix free issue of mbuf gso segments attach to
  2020-10-16  8:31                 ` Ananyev, Konstantin
@ 2020-10-19  3:17                   ` Hu, Jiayu
  2020-10-19  6:44                     ` yang_y_yi
  0 siblings, 1 reply; 16+ messages in thread
From: Hu, Jiayu @ 2020-10-19  3:17 UTC (permalink / raw)
  To: Ananyev, Konstantin, yang_y_yi; +Cc: dev, olivier.matz, thomas, yangyi01



> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Friday, October 16, 2020 4:31 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>; yang_y_yi <yang_y_yi@163.com>
> Cc: dev@dpdk.org; olivier.matz@6wind.com; thomas@monjalon.net;
> yangyi01@inspur.com
> Subject: RE: 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?
> > > >
> > > > In what case will pkt_out[] contain the input pkt? Can you give an
> example?
> > >
> > > As I can read the code, whenever gso code decides that
> > > no segmentation is not really needed, or it is not capable
> > > of doing it properly.
> > > Let say:
> > >
> > > gso_tcp4_segment(struct rte_mbuf *pkt,
> > >                 uint16_t gso_size,
> > >                 uint8_t ipid_delta,
> > >                 struct rte_mempool *direct_pool,
> > >                 struct rte_mempool *indirect_pool,
> > >                 struct rte_mbuf **pkts_out,
> > >                 uint16_t nb_pkts_out)
> > > {
> > > ...
> > > /* Don't process the fragmented packet */
> > >         ipv4_hdr = (struct rte_ipv4_hdr *)(rte_pktmbuf_mtod(pkt, char *) +
> > >                         pkt->l2_len);
> > >         frag_off = rte_be_to_cpu_16(ipv4_hdr->fragment_offset);
> > >         if (unlikely(IS_FRAGMENTED(frag_off))) {
> > >                 pkts_out[0] = pkt;
> > >                 return 1;
> > >         }
> > >
> > >         /* Don't process the packet without data */
> > >         hdr_offset = pkt->l2_len + pkt->l3_len + pkt->l4_len;
> > >         if (unlikely(hdr_offset >= pkt->pkt_len)) {
> > >                 pkts_out[0] = pkt;
> > >                 return 1;
> > >         }
> > >
> > > That's why in rte_gso_segment() we update refcnt only when ret > 1.
> >
> > But in these cases, the value of ret is 1. So we can free input pkt only when
> > ret > 1. Like:
> >
> > -       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 > 1)
> > +               rte_pktmbuf_free(pkt);
> > +       else if (ret < 0) {
> >                 /* Revert the ol_flags in the event of failure. */
> >                 pkt->ol_flags = ol_flags;
> >         }
> 
> Yes, definitely. I am not arguing about that.
> My question was to the author of the original patch:
> He suggests not to free input packet inside gso function and leave it to the
> user.
> So, in his proposition, would it also become user responsibility to determine
> when input packet can be freed (it is not present in pkt_out[]) or not?

@Yi, I am OK with the both designs. If you think it's better to free the input pkt by
users, you can keep the original design. But one thing to notice is that you need
to update definition of rte_gso_segment() in rte_gso.h too.

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH] gso: fix free issue of mbuf gso segments attach to
  2020-10-19  3:17                   ` Hu, Jiayu
@ 2020-10-19  6:44                     ` yang_y_yi
  2020-10-19  8:47                       ` Ananyev, Konstantin
  0 siblings, 1 reply; 16+ messages in thread
From: yang_y_yi @ 2020-10-19  6:44 UTC (permalink / raw)
  To: Hu, Jiayu; +Cc: Ananyev, Konstantin, dev, olivier.matz, thomas, yangyi01

At 2020-10-19 11:17:48, "Hu, Jiayu" <jiayu.hu@intel.com> wrote:
>
>
>> -----Original Message-----
>> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
>> Sent: Friday, October 16, 2020 4:31 PM
>> To: Hu, Jiayu <jiayu.hu@intel.com>; yang_y_yi <yang_y_yi@163.com>
>> Cc: dev@dpdk.org; olivier.matz@6wind.com; thomas@monjalon.net;
>> yangyi01@inspur.com
>> Subject: RE: 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?
>> > > >
>> > > > In what case will pkt_out[] contain the input pkt? Can you give an
>> example?
>> > >
>> > > As I can read the code, whenever gso code decides that
>> > > no segmentation is not really needed, or it is not capable
>> > > of doing it properly.
>> > > Let say:
>> > >
>> > > gso_tcp4_segment(struct rte_mbuf *pkt,
>> > >                 uint16_t gso_size,
>> > >                 uint8_t ipid_delta,
>> > >                 struct rte_mempool *direct_pool,
>> > >                 struct rte_mempool *indirect_pool,
>> > >                 struct rte_mbuf **pkts_out,
>> > >                 uint16_t nb_pkts_out)
>> > > {
>> > > ...
>> > > /* Don't process the fragmented packet */
>> > >         ipv4_hdr = (struct rte_ipv4_hdr *)(rte_pktmbuf_mtod(pkt, char *) +
>> > >                         pkt->l2_len);
>> > >         frag_off = rte_be_to_cpu_16(ipv4_hdr->fragment_offset);
>> > >         if (unlikely(IS_FRAGMENTED(frag_off))) {
>> > >                 pkts_out[0] = pkt;
>> > >                 return 1;
>> > >         }
>> > >
>> > >         /* Don't process the packet without data */
>> > >         hdr_offset = pkt->l2_len + pkt->l3_len + pkt->l4_len;
>> > >         if (unlikely(hdr_offset >= pkt->pkt_len)) {
>> > >                 pkts_out[0] = pkt;
>> > >                 return 1;
>> > >         }
>> > >
>> > > That's why in rte_gso_segment() we update refcnt only when ret > 1.
>> >
>> > But in these cases, the value of ret is 1. So we can free input pkt only when
>> > ret > 1. Like:
>> >
>> > -       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 > 1)
>> > +               rte_pktmbuf_free(pkt);
>> > +       else if (ret < 0) {
>> >                 /* Revert the ol_flags in the event of failure. */
>> >                 pkt->ol_flags = ol_flags;
>> >         }
>> 
>> Yes, definitely. I am not arguing about that.
>> My question was to the author of the original patch:
>> He suggests not to free input packet inside gso function and leave it to the
>> user.
>> So, in his proposition, would it also become user responsibility to determine
>> when input packet can be freed (it is not present in pkt_out[]) or not?
>
>@Yi, I am OK with the both designs. If you think it's better to free the input pkt by
>users, you can keep the original design. But one thing to notice is that you need
>to update definition of rte_gso_segment() in rte_gso.h too.
>
>Thanks,
>Jiayu

Ok, I prefer to handle it by users, this is incremental patch for rte_gso_segment description. Is it ok to you?

diff --git a/lib/librte_gso/rte_gso.h b/lib/librte_gso/rte_gso.h
index 3aab297..3762eba 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,

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH] gso: fix free issue of mbuf gso segments attach to
  2020-10-19  6:44                     ` yang_y_yi
@ 2020-10-19  8:47                       ` Ananyev, Konstantin
  2020-10-20  1:16                         ` yang_y_yi
  0 siblings, 1 reply; 16+ messages in thread
From: Ananyev, Konstantin @ 2020-10-19  8:47 UTC (permalink / raw)
  To: yang_y_yi, Hu, Jiayu; +Cc: dev, olivier.matz, thomas, yangyi01

> -----Original Message-----

> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>

> Sent: Monday, October 19, 2020 9:44 AM

> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>

> Subject: FW: Re:RE: Re:RE: [PATCH] gso: fix free issue of mbuf gso segments attach to

>

>

>

> From: yang_y_yi <yang_y_yi@163.com<mailto:yang_y_yi@163.com>>

> Sent: Monday, October 19, 2020 7:45 AM

> To: Hu, Jiayu <jiayu.hu@intel.com<mailto:jiayu.hu@intel.com>>

> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com<mailto:konstantin.ananyev@intel.com>>; dev@dpdk.org<mailto:dev@dpdk.org>; olivier.matz@6wind.com<mailto:olivier.matz@6wind.com>; thomas@monjalon.net<mailto:thomas@monjalon.net>;

> yangyi01@inspur.com<mailto:yangyi01@inspur.com>

> Subject: Re:RE: Re:RE: [PATCH] gso: fix free issue of mbuf gso segments attach to

>

> At 2020-10-19 11:17:48, "Hu, Jiayu" <mailto:jiayu.hu@intel.com> wrote:

> >

> >

> >> -----Original Message-----

> >> From: Ananyev, Konstantin <mailto:konstantin.ananyev@intel.com>

> >> Sent: Friday, October 16, 2020 4:31 PM

> >> To: Hu, Jiayu <mailto:jiayu.hu@intel.com>; yang_y_yi <mailto:yang_y_yi@163.com>

> >> Cc: mailto:dev@dpdk.org; mailto:olivier.matz@6wind.com; mailto:thomas@monjalon.net;

> >> mailto:yangyi01@inspur.com

> >> Subject: RE: 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?

> >> > > >

> >> > > > In what case will pkt_out[] contain the input pkt? Can you give an

> >> example?

> >> > >

> >> > > As I can read the code, whenever gso code decides that

> >> > > no segmentation is not really needed, or it is not capable

> >> > > of doing it properly.

> >> > > Let say:

> >> > >

> >> > > gso_tcp4_segment(struct rte_mbuf *pkt,

> >> > >                 uint16_t gso_size,

> >> > >                 uint8_t ipid_delta,

> >> > >                 struct rte_mempool *direct_pool,

> >> > >                 struct rte_mempool *indirect_pool,

> >> > >                 struct rte_mbuf **pkts_out,

> >> > >                 uint16_t nb_pkts_out)

> >> > > {

> >> > > ...

> >> > > /* Don't process the fragmented packet */

> >> > >         ipv4_hdr = (struct rte_ipv4_hdr *)(rte_pktmbuf_mtod(pkt, char *) +

> >> > >                         pkt->l2_len);

> >> > >         frag_off = rte_be_to_cpu_16(ipv4_hdr->fragment_offset);

> >> > >         if (unlikely(IS_FRAGMENTED(frag_off))) {

> >> > >                 pkts_out[0] = pkt;

> >> > >                 return 1;

> >> > >         }

> >> > >

> >> > >         /* Don't process the packet without data */

> >> > >         hdr_offset = pkt->l2_len + pkt->l3_len + pkt->l4_len;

> >> > >         if (unlikely(hdr_offset >= pkt->pkt_len)) {

> >> > >                 pkts_out[0] = pkt;

> >> > >                 return 1;

> >> > >         }

> >> > >

> >> > > That's why in rte_gso_segment() we update refcnt only when ret > 1.

> >> >

> >> > But in these cases, the value of ret is 1. So we can free input pkt only when

> >> > ret > 1. Like:

> >> >

> >> > -       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 > 1)

> >> > +               rte_pktmbuf_free(pkt);

> >> > +       else if (ret < 0) {

> >> >                 /* Revert the ol_flags in the event of failure. */

> >> >                 pkt->ol_flags = ol_flags;

> >> >         }

> >>

> >> Yes, definitely. I am not arguing about that.

> >> My question was to the author of the original patch:

> >> He suggests not to free input packet inside gso function and leave it to the

> >> user.

> >> So, in his proposition, would it also become user responsibility to determine

> >> when input packet can be freed (it is not present in pkt_out[]) or not?

> >

> >@Yi, I am OK with the both designs. If you think it's better to free the input pkt by

> >users, you can keep the original design. But one thing to notice is that you need

> >to update definition of rte_gso_segment() in rte_gso.h too.

> >

> >Thanks,

> >Jiayu

>

> Ok, I prefer to handle it by users, this is incremental patch for rte_gso_segment description. Is it ok to you?

>

> diff --git a/lib/librte_gso/rte_gso.h b/lib/librte_gso/rte_gso.h

> index 3aab297..3762eba 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,



I think such change will require much more than that.

While formally API is not changed, due to behaviour change,

API users will need to change their code, right?

If so, I think such change needs to be treated as API breakage.

So I think if you'll choose to go ahead with your original approach,

you'll need to:

1. Change all places in dpdk.org that uses rte_gso_segment()

to make them work correctly with new behaviour.

2. Usually such change has to be declared at least one release in advance via deprecation notice.

In 20.11 we do allow some API changes without deprecation notice,

but all such changes have to be reviewed by dpdk tech-board.

So don't forget to CC your patch to TB for review,

and explain clearly changes required in API caller code.



Konstantin





^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH] gso: fix free issue of mbuf gso segments attach to
  2020-10-19  8:47                       ` Ananyev, Konstantin
@ 2020-10-20  1:16                         ` yang_y_yi
  0 siblings, 0 replies; 16+ messages in thread
From: yang_y_yi @ 2020-10-20  1:16 UTC (permalink / raw)
  To: Ananyev, Konstantin; +Cc: Hu, Jiayu, dev, olivier.matz, thomas, yangyi01

At 2020-10-19 16:47:55, "Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:

> -----Original Message-----

> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>

> Sent: Monday, October 19, 2020 9:44 AM

> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>

> Subject: FW: Re:RE: Re:RE: [PATCH] gso: fix free issue of mbuf gso segments attach to

>

>

>

> From: yang_y_yi <yang_y_yi@163.com>

> Sent: Monday, October 19, 2020 7:45 AM

> To: Hu, Jiayu <jiayu.hu@intel.com>

> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org; olivier.matz@6wind.com; thomas@monjalon.net;

> yangyi01@inspur.com

> Subject: Re:RE: Re:RE: [PATCH] gso: fix free issue of mbuf gso segments attach to

>

> At 2020-10-19 11:17:48, "Hu, Jiayu" <mailto:jiayu.hu@intel.com> wrote:

> >

> >

> >> -----Original Message-----

> >> From: Ananyev, Konstantin <mailto:konstantin.ananyev@intel.com>

> >> Sent: Friday, October 16, 2020 4:31 PM

> >> To: Hu, Jiayu <mailto:jiayu.hu@intel.com>; yang_y_yi <mailto:yang_y_yi@163.com>

> >> Cc: mailto:dev@dpdk.org; mailto:olivier.matz@6wind.com; mailto:thomas@monjalon.net;

> >> mailto:yangyi01@inspur.com

> >> Subject: RE: 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?

> >> > > >

> >> > > > In what case will pkt_out[] contain the input pkt? Can you give an

> >> example?

> >> > >

> >> > > As I can read the code, whenever gso code decides that

> >> > > no segmentation is not really needed, or it is not capable

> >> > > of doing it properly.

> >> > > Let say:

> >> > >

> >> > > gso_tcp4_segment(struct rte_mbuf *pkt,

> >> > >                 uint16_t gso_size,

> >> > >                 uint8_t ipid_delta,

> >> > >                 struct rte_mempool *direct_pool,

> >> > >                 struct rte_mempool *indirect_pool,

> >> > >                 struct rte_mbuf **pkts_out,

> >> > >                 uint16_t nb_pkts_out)

> >> > > {

> >> > > ...

> >> > > /* Don't process the fragmented packet */

> >> > >         ipv4_hdr = (struct rte_ipv4_hdr *)(rte_pktmbuf_mtod(pkt, char *) +

> >> > >                         pkt->l2_len);

> >> > >         frag_off = rte_be_to_cpu_16(ipv4_hdr->fragment_offset);

> >> > >         if (unlikely(IS_FRAGMENTED(frag_off))) {

> >> > >                 pkts_out[0] = pkt;

> >> > >                 return 1;

> >> > >         }

> >> > >

> >> > >         /* Don't process the packet without data */

> >> > >         hdr_offset = pkt->l2_len + pkt->l3_len + pkt->l4_len;

> >> > >         if (unlikely(hdr_offset >= pkt->pkt_len)) {

> >> > >                 pkts_out[0] = pkt;

> >> > >                 return 1;

> >> > >         }

> >> > >

> >> > > That's why in rte_gso_segment() we update refcnt only when ret > 1.

> >> >

> >> > But in these cases, the value of ret is 1. So we can free input pkt only when

> >> > ret > 1. Like:

> >> >

> >> > -       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 > 1)

> >> > +               rte_pktmbuf_free(pkt);

> >> > +       else if (ret < 0) {

> >> >                 /* Revert the ol_flags in the event of failure. */

> >> >                 pkt->ol_flags = ol_flags;

> >> >         }

> >>

> >> Yes, definitely. I am not arguing about that.

> >> My question was to the author of the original patch:

> >> He suggests not to free input packet inside gso function and leave it to the

> >> user.

> >> So, in his proposition, would it also become user responsibility to determine

> >> when input packet can be freed (it is not present in pkt_out[]) or not?

> >

> >@Yi, I am OK with the both designs. If you think it's better to free the input pkt by

> >users, you can keep the original design. But one thing to notice is that you need

> >to update definition of rte_gso_segment() in rte_gso.h too.

> >

> >Thanks,

> >Jiayu

>

> Ok, I prefer to handle it by users, this is incremental patch for rte_gso_segment description. Is it ok to you?

>

> diff --git a/lib/librte_gso/rte_gso.h b/lib/librte_gso/rte_gso.h

> index 3aab297..3762eba 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,

 

I think such change will require much more than that.

While formally API is not changed, due to behaviour change,

API users will need to change their code, right?

If so, I think such change needs to be treated as API breakage.

So I think if you'll choose to go ahead with your original approach,

you'll need to:

1. Change all places in dpdk.org that uses rte_gso_segment()

to make them work correctly with new behaviour.

2. Usually such change has to be declared at least one release in advance via deprecation notice.

In 20.11 we do allow some API changes without deprecation notice,

but all such changes have to be reviewed by dpdk tech-board.

So don¡¯t forget to CC your patch to TB for review,

and explain clearly changes required in API caller code.

 

Konstantin

 

 

Thanks Konstantin for pointing out this, fortunately there are hardly users of rte_gso_segment() in dpdk source tree. drivers/net/tap/rte_eth_tap.c has already had correct code there.

                        num_tso_mbufs = rte_gso_segment(mbuf_in,
                                gso_ctx, /* gso control block */
                                (struct rte_mbuf **)&gso_mbufs, /* out mbufs */
                                RTE_DIM(gso_mbufs)); /* max tso mbufs */

                        /* ret contains the number of new created mbufs */
                        if (num_tso_mbufs < 0)
                                break;
......
                /* free original mbuf */
                rte_pktmbuf_free(mbuf_in);

We only need to change app/test-pmd/csumonly.c.

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]);
                        }
                }


I'll send a new version and cc to TB if you have no other comments.




^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, back to index

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-10  3:10 [dpdk-dev] [PATCH] gso: fix free issue of mbuf gso segments attach to yang_y_yi
2020-10-13  7:28 ` Hu, Jiayu
2020-10-13 15:39   ` Ananyev, Konstantin
2020-10-14  1:00     ` Hu, Jiayu
2020-10-14  2:56       ` yang_y_yi
2020-10-14 12:05         ` Ananyev, Konstantin
2020-10-15  5:14           ` Hu, Jiayu
2020-10-15 16:16             ` Ananyev, Konstantin
2020-10-16  0:53               ` Hu, Jiayu
2020-10-16  8:31                 ` Ananyev, Konstantin
2020-10-19  3:17                   ` Hu, Jiayu
2020-10-19  6:44                     ` yang_y_yi
2020-10-19  8:47                       ` Ananyev, Konstantin
2020-10-20  1:16                         ` yang_y_yi
2020-10-19  2:29                 ` yang_y_yi
2020-10-19  2:20               ` yang_y_yi

DPDK patches and discussions

Archives are clonable:
	git clone --mirror http://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ http://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev


Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox