[PATCH] mbuf: remove redundant line in rte_pktmbuf_attach

[PATCH] mbuf: remove redundant line in rte_pktmbuf_attach

[Ilya Matveychikov]

mi->next will be assigned to NULL few lines later, trivial patch

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

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index ead7c6e..5589d54 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1139,7 +1139,6 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *m)
 	mi->buf_addr = m->buf_addr;
 	mi->buf_len = m->buf_len;
 
-	mi->next = m->next;
 	mi->data_off = m->data_off;
 	mi->data_len = m->data_len;
 	mi->port = m->port;
-- 
2.7.4

Re: [PATCH] mbuf: remove redundant line in rte_pktmbuf_attach

[Ferruh Yigit]

On 1/20/2017 12:19 AM, Ilya Matveychikov wrote:
> mi->next will be assigned to NULL few lines later, trivial patch
> 
> Signed-off-by: Ilya V. Matveychikov <matvejchikov@gmail.com>
> ---
>  lib/librte_mbuf/rte_mbuf.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index ead7c6e..5589d54 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -1139,7 +1139,6 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *m)
>  	mi->buf_addr = m->buf_addr;
>  	mi->buf_len = m->buf_len;
>  
> -	mi->next = m->next;

Do you know why attaching mbuf is not supporting multi-segment?
Perhaps this can be documented in function comment, as one of the "not
supported" items.

Also, should we check mi->next before overwriting, in case it is not NULL?

>  	mi->data_off = m->data_off;
>  	mi->data_len = m->data_len;
>  	mi->port = m->port;
> 

Re: [PATCH] mbuf: remove redundant line in rte_pktmbuf_attach

[Ilya Matveychikov]

> On Jan 20, 2017, at 4:08 PM, Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 
> On 1/20/2017 12:19 AM, Ilya Matveychikov wrote:
>> mi->next will be assigned to NULL few lines later, trivial patch
>> 
>> Signed-off-by: Ilya V. Matveychikov <matvejchikov@gmail.com>
>> ---
>> lib/librte_mbuf/rte_mbuf.h | 1 -
>> 1 file changed, 1 deletion(-)
>> 
>> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
>> index ead7c6e..5589d54 100644
>> --- a/lib/librte_mbuf/rte_mbuf.h
>> +++ b/lib/librte_mbuf/rte_mbuf.h
>> @@ -1139,7 +1139,6 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *m)
>> 	mi->buf_addr = m->buf_addr;
>> 	mi->buf_len = m->buf_len;
>> 
>> -	mi->next = m->next;
> 
> Do you know why attaching mbuf is not supporting multi-segment?
> Perhaps this can be documented in function comment, as one of the "not
> supported" items.

No, I don’t know. For my application I’ve found that nb_segs with it’s limit in 256 segments is very annoying and I’ve decided not to use DPDK functions that dealt with nb_segs… But it is not about the rte_pktmbuf_attach() function and the patch.

> Also, should we check mi->next before overwriting, in case it is not NULL?
> 
>> 	mi->data_off = m->data_off;
>> 	mi->data_len = m->data_len;
>> 	mi->port = m->port;
>> 
> 

I don’t know. It depends of the usage. Will someone needs to chain two chains of mbuf?

Re: [PATCH] mbuf: remove redundant line in rte_pktmbuf_attach

[Ananyev, Konstantin]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ilya Matveychikov
> Sent: Saturday, January 21, 2017 3:08 PM
> To: Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] mbuf: remove redundant line in rte_pktmbuf_attach
> 
> 
> > On Jan 20, 2017, at 4:08 PM, Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> >
> > On 1/20/2017 12:19 AM, Ilya Matveychikov wrote:
> >> mi->next will be assigned to NULL few lines later, trivial patch
> >>
> >> Signed-off-by: Ilya V. Matveychikov <matvejchikov@gmail.com>
> >> ---
> >> lib/librte_mbuf/rte_mbuf.h | 1 -
> >> 1 file changed, 1 deletion(-)
> >>
> >> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> >> index ead7c6e..5589d54 100644
> >> --- a/lib/librte_mbuf/rte_mbuf.h
> >> +++ b/lib/librte_mbuf/rte_mbuf.h
> >> @@ -1139,7 +1139,6 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *m)
> >> 	mi->buf_addr = m->buf_addr;
> >> 	mi->buf_len = m->buf_len;
> >>
> >> -	mi->next = m->next;
> >
> > Do you know why attaching mbuf is not supporting multi-segment?

This is supported, but you have to do it segment by segment.
Actually  rte_pktmbuf_clone() does that.
Konstantin


> > Perhaps this can be documented in function comment, as one of the "not
> > supported" items.
> 
> No, I don’t know. For my application I’ve found that nb_segs with it’s limit in 256 segments is very annoying and I’ve decided not to use
> DPDK functions that dealt with nb_segs… But it is not about the rte_pktmbuf_attach() function and the patch.
> 
> > Also, should we check mi->next before overwriting, in case it is not NULL?
> >
> >> 	mi->data_off = m->data_off;
> >> 	mi->data_len = m->data_len;
> >> 	mi->port = m->port;
> >>
> >
> 
> I don’t know. It depends of the usage. Will someone needs to chain two chains of mbuf?

Re: [PATCH] mbuf: remove redundant line in rte_pktmbuf_attach

[Olivier MATZ]

Hi,

On Sat, 21 Jan 2017 16:28:29 +0000, "Ananyev, Konstantin"
<konstantin.ananyev@intel.com> wrote:
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ilya
> > Matveychikov Sent: Saturday, January 21, 2017 3:08 PM
> > To: Yigit, Ferruh <ferruh.yigit@intel.com>
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] mbuf: remove redundant line in
> > rte_pktmbuf_attach
> > 
> >   
> > > On Jan 20, 2017, at 4:08 PM, Ferruh Yigit
> > > <ferruh.yigit@intel.com> wrote:
> > >
> > > On 1/20/2017 12:19 AM, Ilya Matveychikov wrote:  
> > >> mi->next will be assigned to NULL few lines later, trivial patch
> > >>
> > >> Signed-off-by: Ilya V. Matveychikov <matvejchikov@gmail.com>
> > >> ---
> > >> lib/librte_mbuf/rte_mbuf.h | 1 -
> > >> 1 file changed, 1 deletion(-)
> > >>
> > >> diff --git a/lib/librte_mbuf/rte_mbuf.h
> > >> b/lib/librte_mbuf/rte_mbuf.h index ead7c6e..5589d54 100644
> > >> --- a/lib/librte_mbuf/rte_mbuf.h
> > >> +++ b/lib/librte_mbuf/rte_mbuf.h
> > >> @@ -1139,7 +1139,6 @@ static inline void
> > >> rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *m)
> > >> mi->buf_addr = m->buf_addr; mi->buf_len = m->buf_len;
> > >>
> > >> -	mi->next = m->next;  
> > >

Fixes: ea672a8b1655 ("mbuf: remove the rte_pktmbuf structure")

Acked-by: Olivier Matz <olivier.matz@6wind.com>


> > > Do you know why attaching mbuf is not supporting multi-segment?  
> 
> This is supported, but you have to do it segment by segment.
> Actually  rte_pktmbuf_clone() does that.
> Konstantin
> 
> 
> > > Perhaps this can be documented in function comment, as one of the
> > > "not supported" items.  
> > 
> > No, I don’t know. For my application I’ve found that nb_segs with
> > it’s limit in 256 segments is very annoying and I’ve decided not to
> > use DPDK functions that dealt with nb_segs… But it is not about the
> > rte_pktmbuf_attach() function and the patch. 


Out of curiosity, can you explain why your application needs more
than 256 segments? When we were discussing the possibility of extending
this field to 16 bits, Konstantin convinced me that it was not so
useful.


Thanks,
Olivier

Re: [PATCH] mbuf: remove redundant line in rte_pktmbuf_attach

[Ilya Matveychikov]

> On Jan 24, 2017, at 4:56 PM, Olivier MATZ <olivier.matz@6wind.com> wrote:
> 
> Hi,
> 
> On Sat, 21 Jan 2017 16:28:29 +0000, "Ananyev, Konstantin"
> <konstantin.ananyev@intel.com> wrote:
>>> -----Original Message-----
>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ilya
>>> Matveychikov Sent: Saturday, January 21, 2017 3:08 PM
>>> To: Yigit, Ferruh <ferruh.yigit@intel.com>
>>> Cc: dev@dpdk.org
>>> Subject: Re: [dpdk-dev] [PATCH] mbuf: remove redundant line in
>>> rte_pktmbuf_attach
>>> 
>>> 
>>>> On Jan 20, 2017, at 4:08 PM, Ferruh Yigit
>>>> <ferruh.yigit@intel.com> wrote:
>>>> 
>>>> On 1/20/2017 12:19 AM, Ilya Matveychikov wrote:  
>>>>> mi->next will be assigned to NULL few lines later, trivial patch
>>>>> 
>>>>> Signed-off-by: Ilya V. Matveychikov <matvejchikov@gmail.com>
>>>>> ---
>>>>> lib/librte_mbuf/rte_mbuf.h | 1 -
>>>>> 1 file changed, 1 deletion(-)
>>>>> 
>>>>> diff --git a/lib/librte_mbuf/rte_mbuf.h
>>>>> b/lib/librte_mbuf/rte_mbuf.h index ead7c6e..5589d54 100644
>>>>> --- a/lib/librte_mbuf/rte_mbuf.h
>>>>> +++ b/lib/librte_mbuf/rte_mbuf.h
>>>>> @@ -1139,7 +1139,6 @@ static inline void
>>>>> rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *m)
>>>>> mi->buf_addr = m->buf_addr; mi->buf_len = m->buf_len;
>>>>> 
>>>>> -	mi->next = m->next;  
>>>> 
> 
> Fixes: ea672a8b1655 ("mbuf: remove the rte_pktmbuf structure")
> 
> Acked-by: Olivier Matz <olivier.matz@6wind.com>
> 
> 
>>>> Do you know why attaching mbuf is not supporting multi-segment?  
>> 
>> This is supported, but you have to do it segment by segment.
>> Actually  rte_pktmbuf_clone() does that.
>> Konstantin
>> 
>> 
>>>> Perhaps this can be documented in function comment, as one of the
>>>> "not supported" items.  
>>> 
>>> No, I don’t know. For my application I’ve found that nb_segs with
>>> it’s limit in 256 segments is very annoying and I’ve decided not to
>>> use DPDK functions that dealt with nb_segs… But it is not about the
>>> rte_pktmbuf_attach() function and the patch. 
> 
> 
> Out of curiosity, can you explain why your application needs more
> than 256 segments? When we were discussing the possibility of extending
> this field to 16 bits, Konstantin convinced me that it was not so
> useful.

In my application I need to do IPv4 fragments reassembly. There is no explicit limit of number of fragments in datagram, so I’m trying to avoid any limitations and `nb_segs` here is a constraint for me. Expanding it from 8-bit to 16-bit can solve that issue for me. But I don’t remember are there any  other places in DPDK where we need to know how many segments are in the packet? I mean that is `nb_segs` required at all?

Re: [PATCH] mbuf: remove redundant line in rte_pktmbuf_attach

[Olivier MATZ]

On Tue, 24 Jan 2017 19:57:13 +0400, Ilya Matveychikov
<matvejchikov@gmail.com> wrote:
> > On Jan 24, 2017, at 4:56 PM, Olivier MATZ <olivier.matz@6wind.com>
> > wrote:
> > 
> > Hi,
> > 
> > On Sat, 21 Jan 2017 16:28:29 +0000, "Ananyev, Konstantin"
> > <konstantin.ananyev@intel.com> wrote:  
> >>> -----Original Message-----
> >>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ilya
> >>> Matveychikov Sent: Saturday, January 21, 2017 3:08 PM
> >>> To: Yigit, Ferruh <ferruh.yigit@intel.com>
> >>> Cc: dev@dpdk.org
> >>> Subject: Re: [dpdk-dev] [PATCH] mbuf: remove redundant line in
> >>> rte_pktmbuf_attach
> >>> 
> >>>   
> >>>> On Jan 20, 2017, at 4:08 PM, Ferruh Yigit
> >>>> <ferruh.yigit@intel.com> wrote:
> >>>> 
> >>>> On 1/20/2017 12:19 AM, Ilya Matveychikov wrote:    
> >>>>> mi->next will be assigned to NULL few lines later, trivial patch
> >>>>> 
> >>>>> Signed-off-by: Ilya V. Matveychikov <matvejchikov@gmail.com>
> >>>>> ---
> >>>>> lib/librte_mbuf/rte_mbuf.h | 1 -
> >>>>> 1 file changed, 1 deletion(-)
> >>>>> 
> >>>>> diff --git a/lib/librte_mbuf/rte_mbuf.h
> >>>>> b/lib/librte_mbuf/rte_mbuf.h index ead7c6e..5589d54 100644
> >>>>> --- a/lib/librte_mbuf/rte_mbuf.h
> >>>>> +++ b/lib/librte_mbuf/rte_mbuf.h
> >>>>> @@ -1139,7 +1139,6 @@ static inline void
> >>>>> rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *m)
> >>>>> mi->buf_addr = m->buf_addr; mi->buf_len = m->buf_len;
> >>>>> 
> >>>>> -	mi->next = m->next;    
> >>>>   
> > 
> > Fixes: ea672a8b1655 ("mbuf: remove the rte_pktmbuf structure")
> > 
> > Acked-by: Olivier Matz <olivier.matz@6wind.com>
> > 
> >   
> >>>> Do you know why attaching mbuf is not supporting
> >>>> multi-segment?    
> >> 
> >> This is supported, but you have to do it segment by segment.
> >> Actually  rte_pktmbuf_clone() does that.
> >> Konstantin
> >> 
> >>   
> >>>> Perhaps this can be documented in function comment, as one of the
> >>>> "not supported" items.    
> >>> 
> >>> No, I don’t know. For my application I’ve found that nb_segs with
> >>> it’s limit in 256 segments is very annoying and I’ve decided not
> >>> to use DPDK functions that dealt with nb_segs… But it is not
> >>> about the rte_pktmbuf_attach() function and the patch.   
> > 
> > 
> > Out of curiosity, can you explain why your application needs more
> > than 256 segments? When we were discussing the possibility of
> > extending this field to 16 bits, Konstantin convinced me that it
> > was not so useful.  
> 
> In my application I need to do IPv4 fragments reassembly. There is no
> explicit limit of number of fragments in datagram, so I’m trying to
> avoid any limitations and `nb_segs` here is a constraint for me.
> Expanding it from 8-bit to 16-bit can solve that issue for me. But I
> don’t remember are there any  other places in DPDK where we need to
> know how many segments are in the packet? I mean that is `nb_segs`
> required at all?
> 

Yes, it is used for instance in some PMDs to know how many tx ring
descriptors are needed to send a packet.

Thank you for the explanation. As you probably seen, I'm proposing to
extend the nb_segs to 16 bits in my latest RFC patchset.

Regards,
Olivier

Re: [PATCH] mbuf: remove redundant line in rte_pktmbuf_attach

[Thomas Monjalon]

> > > >> mi->next will be assigned to NULL few lines later, trivial patch
> > > >>
> > > >> Signed-off-by: Ilya V. Matveychikov <matvejchikov@gmail.com>
> 
> Fixes: ea672a8b1655 ("mbuf: remove the rte_pktmbuf structure")
> 
> Acked-by: Olivier Matz <olivier.matz@6wind.com>

Applied, thanks