diff mbox

[dpdk-dev,2/2] mbuf: reset nb_segs of chained packet

Message ID AB67A84E-B83C-49C3-AA48-8ACA06E24B39@gmail.com (mailing list archive)
State Rejected, archived
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Ilya Matveychikov Nov. 16, 2017, 2:05 p.m. UTC 
  Fixes: 139debc42dc0 ("mbuf: move chaining from ip_frag library")
Cc: simon.kagstrom@netinsight.net

Signed-off-by: Ilya V. Matveychikov <matvejchikov@gmail.com>
---
 lib/librte_mbuf/rte_mbuf.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

--
2.15.0

Comments

Stephen Hemminger Nov. 16, 2017, 5:01 p.m. UTC | #1 
On Thu, 16 Nov 2017 18:05:35 +0400
Ilya Matveychikov <matvejchikov@gmail.com> wrote:

> Fixes: 139debc42dc0 ("mbuf: move chaining from ip_frag library")
> Cc: simon.kagstrom@netinsight.net
> 
> Signed-off-by: Ilya V. Matveychikov <matvejchikov@gmail.com>
> ---
>  lib/librte_mbuf/rte_mbuf.h | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index ce8a05ddf..2126dc94b 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -1828,9 +1828,12 @@ static inline int rte_pktmbuf_chain(struct rte_mbuf *head, struct rte_mbuf *tail
>  	head->nb_segs += tail->nb_segs;
>  	head->pkt_len += tail->pkt_len;
> 
> -	/* pkt_len is only set in the head */
> +	/* nb_segs and pkt_len are only set in the head */
> +	tail->nb_segs = 1;
>  	tail->pkt_len = tail->data_len;
> 
> +	__rte_mbuf_sanity_check(head, 1);
> +
>  	return 0;
>  }

My understanding is that nb_segs and pkt_len are only valid
in head. For other packets in the chain nb_segs and pkt_len
can be anything.
Ilya Matveychikov Nov. 16, 2017, 7:15 p.m. UTC | #2 
> On Nov 16, 2017, at 9:01 PM, Stephen Hemminger <stephen@networkplumber.org> wrote:
> 
> On Thu, 16 Nov 2017 18:05:35 +0400
> Ilya Matveychikov <matvejchikov@gmail.com> wrote:
> 
>> Fixes: 139debc42dc0 ("mbuf: move chaining from ip_frag library")
>> Cc: simon.kagstrom@netinsight.net
>> 
>> Signed-off-by: Ilya V. Matveychikov <matvejchikov@gmail.com>
>> ---
>> lib/librte_mbuf/rte_mbuf.h | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>> 
>> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
>> index ce8a05ddf..2126dc94b 100644
>> --- a/lib/librte_mbuf/rte_mbuf.h
>> +++ b/lib/librte_mbuf/rte_mbuf.h
>> @@ -1828,9 +1828,12 @@ static inline int rte_pktmbuf_chain(struct rte_mbuf *head, struct rte_mbuf *tail
>> 	head->nb_segs += tail->nb_segs;
>> 	head->pkt_len += tail->pkt_len;
>> 
>> -	/* pkt_len is only set in the head */
>> +	/* nb_segs and pkt_len are only set in the head */
>> +	tail->nb_segs = 1;
>> 	tail->pkt_len = tail->data_len;
>> 
>> +	__rte_mbuf_sanity_check(head, 1);
>> +
>> 	return 0;
>> }
> 
> My understanding is that nb_segs and pkt_len are only valid
> in head. For other packets in the chain nb_segs and pkt_len
> can be anything.

So why not to keep them in consistency with multi-seg logic?
I mean that pkt_len/nb_segs for the head always have meaning but for
the rest of chain pkt_len is the same as data_len and nb_segs := 1
Olivier Matz Dec. 1, 2017, 4:37 p.m. UTC | #3 
Hi Ilya,

On Thu, Nov 16, 2017 at 11:15:18PM +0400, Ilya Matveychikov wrote:
> 
> > On Nov 16, 2017, at 9:01 PM, Stephen Hemminger <stephen@networkplumber.org> wrote:
> > 
> > On Thu, 16 Nov 2017 18:05:35 +0400
> > Ilya Matveychikov <matvejchikov@gmail.com> wrote:
> > 
> >> Fixes: 139debc42dc0 ("mbuf: move chaining from ip_frag library")
> >> Cc: simon.kagstrom@netinsight.net
> >> 
> >> Signed-off-by: Ilya V. Matveychikov <matvejchikov@gmail.com>
> >> ---
> >> lib/librte_mbuf/rte_mbuf.h | 5 ++++-
> >> 1 file changed, 4 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> >> index ce8a05ddf..2126dc94b 100644
> >> --- a/lib/librte_mbuf/rte_mbuf.h
> >> +++ b/lib/librte_mbuf/rte_mbuf.h
> >> @@ -1828,9 +1828,12 @@ static inline int rte_pktmbuf_chain(struct rte_mbuf *head, struct rte_mbuf *tail
> >> 	head->nb_segs += tail->nb_segs;
> >> 	head->pkt_len += tail->pkt_len;
> >> 
> >> -	/* pkt_len is only set in the head */
> >> +	/* nb_segs and pkt_len are only set in the head */
> >> +	tail->nb_segs = 1;
> >> 	tail->pkt_len = tail->data_len;
> >> 
> >> +	__rte_mbuf_sanity_check(head, 1);
> >> +
> >> 	return 0;
> >> }
> > 
> > My understanding is that nb_segs and pkt_len are only valid
> > in head. For other packets in the chain nb_segs and pkt_len
> > can be anything.
> 
> So why not to keep them in consistency with multi-seg logic?
> I mean that pkt_len/nb_segs for the head always have meaning but for
> the rest of chain pkt_len is the same as data_len and nb_segs := 1
> 

Stephen is right: like most mbuf fields, nb_segs and pkt_len are only
valid for the first mbuf of the chain.

What would be the advantage of changing this?
In addition, I think it would require to do the same change that in many
places, like drivers that build multi-seg mbufs.

If you are fixing an issue, please describe it in the commit log.

Olivier
diff mbox

Patch

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index ce8a05ddf..2126dc94b 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1828,9 +1828,12 @@  static inline int rte_pktmbuf_chain(struct rte_mbuf *head, struct rte_mbuf *tail
 	head->nb_segs += tail->nb_segs;
 	head->pkt_len += tail->pkt_len;

-	/* pkt_len is only set in the head */
+	/* nb_segs and pkt_len are only set in the head */
+	tail->nb_segs = 1;
 	tail->pkt_len = tail->data_len;

+	__rte_mbuf_sanity_check(head, 1);
+
 	return 0;
 }