* Re: [PATCH] Revert "bonding: use existing enslaved device queues"
From: Bruce Richardson @ 2016-10-25 13:59 UTC (permalink / raw)
To: Declan Doherty
Cc: Ilya Maximets, dev, Heetae Ahn, Yuanhan Liu, Eric Kinzie,
Bernard Iremonger, stable
In-Reply-To: <20161019095525.GK27816@bricha3-MOBL3.ger.corp.intel.com>
On Wed, Oct 19, 2016 at 10:55:25AM +0100, Bruce Richardson wrote:
> On Thu, Oct 06, 2016 at 03:32:36PM +0100, Declan Doherty wrote:
> > On 07/09/16 13:28, Ilya Maximets wrote:
> > > This reverts commit 5b7bb2bda5519b7800f814df64d4e015282140e5.
> > >
> > > It is necessary to reconfigure all queues every time because configuration
> > > can be changed.
> > >
> >
> > Hey Ilya, this makes sense. I guess this was my original intention but I
> > missed this case in my review of the change.
> >
> > > For example, if we're reconfiguring bonding device with new memory pool,
> > > already configured queues will still use the old one. And if the old
> > > mempool be freed, application likely will panic in attempt to use
> > > freed mempool.
> > >
> > > This happens when we use the bonding device with OVS 2.6 while MTU
> > > reconfiguration:
> > >
> > > PANIC in rte_mempool_get_ops():
> > > assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)" failed
> > >
> > > Cc: <stable@dpdk.org>
> > > Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> > > ---
> >
> > Acked-by: Declan Doherty <declan.doherty@intel.com>
>
> Applied to dpdk-next-net/rel_16_11
>
Patch taken out of branch due to on-going discussion in this thread. It
can be re-applied later if consensus is reached that it is ok.
Apologies for any confusion caused by this, but after discussion with
the driver maintainer, we feel this is the safest course for now.
/Bruce
^ permalink raw reply
* Re: mbuf changes
From: Ananyev, Konstantin @ 2016-10-25 13:58 UTC (permalink / raw)
To: Adrien Mazarguil, Morten Brørup
Cc: Richardson, Bruce, Wiles, Keith, dev@dpdk.org, Olivier Matz,
Oleg Kuporosov
In-Reply-To: <20161025134817.GL5733@6wind.com>
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Adrien Mazarguil
> Sent: Tuesday, October 25, 2016 2:48 PM
> To: Morten Brørup <mb@smartsharesystems.com>
> Cc: Richardson, Bruce <bruce.richardson@intel.com>; Wiles, Keith <keith.wiles@intel.com>; dev@dpdk.org; Olivier Matz
> <olivier.matz@6wind.com>; Oleg Kuporosov <olegk@mellanox.com>
> Subject: Re: [dpdk-dev] mbuf changes
>
> On Tue, Oct 25, 2016 at 02:16:29PM +0200, Morten Brørup wrote:
> > Comments inline.
>
> I'm only replying to the nb_segs bits here.
>
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
> > > Sent: Tuesday, October 25, 2016 1:14 PM
> > > To: Adrien Mazarguil
> > > Cc: Morten Brørup; Wiles, Keith; dev@dpdk.org; Olivier Matz; Oleg
> > > Kuporosov
> > > Subject: Re: [dpdk-dev] mbuf changes
> > >
> > > On Tue, Oct 25, 2016 at 01:04:44PM +0200, Adrien Mazarguil wrote:
> > > > On Tue, Oct 25, 2016 at 12:11:04PM +0200, Morten Brørup wrote:
> > > > > Comments inline.
> > > > >
> > > > > Med venlig hilsen / kind regards
> > > > > - Morten Brørup
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > > > > > Sent: Tuesday, October 25, 2016 11:39 AM
> > > > > > To: Bruce Richardson
> > > > > > Cc: Wiles, Keith; Morten Brørup; dev@dpdk.org; Olivier Matz; Oleg
> > > > > > Kuporosov
> > > > > > Subject: Re: [dpdk-dev] mbuf changes
> > > > > >
> > > > > > On Mon, Oct 24, 2016 at 05:25:38PM +0100, Bruce Richardson wrote:
> > > > > > > On Mon, Oct 24, 2016 at 04:11:33PM +0000, Wiles, Keith wrote:
> > > > > > [...]
> > > > > > > > > On Oct 24, 2016, at 10:49 AM, Morten Brørup
> > > > > > <mb@smartsharesystems.com> wrote:
> > > > > > [...]
> > > > > > > > > 5.
> > > > > > > > >
> > > > > > > > > And here’s something new to think about:
> > > > > > > > >
> > > > > > > > > m->next already reveals if there are more segments to a
> > > packet.
> > > > > > Which purpose does m->nb_segs serve that is not already covered
> > > by
> > > > > > m-
> > > > > > >next?
> > > > > > >
> > > > > > > It is duplicate info, but nb_segs can be used to check the
> > > > > > > validity
> > > > > > of
> > > > > > > the next pointer without having to read the second mbuf
> > > cacheline.
> > > > > > >
> > > > > > > Whether it's worth having is something I'm happy enough to
> > > > > > > discuss, though.
> > > > > >
> > > > > > Although slower in some cases than a full blown "next packet"
> > > > > > pointer, nb_segs can also be conveniently abused to link several
> > > > > > packets and their segments in the same list without wasting
> > > space.
> > > > >
> > > > > I don’t understand that; can you please elaborate? Are you abusing
> > > m->nb_segs as an index into an array in your application? If that is
> > > the case, and it is endorsed by the community, we should get rid of m-
> > > >nb_segs and add a member for application specific use instead.
> > > >
> > > > Well, that's just an idea, I'm not aware of any application using
> > > > this, however the ability to link several packets with segments seems
> > > > useful to me (e.g. buffering packets). Here's a diagram:
> > > >
> > > > .-----------. .-----------. .-----------. .-----------. .---
> > > ---
> > > > | pkt 0 | | seg 1 | | seg 2 | | pkt 1 | |
> > > pkt 2
> > > > | next --->| next --->| next --->| next --->|
> > > ...
> > > > | nb_segs 3 | | nb_segs 1 | | nb_segs 1 | | nb_segs 1 | |
> > > > `-----------' `-----------' `-----------' `-----------' `---
> > > ---
> >
> > I see. It makes it possible to refer to a burst of packets (with segments or not) by a single mbuf reference, as an alternative to the current
> design pattern of using an array and length (struct rte_mbuf **mbufs, unsigned count).
> >
> > This would require implementation in the PMDs etc.
> >
> > And even in this case, m->nb_segs does not need to be an integer, but could be replaced by a single bit indicating if the segment is a
> continuation of a packet or the beginning (alternatively the end) of a packet, i.e. the bit can be set for either the first or the last segment in
> the packet.
We do need nb_segs - at least for TX.
That's how TX function calculates how many TXDs it needs to allocate(and fill).
Of-course it can re-scan whole chain of segments to count them, but I think
it would slowdown things even more.
Though yes, I suppose it can be moved to the second cahe-line.
Konstantin
>
> Sure however if we keep the current definition, a single bit would not be
> enough as it must be nonzero for the buffer to be valid. I think a 8 bit
> field is not that expensive for a counter.
>
> > It is an almost equivalent alternative to the fundamental design pattern of using an array of mbuf with count, which is widely implemented
> in DPDK. And m->next still lives in the second cache line, so I don't see any gain by this.
>
> That's right, it does not have to live in the first cache line, my only
> concern was its entire removal.
>
> > I still don't get how m->nb_segs can be abused without m->next.
>
> By "abused" I mean that applications are not supposed to pass this kind of
> mbuf lists directly to existing mbuf-handling functions (TX burst,
> rte_pktmbuf_free() and so on), however these same applications (even PMDs)
> can do so internally temporarily because it's so simple.
>
> The next pointer of the last segment of a packet must still be set to NULL
> every time a packet is retrieved from such a list to be processed.
>
> > > However, nb_segs may be a good candidate for demotion, along with
> > > possibly the port value, or the reference count.
>
> Yes, I think that's fine as long as it's kept somewhere.
>
> --
> Adrien Mazarguil
> 6WIND
^ permalink raw reply
* Re: mbuf changes
From: Adrien Mazarguil @ 2016-10-25 13:48 UTC (permalink / raw)
To: Morten Brørup
Cc: Bruce Richardson, Wiles, Keith, dev, Olivier Matz, Oleg Kuporosov
In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC359EA8BA@smartserver.smartshare.dk>
On Tue, Oct 25, 2016 at 02:16:29PM +0200, Morten Brørup wrote:
> Comments inline.
I'm only replying to the nb_segs bits here.
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
> > Sent: Tuesday, October 25, 2016 1:14 PM
> > To: Adrien Mazarguil
> > Cc: Morten Brørup; Wiles, Keith; dev@dpdk.org; Olivier Matz; Oleg
> > Kuporosov
> > Subject: Re: [dpdk-dev] mbuf changes
> >
> > On Tue, Oct 25, 2016 at 01:04:44PM +0200, Adrien Mazarguil wrote:
> > > On Tue, Oct 25, 2016 at 12:11:04PM +0200, Morten Brørup wrote:
> > > > Comments inline.
> > > >
> > > > Med venlig hilsen / kind regards
> > > > - Morten Brørup
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > > > > Sent: Tuesday, October 25, 2016 11:39 AM
> > > > > To: Bruce Richardson
> > > > > Cc: Wiles, Keith; Morten Brørup; dev@dpdk.org; Olivier Matz; Oleg
> > > > > Kuporosov
> > > > > Subject: Re: [dpdk-dev] mbuf changes
> > > > >
> > > > > On Mon, Oct 24, 2016 at 05:25:38PM +0100, Bruce Richardson wrote:
> > > > > > On Mon, Oct 24, 2016 at 04:11:33PM +0000, Wiles, Keith wrote:
> > > > > [...]
> > > > > > > > On Oct 24, 2016, at 10:49 AM, Morten Brørup
> > > > > <mb@smartsharesystems.com> wrote:
> > > > > [...]
> > > > > > > > 5.
> > > > > > > >
> > > > > > > > And here’s something new to think about:
> > > > > > > >
> > > > > > > > m->next already reveals if there are more segments to a
> > packet.
> > > > > Which purpose does m->nb_segs serve that is not already covered
> > by
> > > > > m-
> > > > > >next?
> > > > > >
> > > > > > It is duplicate info, but nb_segs can be used to check the
> > > > > > validity
> > > > > of
> > > > > > the next pointer without having to read the second mbuf
> > cacheline.
> > > > > >
> > > > > > Whether it's worth having is something I'm happy enough to
> > > > > > discuss, though.
> > > > >
> > > > > Although slower in some cases than a full blown "next packet"
> > > > > pointer, nb_segs can also be conveniently abused to link several
> > > > > packets and their segments in the same list without wasting
> > space.
> > > >
> > > > I don’t understand that; can you please elaborate? Are you abusing
> > m->nb_segs as an index into an array in your application? If that is
> > the case, and it is endorsed by the community, we should get rid of m-
> > >nb_segs and add a member for application specific use instead.
> > >
> > > Well, that's just an idea, I'm not aware of any application using
> > > this, however the ability to link several packets with segments seems
> > > useful to me (e.g. buffering packets). Here's a diagram:
> > >
> > > .-----------. .-----------. .-----------. .-----------. .---
> > ---
> > > | pkt 0 | | seg 1 | | seg 2 | | pkt 1 | |
> > pkt 2
> > > | next --->| next --->| next --->| next --->|
> > ...
> > > | nb_segs 3 | | nb_segs 1 | | nb_segs 1 | | nb_segs 1 | |
> > > `-----------' `-----------' `-----------' `-----------' `---
> > ---
>
> I see. It makes it possible to refer to a burst of packets (with segments or not) by a single mbuf reference, as an alternative to the current design pattern of using an array and length (struct rte_mbuf **mbufs, unsigned count).
>
> This would require implementation in the PMDs etc.
>
> And even in this case, m->nb_segs does not need to be an integer, but could be replaced by a single bit indicating if the segment is a continuation of a packet or the beginning (alternatively the end) of a packet, i.e. the bit can be set for either the first or the last segment in the packet.
Sure however if we keep the current definition, a single bit would not be
enough as it must be nonzero for the buffer to be valid. I think a 8 bit
field is not that expensive for a counter.
> It is an almost equivalent alternative to the fundamental design pattern of using an array of mbuf with count, which is widely implemented in DPDK. And m->next still lives in the second cache line, so I don't see any gain by this.
That's right, it does not have to live in the first cache line, my only
concern was its entire removal.
> I still don't get how m->nb_segs can be abused without m->next.
By "abused" I mean that applications are not supposed to pass this kind of
mbuf lists directly to existing mbuf-handling functions (TX burst,
rte_pktmbuf_free() and so on), however these same applications (even PMDs)
can do so internally temporarily because it's so simple.
The next pointer of the last segment of a packet must still be set to NULL
every time a packet is retrieved from such a list to be processed.
> > However, nb_segs may be a good candidate for demotion, along with
> > possibly the port value, or the reference count.
Yes, I think that's fine as long as it's kept somewhere.
--
Adrien Mazarguil
6WIND
^ permalink raw reply
* Re: [PATCH] Revert "bonding: use existing enslaved device queues"
From: Declan Doherty @ 2016-10-25 13:48 UTC (permalink / raw)
To: Bruce Richardson
Cc: Jan Blunck, Eric Kinzie, Ilya Maximets, dev, Heetae Ahn,
Yuanhan Liu, Bernard Iremonger, stable, Thomas Monjalon
In-Reply-To: <20161025125750.GB57276@bricha3-MOBL3.ger.corp.intel.com>
On 25/10/16 13:57, Bruce Richardson wrote:
> On Mon, Oct 24, 2016 at 04:07:17PM +0100, Declan Doherty wrote:
>> On 24/10/16 15:51, Jan Blunck wrote:
>>> On Mon, Oct 24, 2016 at 7:02 AM, Declan Doherty
>>> <declan.doherty@intel.com> wrote:
>>>> On 14/10/16 00:37, Eric Kinzie wrote:
>>>>>
>>>>> On Wed Oct 12 16:24:21 +0100 2016, Bruce Richardson wrote:
>>>>>>
>>>>>> On Wed, Oct 12, 2016 at 04:24:54PM +0300, Ilya Maximets wrote:
>>>>>>>
>>>>>>> On 07.10.2016 05:02, Eric Kinzie wrote:
>>>>>>>>
>>>>>>>> On Wed Sep 07 15:28:10 +0300 2016, Ilya Maximets wrote:
>>>>>>>>>
>>>>>>>>> This reverts commit 5b7bb2bda5519b7800f814df64d4e015282140e5.
>>>>>>>>>
>>>>>>>>> It is necessary to reconfigure all queues every time because
>>>>>>>>> configuration
>>>>>>>>> can be changed.
>>>>>>>>>
>>>>>>>>> For example, if we're reconfiguring bonding device with new memory
>>>>>>>>> pool,
>>>>>>>>> already configured queues will still use the old one. And if the old
>>>>>>>>> mempool be freed, application likely will panic in attempt to use
>>>>>>>>> freed mempool.
>>>>>>>>>
>>>>>>>>> This happens when we use the bonding device with OVS 2.6 while MTU
>>>>>>>>> reconfiguration:
>>>>>>>>>
>>>>>>>>> PANIC in rte_mempool_get_ops():
>>>>>>>>> assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)"
>>>>>>>>> failed
>>>>>>>>>
>>>>>>>>> Cc: <stable@dpdk.org>
>>>>>>>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>>>>>>>>> ---
>>>>>>>>> drivers/net/bonding/rte_eth_bond_pmd.c | 10 ++--------
>>>>>>>>> 1 file changed, 2 insertions(+), 8 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>>>>>> b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>>>>>> index b20a272..eb5b6d1 100644
>>>>>>>>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>>>>>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>>>>>> @@ -1305,8 +1305,6 @@ slave_configure(struct rte_eth_dev
>>>>>>>>> *bonded_eth_dev,
>>>>>>>>> struct bond_rx_queue *bd_rx_q;
>>>>>>>>> struct bond_tx_queue *bd_tx_q;
>>>>>>>>>
>>>>>>>>> - uint16_t old_nb_tx_queues = slave_eth_dev->data->nb_tx_queues;
>>>>>>>>> - uint16_t old_nb_rx_queues = slave_eth_dev->data->nb_rx_queues;
>>>>>>>>> int errval;
>>>>>>>>> uint16_t q_id;
>>>>>>>>>
>>>>>>>>> @@ -1347,9 +1345,7 @@ slave_configure(struct rte_eth_dev
>>>>>>>>> *bonded_eth_dev,
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> /* Setup Rx Queues */
>>>>>>>>> - /* Use existing queues, if any */
>>>>>>>>> - for (q_id = old_nb_rx_queues;
>>>>>>>>> - q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
>>>>>>>>> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_rx_queues;
>>>>>>>>> q_id++) {
>>>>>>>>> bd_rx_q = (struct bond_rx_queue
>>>>>>>>> *)bonded_eth_dev->data->rx_queues[q_id];
>>>>>>>>>
>>>>>>>>> errval =
>>>>>>>>> rte_eth_rx_queue_setup(slave_eth_dev->data->port_id, q_id,
>>>>>>>>> @@ -1365,9 +1361,7 @@ slave_configure(struct rte_eth_dev
>>>>>>>>> *bonded_eth_dev,
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> /* Setup Tx Queues */
>>>>>>>>> - /* Use existing queues, if any */
>>>>>>>>> - for (q_id = old_nb_tx_queues;
>>>>>>>>> - q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
>>>>>>>>> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_tx_queues;
>>>>>>>>> q_id++) {
>>>>>>>>> bd_tx_q = (struct bond_tx_queue
>>>>>>>>> *)bonded_eth_dev->data->tx_queues[q_id];
>>>>>>>>>
>>>>>>>>> errval =
>>>>>>>>> rte_eth_tx_queue_setup(slave_eth_dev->data->port_id, q_id,
>>>>>>>>> --
>>>>>>>>> 2.7.4
>>>>>>>>>
>>>>>>>>
>>>>>>>> NAK
>>>>>>>>
>>>>>>>> There are still some users of this code. Let's give them a chance to
>>>>>>>> comment before removing it.
>>>>>>>
>>>>>>>
>>>>>>> Hi Eric,
>>>>>>>
>>>>>>> Are these users in CC-list? If not, could you, please, add them?
>>>>>>> This patch awaits in mail-list already more than a month. I think, it's
>>>>>>> enough
>>>>>>> time period for all who wants to say something. Patch fixes a real bug
>>>>>>> that
>>>>>>> prevent using of DPDK bonding in all applications that reconfigures
>>>>>>> devices
>>>>>>> in runtime including OVS.
>>>>>>>
>>>>>> Agreed.
>>>>>>
>>>>>> Eric, does reverting this patch cause you problems directly, or is your
>>>>>> concern
>>>>>> just with regards to potential impact to others?
>>>>>>
>>>>>> Thanks,
>>>>>> /Bruce
>>>>>
>>>>>
>>>>> This won't impact me directly. The users are CCed (different thread)
>>>>> and I haven't seen any comment, so I no longer have any objection to
>>>>> reverting this change.
>>>>>
>>>>> Eric
>>>>>
>>>>
>>>> As there has been no further objections and this reinstates the original
>>>> expected behavior of the bonding driver. I'm re-ack'ing for inclusion in
>>>> release.
>>>>
>>>> Acked-by: Declan Doherty <declan.doherty@intel.com>
>>>
>>> Ok, I can revert the revert for us.
>>>
>>> Do I read this correctly that you are not interested in fixing this properly?!
>>>
>>> Thanks,
>>> Jan
>>>
>>
>> Jan, sorry I missed the replies from last week due to the way my mail client
>> was filtering the conversation. Let me have another look at this and I'll
>> come back to the list.
>>
>> Thanks
>> Declan
>
> While this patch has already been applied to dpdk-next-net tree, it
> appears that there is still some ongoing discussion about it. I'm
> therefore planning to pull it back out of the tree for rc2. If a
> subsequent consensus is reached we can see about including it in rc3.
>
> Declan, as maintainer, does this seem reasonable to you.
>
> Regards,
> /Bruce
>
Hey Bruce, that seems reasonable, I would like to discuss this further
with Jan and Ilya.
Declan
^ permalink raw reply
* Re: mbuf changes
From: Ananyev, Konstantin @ 2016-10-25 13:38 UTC (permalink / raw)
To: Olivier Matz, Richardson, Bruce, Morten Brørup
Cc: Adrien Mazarguil, Wiles, Keith, dev@dpdk.org, Oleg Kuporosov
In-Reply-To: <4427b23b-e9c0-4e43-9c28-fb7e56da745c@6wind.com>
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
> Sent: Tuesday, October 25, 2016 1:49 PM
> To: Richardson, Bruce <bruce.richardson@intel.com>; Morten Brørup <mb@smartsharesystems.com>
> Cc: Adrien Mazarguil <adrien.mazarguil@6wind.com>; Wiles, Keith <keith.wiles@intel.com>; dev@dpdk.org; Oleg Kuporosov
> <olegk@mellanox.com>
> Subject: Re: [dpdk-dev] mbuf changes
>
>
>
> On 10/25/2016 02:45 PM, Bruce Richardson wrote:
> > On Tue, Oct 25, 2016 at 02:33:55PM +0200, Morten Brørup wrote:
> >> Comments at the end.
> >>
> >> Med venlig hilsen / kind regards
> >> - Morten Brørup
> >>
> >>> -----Original Message-----
> >>> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> >>> Sent: Tuesday, October 25, 2016 2:20 PM
> >>> To: Morten Brørup
> >>> Cc: Adrien Mazarguil; Wiles, Keith; dev@dpdk.org; Olivier Matz; Oleg
> >>> Kuporosov
> >>> Subject: Re: [dpdk-dev] mbuf changes
> >>>
> >>> On Tue, Oct 25, 2016 at 02:16:29PM +0200, Morten Brørup wrote:
> >>>> Comments inline.
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce
> >>>>> Richardson
> >>>>> Sent: Tuesday, October 25, 2016 1:14 PM
> >>>>> To: Adrien Mazarguil
> >>>>> Cc: Morten Brørup; Wiles, Keith; dev@dpdk.org; Olivier Matz; Oleg
> >>>>> Kuporosov
> >>>>> Subject: Re: [dpdk-dev] mbuf changes
> >>>>>
> >>>>> On Tue, Oct 25, 2016 at 01:04:44PM +0200, Adrien Mazarguil wrote:
> >>>>>> On Tue, Oct 25, 2016 at 12:11:04PM +0200, Morten Brørup wrote:
> >>>>>>> Comments inline.
> >>>>>>>
> >>>>>>> Med venlig hilsen / kind regards
> >>>>>>> - Morten Brørup
> >>>>>>>
> >>>>>>>
> >>>>>>>> -----Original Message-----
> >>>>>>>> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> >>>>>>>> Sent: Tuesday, October 25, 2016 11:39 AM
> >>>>>>>> To: Bruce Richardson
> >>>>>>>> Cc: Wiles, Keith; Morten Brørup; dev@dpdk.org; Olivier Matz;
> >>>>>>>> Oleg Kuporosov
> >>>>>>>> Subject: Re: [dpdk-dev] mbuf changes
> >>>>>>>>
> >>>>>>>> On Mon, Oct 24, 2016 at 05:25:38PM +0100, Bruce Richardson
> >>> wrote:
> >>>>>>>>> On Mon, Oct 24, 2016 at 04:11:33PM +0000, Wiles, Keith
> >>> wrote:
> >>>>>>>> [...]
> >>>>>>>>>>> On Oct 24, 2016, at 10:49 AM, Morten Brørup
> >>>>>>>> <mb@smartsharesystems.com> wrote:
> >>>>>>>> [...]
> >>>>
> >>>>>>>>> One other point I'll mention is that we need to have a
> >>>>>>>>> discussion on how/where to add in a timestamp value into
> >>> the
> >>>>>>>>> mbuf. Personally, I think it can be in a union with the
> >>>>> sequence
> >>>>>>>>> number value, but I also suspect that 32-bits of a
> >>> timestamp
> >>>>>>>>> is not going to be enough for
> >>>>>>>> many.
> >>>>>>>>>
> >>>>>>>>> Thoughts?
> >>>>>>>>
> >>>>>>>> If we consider that timestamp representation should use
> >>>>> nanosecond
> >>>>>>>> granularity, a 32-bit value may likely wrap around too
> >>> quickly
> >>>>>>>> to be useful. We can also assume that applications requesting
> >>>>>>>> timestamps may care more about latency than throughput, Oleg
> >>>>> found
> >>>>>>>> that using the second cache line for this purpose had a
> >>>>> noticeable impact [1].
> >>>>>>>>
> >>>>>>>> [1] http://dpdk.org/ml/archives/dev/2016-October/049237.html
> >>>>>>>
> >>>>>>> I agree with Oleg about the latency vs. throughput importance
> >>>>>>> for
> >>>>> such applications.
> >>>>>>>
> >>>>>>> If you need high resolution timestamps, consider them to be
> >>>>> generated by the NIC RX driver, possibly by the hardware itself
> >>>>> (http://w3new.napatech.com/features/time-precision/hardware-time-
> >>>>> stamp), so the timestamp belongs in the first cache line. And I am
> >>>>> proposing that it should have the highest possible accuracy, which
> >>>>> makes the value hardware dependent.
> >>>>>>>
> >>>>>>> Furthermore, I am arguing that we leave it up to the
> >>> application
> >>>>>>> to
> >>>>> keep track of the slowly moving bits (i.e. counting whole seconds,
> >>>>> hours and calendar date) out of band, so we don't use precious
> >>> space
> >>>>> in the mbuf. The application doesn't need the NIC RX driver's fast
> >>>>> path to capture which date (or even which second) a packet was
> >>>>> received. Yes, it adds complexity to the application, but we can't
> >>>>> set aside 64 bit for a generic timestamp. Or as a weird tradeoff:
> >>>>> Put the fast moving 32 bit in the first cache line and the slow
> >>>>> moving 32 bit in the second cache line, as a placeholder for the
> >>> application to fill out if needed.
> >>>>> Yes, it means that the application needs to check the time and
> >>>>> update its variable holding the slow moving time once every second
> >>>>> or so; but that should be doable without significant effort.
> >>>>>>
> >>>>>> That's a good point, however without a 64 bit value, elapsed time
> >>>>>> between two arbitrary mbufs cannot be measured reliably due to
> >>> not
> >>>>>> enough context, one way or another the low resolution value is
> >>>>>> also
> >>>>> needed.
> >>>>>>
> >>>>>> Obviously latency-sensitive applications are unlikely to perform
> >>>>>> lengthy buffering and require this but I'm not sure about all the
> >>>>>> possible use-cases. Considering many NICs expose 64 bit
> >>> timestaps,
> >>>>>> I suggest we do not truncate them.
> >>>>>>
> >>>>>> I'm not a fan of the weird tradeoff either, PMDs will be tempted
> >>>>>> to fill the extra 32 bits whenever they can and negate the
> >>>>>> performance improvement of the first cache line.
> >>>>>
> >>>>> I would tend to agree, and I don't really see any convenient way to
> >>>>> avoid putting in a 64-bit field for the timestamp in cache-line 0.
> >>>>> If we are ok with having this overlap/partially overlap with
> >>>>> sequence number, it will use up an extra 4B of storage in that
> >>> cacheline.
> >>>>
> >>>> I agree about the lack of convenience! And Adrien certainly has a
> >>> point about PMD temptations.
> >>>>
> >>>> However, I still don't think that a NICs ability to date-stamp a
> >>> packet is sufficient reason to put a date-stamp in cache line 0 of the
> >>> mbuf. Storing only the fast moving 32 bit in cache line 0 seems like a
> >>> good compromise to me.
> >>>>
> >>>> Maybe you can find just one more byte, so it can hold 17 minutes with
> >>>> nanosecond resolution. (I'm joking!)
> >>>>
> >>>> Please don't sacrifice the sequence number for the seconds/hours/days
> >>> part a timestamp. Maybe it could be configurable to use a 32 bit or 64
> >>> bit timestamp.
> >>>>
> >>> Do you see both timestamp and sequence numbers being used together? I
> >>> would have thought that apps would either use one or the other?
> >>> However, your suggestion is workable in any case, to allow the sequence
> >>> number to overlap just the high 32 bits of the timestamp, rather than
> >>> the low.
> >>
> >> In our case, I can foresee sequence numbers used for packet processing and timestamps for timing analysis (and possibly for packet
> capturing, when being used).
Great, but right now none of these fields are filled from NIC HW by PMD RX function
(except RFC for melanox provided by Oleg, but again it is pure SW implementation).
So I would repeat my question: why these fields should stay in the first cache-line?
I understand that it would speed-up some particular application, but there are plenty
of apps which do use different metadata.
Let say l2/l3/l4 len - is very useful information for upper layer (L3/L4) packet processing.
Should people who do use it start to push moving that fields into first cache-line too?
>For timing analysis, we don’t need long durations, e.g. 4 seconds with 32 bit nanosecond resolution suffices.
> And for packet capturing we are perfectly capable of adding the slowly moving 32 bit of the timestamp to our output data stream without
> fetching it from the mbuf.
> >>
>
> We should keep in mind that today we have the seqn field but it is
> not used by any PMD. In case it is implemented, would it be a per-queue
> sequence number? Is it useful from an application view?
Exactly - it depends from SW that uses that field, right?
It could be per process / per group of lcores / per port / per queue, etc.
Depending on what are upper layer needs.
>
> This field is only used by the librte_reorder library, and in my
> opinion, we should consider moving it in the second cache line since
> it is not filled by the PMD.
+1
>
>
> > For the 32-bit timestamp case, it might be useful to have a right-shift
> > value passed in to the ethdev driver. If we assume a NIC with nanosecond
> > resolution, (or TSC value with resolution of that order of magnitude),
> > then the app can choose to have 1 ns resolution with 4 second
> > wraparound, or alternatively 4ns resolution with 16 second wraparound,
> > or even microsecond resolution with wrap around of over an hour.
> > The cost is obviously just a shift op in the driver code per packet -
> > hopefully with multiple packets done at a time using vector operations.
>
>
> About the timestamp, we can manage to find 64 bits in the first cache
> line, without sacrifying any field we have today.
We can I suppose, but again what for and who will fill it?
If PMD, then where it will get this information?
If it is from rte_rdtsc() or clock(), then why upper layer can't do it itself?
Konstantin
>The question is more
> for the fields we may want to add later.
>
> To answer to the question of the size of the timestamp, the first
> question is to know what is the precision required for the
> applications using it?
>
> I don't quite like the idea of splitting the timestamp in the 2 cache
> lines, I think it would not be easy to use.
>
>
> Olivier
^ permalink raw reply
* Re: mbuf changes
From: Thomas Monjalon @ 2016-10-25 13:24 UTC (permalink / raw)
To: Ramia, Kannan Babu
Cc: dev, Olivier Matz, Morten Brørup, Ananyev, Konstantin,
Richardson, Bruce, Wiles, Keith
In-Reply-To: <5wj6euqlvai26vmpn4dcfe4p.1477400683290@email.android.com>
2016-10-25 13:04, Ramia, Kannan Babu:
> Port filed is important meta information for the application use like
> CGNAT vEPC functions etc.
> I strongly recommend to keep the field in mind meta.
Have you tried to move this field outside of the mbuf?
What is the performance degradation?
We need more information than some assumptions.
^ permalink raw reply
* Re: mbuf changes
From: Thomas Monjalon @ 2016-10-25 13:20 UTC (permalink / raw)
To: Olivier Matz
Cc: dev, Morten Brørup, Ananyev, Konstantin, Richardson, Bruce,
Wiles, Keith
In-Reply-To: <39b70924-0e66-478c-36ea-dd18d27ed8a6@6wind.com>
2016-10-25 15:00, Olivier Matz:
> On 10/25/2016 12:22 PM, Morten Brørup wrote:
> > From: Ananyev, Konstantin
> >> From: Bruce Richardson
> >>> On Mon, Oct 24, 2016 at 11:47:16PM +0200, Morten Brørup wrote:
> >>>> From: Bruce Richardson
> >>>>> On Mon, Oct 24, 2016 at 04:11:33PM +0000, Wiles, Keith wrote:
> >>>>>> I thought we also talked about removing the m->port from the
> >>>>>> mbuf as it is not really needed.
> >>>>>>
> >>>>> Yes, this was mentioned, and also the option of moving the port
> >>>>> value to the second cacheline, but it appears that NXP are using
> >>>>> the port value in their NIC drivers for passing in metadata, so
> >>>>> we'd need their agreement on any move (or removal).
> >>>>>
> >>>> If a single driver instance services multiple ports, so the ports
> >>>> are not polled individually, the m->port member will be required to
> >>>> identify the port.
> >>>> In that case, it might also used elsewhere in the ingress path,
> >>>> and thus appropriate having in the first cache line.
> >>
> >> Ok, but these days most of devices have multiple rx queues.
> >> So identify the RX source properly you need not only port, but
> >> port+queue (at least 3 bytes).
> >> But I suppose better to wait for NXP input here.
> >>
> >>> So yes, this needs further discussion with NXP.
> >
> > It seems that this concept might be somewhat similar to the
> > Flow Director information, which already has plenty of bits
> > in the first cache line. You should consider if this can be
> > somehow folded into the m->hash union.
>
> I'll tend to agree with Morten.
>
> First, I think that having more than 255 virtual links is possible,
> so increasing the port size to 16 bits is not a bad idea.
>
> I think the port information is more useful for a network stack
> than the queue_id, but it may not be the case for all applications.
> On the other hand, the queue_id (or something else providing the same
> level of information) could led in the flow director structure.
>
> Now, the question is: do we really need the port to be inside the mbuf?
> Indeed, the application can already associate a bulk of packet with a
> port id.
>
> The reason why I'll prefer to keep it is because I'm pretty sure that
> some applications use it. At least in the examples/ directory, we can
> find distributor, dpdk_qat, ipv4_multicast, load_balancer,
> packet_ordering. I did not check these apps in detail, but it makes me
> feel that other external applications could make use of the port field.
Having some applications using it does not mean that there is a good
justification to keep it.
I think the data must be stored inside the mbuf struct only if it
increases the performance of known use cases significantly.
So the questions should be:
- How significant are the use cases?
- What is the performance drop when having the data outside of the struct?
The mbuf space must be kept jealously like a DPDK treasure :)
^ permalink raw reply
* Re: mbuf changes
From: Morten Brørup @ 2016-10-25 13:18 UTC (permalink / raw)
To: Olivier Matz, dev
In-Reply-To: <1fb6b9cd-341c-1aaa-6a9f-56b758af772e@6wind.com>
I support that!
Med venlig hilsen / kind regards
- Morten Brørup
> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Tuesday, October 25, 2016 3:14 PM
> To: Morten Brørup; dev@dpdk.org
> Subject: Re: mbuf changes
>
> Hi,
>
> On 10/24/2016 05:49 PM, Morten Brørup wrote:
> > And here’s something new to think about:
> >
> > m->next already reveals if there are more segments to a packet. Which
> > purpose does m->nb_segs serve that is not already covered by m->next?
> >
>
> I was asking myself the same question some time ago:
> http://dpdk.org/ml/archives/dev/2016-May/039483.html
>
> But it seems nb_segs is useful for PMDs on TX side, to anticipate how
> many descriptors a packet will use in the TX ring. It can also help a
> PMD to check that this packet is supported by the hardware (too many
> segments) without browsing the list.
>
> So finally I think it should be kept in the mbuf. But as suggested by
> Bruce, it could go in the second cache line, since m->next is also
> there, at the condition that this field is set to 1 in mbuf_free().
>
> Regards,
> Olivier
^ permalink raw reply
* Re: mbuf changes
From: Bruce Richardson @ 2016-10-25 13:15 UTC (permalink / raw)
To: Olivier Matz; +Cc: Morten Brørup, Ananyev, Konstantin, Wiles, Keith, dev
In-Reply-To: <39b70924-0e66-478c-36ea-dd18d27ed8a6@6wind.com>
On Tue, Oct 25, 2016 at 03:00:39PM +0200, Olivier Matz wrote:
> Hi,
>
> On 10/25/2016 12:22 PM, Morten Brørup wrote:
> > Comments inline (at the end).
> >
> >> -----Original Message-----
> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev,
> >> Konstantin
> >> Sent: Tuesday, October 25, 2016 12:03 PM
> >> To: Richardson, Bruce; Morten Brørup
> >> Cc: Wiles, Keith; dev@dpdk.org; Olivier Matz
> >> Subject: Re: [dpdk-dev] mbuf changes
> >>
> >> Hi everyone,
> >>
> >>> -----Original Message-----
> >>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
> >>> Sent: Tuesday, October 25, 2016 9:54 AM
> >>> To: Morten Brørup <mb@smartsharesystems.com>
> >>> Cc: Wiles, Keith <keith.wiles@intel.com>; dev@dpdk.org; Olivier Matz
> >>> <olivier.matz@6wind.com>
> >>> Subject: Re: [dpdk-dev] mbuf changes
> >>>
> >>> On Mon, Oct 24, 2016 at 11:47:16PM +0200, Morten Brørup wrote:
> >>>> Comments inline.
> >>>>
> >>>> Med venlig hilsen / kind regards
> >>>> - Morten Brørup
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce
> >>>>> Richardson
> >>>>> Sent: Monday, October 24, 2016 6:26 PM
> >>>>> To: Wiles, Keith
> >>>>> Cc: Morten Brørup; dev@dpdk.org; Olivier Matz
> >>>>> Subject: Re: [dpdk-dev] mbuf changes
> >>>>>
> >>>>> On Mon, Oct 24, 2016 at 04:11:33PM +0000, Wiles, Keith wrote:
> >>>>>>
> >>>>>>> On Oct 24, 2016, at 10:49 AM, Morten Brørup
> >>>>>>> <mb@smartsharesystems.com>
> >>>>> wrote:
> >>>>>>>
> >>>>>>> 2.
> >>>>>>>
> >>>>>>> There seemed to be consensus that the size of m->refcnt
> >> should
> >>>>>>> match
> >>>>> the size of m->port because a packet could be duplicated on all
> >>>>> physical ports for L3 multicast and L2 flooding.
> >>>>>>>
> >>>>>>> Furthermore, although a single physical machine (i.e. a
> >> single
> >>>>>>> server)
> >>>>> with 255 physical ports probably doesn’t exist, it might contain
> >>>>> more than
> >>>>> 255 virtual machines with a virtual port each, so it makes sense
> >>>>> extending these mbuf fields from 8 to 16 bits.
> >>>>>>
> >>>>>> I thought we also talked about removing the m->port from the
> >>>>>> mbuf as it
> >>>>> is not really needed.
> >>>>>>
> >>>>> Yes, this was mentioned, and also the option of moving the port
> >>>>> value to the second cacheline, but it appears that NXP are using
> >>>>> the port value in their NIC drivers for passing in metadata, so
> >>>>> we'd need their agreement on any move (or removal).
> >>>>>
> >>>> If a single driver instance services multiple ports, so the ports
> >>>> are not polled individually, the m->port member will be required to
> >>>> identify
> >>> the port. In that case, it might also used elsewhere in the ingress
> >> path, and thus appropriate having in the first cache line.
> >>
> >> Ok, but these days most of devices have multiple rx queues.
> >> So identify the RX source properly you need not only port, but
> >> port+queue (at least 3 bytes).
> >> But I suppose better to wait for NXP input here.
> >>
> >>> So yes, this needs
> >>> further discussion with NXP.
> >
> > It seems that this concept might be somewhat similar to the Flow Director information, which already has plenty of bits in the first cache line. You should consider if this can be somehow folded into the m->hash union.
>
>
> I'll tend to agree with Morten.
>
> First, I think that having more than 255 virtual links is possible,
> so increasing the port size to 16 bits is not a bad idea.
>
> I think the port information is more useful for a network stack
> than the queue_id, but it may not be the case for all applications.
> On the other hand, the queue_id (or something else providing the same
> level of information) could led in the flow director structure.
>
> Now, the question is: do we really need the port to be inside the mbuf?
> Indeed, the application can already associate a bulk of packet with a
> port id.
>
> The reason why I'll prefer to keep it is because I'm pretty sure that
> some applications use it. At least in the examples/ directory, we can
> find distributor, dpdk_qat, ipv4_multicast, load_balancer,
> packet_ordering. I did not check these apps in detail, but it makes me
> feel that other external applications could make use of the port field.
>
Yes, judging from the reaction at Userspace and here on list, it appears
that port needs to be kept in the mbuf, and since it's present it should
be filled in by the drivers on RX, so it needs to stay where it is, I
think.
/Bruce
^ permalink raw reply
* Re: mbuf changes
From: Olivier Matz @ 2016-10-25 13:14 UTC (permalink / raw)
To: Morten Brørup, dev
In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC359EA8B1@smartserver.smartshare.dk>
Hi,
On 10/24/2016 05:49 PM, Morten Brørup wrote:
> And here’s something new to think about:
>
> m->next already reveals if there are more segments to a packet. Which
> purpose does m->nb_segs serve that is not already covered by m->next?
>
I was asking myself the same question some time ago:
http://dpdk.org/ml/archives/dev/2016-May/039483.html
But it seems nb_segs is useful for PMDs on TX side, to
anticipate how many descriptors a packet will use in the
TX ring. It can also help a PMD to check that this packet
is supported by the hardware (too many segments) without
browsing the list.
So finally I think it should be kept in the mbuf. But as
suggested by Bruce, it could go in the second cache line,
since m->next is also there, at the condition that this
field is set to 1 in mbuf_free().
Regards,
Olivier
^ permalink raw reply
* Re: mbuf changes
From: Morten Brørup @ 2016-10-25 13:13 UTC (permalink / raw)
To: Olivier Matz, Bruce Richardson
Cc: Adrien Mazarguil, Wiles, Keith, dev, Oleg Kuporosov
In-Reply-To: <4427b23b-e9c0-4e43-9c28-fb7e56da745c@6wind.com>
Comments inline.
Med venlig hilsen / kind regards
- Morten Brørup
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
> Sent: Tuesday, October 25, 2016 2:49 PM
> To: Bruce Richardson; Morten Brørup
> Cc: Adrien Mazarguil; Wiles, Keith; dev@dpdk.org; Oleg Kuporosov
> Subject: Re: [dpdk-dev] mbuf changes
>
>
>
> On 10/25/2016 02:45 PM, Bruce Richardson wrote:
> > On Tue, Oct 25, 2016 at 02:33:55PM +0200, Morten Brørup wrote:
> >> Comments at the end.
> >>
> >> Med venlig hilsen / kind regards
> >> - Morten Brørup
> >>
> >>> -----Original Message-----
> >>> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> >>> Sent: Tuesday, October 25, 2016 2:20 PM
> >>> To: Morten Brørup
> >>> Cc: Adrien Mazarguil; Wiles, Keith; dev@dpdk.org; Olivier Matz;
> Oleg
> >>> Kuporosov
> >>> Subject: Re: [dpdk-dev] mbuf changes
> >>>
> >>> On Tue, Oct 25, 2016 at 02:16:29PM +0200, Morten Brørup wrote:
> >>>> Comments inline.
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce
> >>>>> Richardson
> >>>>> Sent: Tuesday, October 25, 2016 1:14 PM
> >>>>> To: Adrien Mazarguil
> >>>>> Cc: Morten Brørup; Wiles, Keith; dev@dpdk.org; Olivier Matz; Oleg
> >>>>> Kuporosov
> >>>>> Subject: Re: [dpdk-dev] mbuf changes
> >>>>>
> >>>>> On Tue, Oct 25, 2016 at 01:04:44PM +0200, Adrien Mazarguil wrote:
> >>>>>> On Tue, Oct 25, 2016 at 12:11:04PM +0200, Morten Brørup wrote:
> >>>>>>> Comments inline.
> >>>>>>>
> >>>>>>> Med venlig hilsen / kind regards
> >>>>>>> - Morten Brørup
> >>>>>>>
> >>>>>>>
> >>>>>>>> -----Original Message-----
> >>>>>>>> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> >>>>>>>> Sent: Tuesday, October 25, 2016 11:39 AM
> >>>>>>>> To: Bruce Richardson
> >>>>>>>> Cc: Wiles, Keith; Morten Brørup; dev@dpdk.org; Olivier Matz;
> >>>>>>>> Oleg Kuporosov
> >>>>>>>> Subject: Re: [dpdk-dev] mbuf changes
> >>>>>>>>
> >>>>>>>> On Mon, Oct 24, 2016 at 05:25:38PM +0100, Bruce Richardson
> >>> wrote:
> >>>>>>>>> On Mon, Oct 24, 2016 at 04:11:33PM +0000, Wiles, Keith
> >>> wrote:
> >>>>>>>> [...]
> >>>>>>>>>>> On Oct 24, 2016, at 10:49 AM, Morten Brørup
> >>>>>>>> <mb@smartsharesystems.com> wrote:
> >>>>>>>> [...]
> >>>>
> >>>>>>>>> One other point I'll mention is that we need to have a
> >>>>>>>>> discussion on how/where to add in a timestamp value into
> >>> the
> >>>>>>>>> mbuf. Personally, I think it can be in a union with the
> >>>>> sequence
> >>>>>>>>> number value, but I also suspect that 32-bits of a
> >>> timestamp
> >>>>>>>>> is not going to be enough for
> >>>>>>>> many.
> >>>>>>>>>
> >>>>>>>>> Thoughts?
> >>>>>>>>
> >>>>>>>> If we consider that timestamp representation should use
> >>>>> nanosecond
> >>>>>>>> granularity, a 32-bit value may likely wrap around too
> >>> quickly
> >>>>>>>> to be useful. We can also assume that applications requesting
> >>>>>>>> timestamps may care more about latency than throughput, Oleg
> >>>>> found
> >>>>>>>> that using the second cache line for this purpose had a
> >>>>> noticeable impact [1].
> >>>>>>>>
> >>>>>>>> [1] http://dpdk.org/ml/archives/dev/2016-October/049237.html
> >>>>>>>
> >>>>>>> I agree with Oleg about the latency vs. throughput importance
> >>>>>>> for
> >>>>> such applications.
> >>>>>>>
> >>>>>>> If you need high resolution timestamps, consider them to be
> >>>>> generated by the NIC RX driver, possibly by the hardware itself
> >>>>> (http://w3new.napatech.com/features/time-precision/hardware-time-
> >>>>> stamp), so the timestamp belongs in the first cache line. And I
> am
> >>>>> proposing that it should have the highest possible accuracy,
> which
> >>>>> makes the value hardware dependent.
> >>>>>>>
> >>>>>>> Furthermore, I am arguing that we leave it up to the
> >>> application
> >>>>>>> to
> >>>>> keep track of the slowly moving bits (i.e. counting whole
> seconds,
> >>>>> hours and calendar date) out of band, so we don't use precious
> >>> space
> >>>>> in the mbuf. The application doesn't need the NIC RX driver's
> fast
> >>>>> path to capture which date (or even which second) a packet was
> >>>>> received. Yes, it adds complexity to the application, but we
> can't
> >>>>> set aside 64 bit for a generic timestamp. Or as a weird tradeoff:
> >>>>> Put the fast moving 32 bit in the first cache line and the slow
> >>>>> moving 32 bit in the second cache line, as a placeholder for the
> >>> application to fill out if needed.
> >>>>> Yes, it means that the application needs to check the time and
> >>>>> update its variable holding the slow moving time once every
> second
> >>>>> or so; but that should be doable without significant effort.
> >>>>>>
> >>>>>> That's a good point, however without a 64 bit value, elapsed
> time
> >>>>>> between two arbitrary mbufs cannot be measured reliably due to
> >>> not
> >>>>>> enough context, one way or another the low resolution value is
> >>>>>> also
> >>>>> needed.
> >>>>>>
> >>>>>> Obviously latency-sensitive applications are unlikely to perform
> >>>>>> lengthy buffering and require this but I'm not sure about all
> the
> >>>>>> possible use-cases. Considering many NICs expose 64 bit
> >>> timestaps,
> >>>>>> I suggest we do not truncate them.
> >>>>>>
> >>>>>> I'm not a fan of the weird tradeoff either, PMDs will be tempted
> >>>>>> to fill the extra 32 bits whenever they can and negate the
> >>>>>> performance improvement of the first cache line.
> >>>>>
> >>>>> I would tend to agree, and I don't really see any convenient way
> >>>>> to avoid putting in a 64-bit field for the timestamp in cache-
> line 0.
> >>>>> If we are ok with having this overlap/partially overlap with
> >>>>> sequence number, it will use up an extra 4B of storage in that
> >>> cacheline.
> >>>>
> >>>> I agree about the lack of convenience! And Adrien certainly has a
> >>> point about PMD temptations.
> >>>>
> >>>> However, I still don't think that a NICs ability to date-stamp a
> >>> packet is sufficient reason to put a date-stamp in cache line 0 of
> >>> the mbuf. Storing only the fast moving 32 bit in cache line 0 seems
> >>> like a good compromise to me.
> >>>>
> >>>> Maybe you can find just one more byte, so it can hold 17 minutes
> >>>> with nanosecond resolution. (I'm joking!)
> >>>>
> >>>> Please don't sacrifice the sequence number for the
> >>>> seconds/hours/days
> >>> part a timestamp. Maybe it could be configurable to use a 32 bit or
> >>> 64 bit timestamp.
> >>>>
> >>> Do you see both timestamp and sequence numbers being used together?
> >>> I would have thought that apps would either use one or the other?
> >>> However, your suggestion is workable in any case, to allow the
> >>> sequence number to overlap just the high 32 bits of the timestamp,
> >>> rather than the low.
> >>
> >> In our case, I can foresee sequence numbers used for packet
> processing and timestamps for timing analysis (and possibly for packet
> capturing, when being used). For timing analysis, we don’t need long
> durations, e.g. 4 seconds with 32 bit nanosecond resolution suffices.
> And for packet capturing we are perfectly capable of adding the slowly
> moving 32 bit of the timestamp to our output data stream without
> fetching it from the mbuf.
> >>
>
> We should keep in mind that today we have the seqn field but it is not
> used by any PMD. In case it is implemented, would it be a per-queue
> sequence number? Is it useful from an application view?
>
> This field is only used by the librte_reorder library, and in my
> opinion, we should consider moving it in the second cache line since it
> is not filled by the PMD.
Our DPDK project is not yet sufficiently progressed to be able to provide quality feedback about this.
> > For the 32-bit timestamp case, it might be useful to have a
> > right-shift value passed in to the ethdev driver. If we assume a NIC
> > with nanosecond resolution, (or TSC value with resolution of that
> > order of magnitude), then the app can choose to have 1 ns resolution
> > with 4 second wraparound, or alternatively 4ns resolution with 16
> > second wraparound, or even microsecond resolution with wrap around of
> over an hour.
> > The cost is obviously just a shift op in the driver code per packet -
> > hopefully with multiple packets done at a time using vector
> operations.
>
>
> About the timestamp, we can manage to find 64 bits in the first cache
> line, without sacrifying any field we have today. The question is more
> for the fields we may want to add later.
>
> To answer to the question of the size of the timestamp, the first
> question is to know what is the precision required for the applications
> using it?
In our application, microsecond resolution probably suffices, although we do use nanoseconds today. And a wrapping counter is fine for us, so we don’t need the slow moving bits.
For dedicated packet capture applications, please refer to e.g. Napatech's reasoning: http://w3new.napatech.com/features/time-precision/hardware-time-stamp
> I don't quite like the idea of splitting the timestamp in the 2 cache
> lines, I think it would not be easy to use.
>
>
> Olivier
^ permalink raw reply
* Re: mbuf changes
From: Ramia, Kannan Babu @ 2016-10-25 13:04 UTC (permalink / raw)
To: Olivier Matz, Morten Brørup, Ananyev, Konstantin,
Richardson, Bruce
Cc: Wiles, Keith, dev@dpdk.org
In-Reply-To: <39b70924-0e66-478c-36ea-dd18d27ed8a6@6wind.com>
Port filed is important meta information for the application use like CGNAT vEPC functions etc. I strongly recommend to keep the field in mind meta.
Sent from my ASUS
-------- Original Message --------
From:Olivier Matz
Sent:Tue, 25 Oct 2016 18:31:36 +0530
To:Morten Brørup ,"Ananyev, Konstantin" ,"Richardson, Bruce"
Cc:"Wiles, Keith" ,dev@dpdk.org
Subject:Re: [dpdk-dev] mbuf changes
Hi,
On 10/25/2016 12:22 PM, Morten Brørup wrote:
> Comments inline (at the end).
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev,
>> Konstantin
>> Sent: Tuesday, October 25, 2016 12:03 PM
>> To: Richardson, Bruce; Morten Brørup
>> Cc: Wiles, Keith; dev@dpdk.org; Olivier Matz
>> Subject: Re: [dpdk-dev] mbuf changes
>>
>> Hi everyone,
>>
>>> -----Original Message-----
>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
>>> Sent: Tuesday, October 25, 2016 9:54 AM
>>> To: Morten Brørup <mb@smartsharesystems.com>
>>> Cc: Wiles, Keith <keith.wiles@intel.com>; dev@dpdk.org; Olivier Matz
>>> <olivier.matz@6wind.com>
>>> Subject: Re: [dpdk-dev] mbuf changes
>>>
>>> On Mon, Oct 24, 2016 at 11:47:16PM +0200, Morten Brørup wrote:
>>>> Comments inline.
>>>>
>>>> Med venlig hilsen / kind regards
>>>> - Morten Brørup
>>>>
>>>>> -----Original Message-----
>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce
>>>>> Richardson
>>>>> Sent: Monday, October 24, 2016 6:26 PM
>>>>> To: Wiles, Keith
>>>>> Cc: Morten Brørup; dev@dpdk.org; Olivier Matz
>>>>> Subject: Re: [dpdk-dev] mbuf changes
>>>>>
>>>>> On Mon, Oct 24, 2016 at 04:11:33PM +0000, Wiles, Keith wrote:
>>>>>>
>>>>>>> On Oct 24, 2016, at 10:49 AM, Morten Brørup
>>>>>>> <mb@smartsharesystems.com>
>>>>> wrote:
>>>>>>>
>>>>>>> 2.
>>>>>>>
>>>>>>> There seemed to be consensus that the size of m->refcnt
>> should
>>>>>>> match
>>>>> the size of m->port because a packet could be duplicated on all
>>>>> physical ports for L3 multicast and L2 flooding.
>>>>>>>
>>>>>>> Furthermore, although a single physical machine (i.e. a
>> single
>>>>>>> server)
>>>>> with 255 physical ports probably doesn’t exist, it might contain
>>>>> more than
>>>>> 255 virtual machines with a virtual port each, so it makes sense
>>>>> extending these mbuf fields from 8 to 16 bits.
>>>>>>
>>>>>> I thought we also talked about removing the m->port from the
>>>>>> mbuf as it
>>>>> is not really needed.
>>>>>>
>>>>> Yes, this was mentioned, and also the option of moving the port
>>>>> value to the second cacheline, but it appears that NXP are using
>>>>> the port value in their NIC drivers for passing in metadata, so
>>>>> we'd need their agreement on any move (or removal).
>>>>>
>>>> If a single driver instance services multiple ports, so the ports
>>>> are not polled individually, the m->port member will be required to
>>>> identify
>>> the port. In that case, it might also used elsewhere in the ingress
>> path, and thus appropriate having in the first cache line.
>>
>> Ok, but these days most of devices have multiple rx queues.
>> So identify the RX source properly you need not only port, but
>> port+queue (at least 3 bytes).
>> But I suppose better to wait for NXP input here.
>>
>>> So yes, this needs
>>> further discussion with NXP.
>
> It seems that this concept might be somewhat similar to the Flow Director information, which already has plenty of bits in the first cache line. You should consider if this can be somehow folded into the m->hash union.
I'll tend to agree with Morten.
First, I think that having more than 255 virtual links is possible,
so increasing the port size to 16 bits is not a bad idea.
I think the port information is more useful for a network stack
than the queue_id, but it may not be the case for all applications.
On the other hand, the queue_id (or something else providing the same
level of information) could led in the flow director structure.
Now, the question is: do we really need the port to be inside the mbuf?
Indeed, the application can already associate a bulk of packet with a
port id.
The reason why I'll prefer to keep it is because I'm pretty sure that
some applications use it. At least in the examples/ directory, we can
find distributor, dpdk_qat, ipv4_multicast, load_balancer,
packet_ordering. I did not check these apps in detail, but it makes me
feel that other external applications could make use of the port field.
Regards,
Olivier
^ permalink raw reply
* Re: mbuf changes
From: Olivier Matz @ 2016-10-25 13:00 UTC (permalink / raw)
To: Morten Brørup, Ananyev, Konstantin, Richardson, Bruce
Cc: Wiles, Keith, dev
In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC359EA8B8@smartserver.smartshare.dk>
Hi,
On 10/25/2016 12:22 PM, Morten Brørup wrote:
> Comments inline (at the end).
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev,
>> Konstantin
>> Sent: Tuesday, October 25, 2016 12:03 PM
>> To: Richardson, Bruce; Morten Brørup
>> Cc: Wiles, Keith; dev@dpdk.org; Olivier Matz
>> Subject: Re: [dpdk-dev] mbuf changes
>>
>> Hi everyone,
>>
>>> -----Original Message-----
>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
>>> Sent: Tuesday, October 25, 2016 9:54 AM
>>> To: Morten Brørup <mb@smartsharesystems.com>
>>> Cc: Wiles, Keith <keith.wiles@intel.com>; dev@dpdk.org; Olivier Matz
>>> <olivier.matz@6wind.com>
>>> Subject: Re: [dpdk-dev] mbuf changes
>>>
>>> On Mon, Oct 24, 2016 at 11:47:16PM +0200, Morten Brørup wrote:
>>>> Comments inline.
>>>>
>>>> Med venlig hilsen / kind regards
>>>> - Morten Brørup
>>>>
>>>>> -----Original Message-----
>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce
>>>>> Richardson
>>>>> Sent: Monday, October 24, 2016 6:26 PM
>>>>> To: Wiles, Keith
>>>>> Cc: Morten Brørup; dev@dpdk.org; Olivier Matz
>>>>> Subject: Re: [dpdk-dev] mbuf changes
>>>>>
>>>>> On Mon, Oct 24, 2016 at 04:11:33PM +0000, Wiles, Keith wrote:
>>>>>>
>>>>>>> On Oct 24, 2016, at 10:49 AM, Morten Brørup
>>>>>>> <mb@smartsharesystems.com>
>>>>> wrote:
>>>>>>>
>>>>>>> 2.
>>>>>>>
>>>>>>> There seemed to be consensus that the size of m->refcnt
>> should
>>>>>>> match
>>>>> the size of m->port because a packet could be duplicated on all
>>>>> physical ports for L3 multicast and L2 flooding.
>>>>>>>
>>>>>>> Furthermore, although a single physical machine (i.e. a
>> single
>>>>>>> server)
>>>>> with 255 physical ports probably doesn’t exist, it might contain
>>>>> more than
>>>>> 255 virtual machines with a virtual port each, so it makes sense
>>>>> extending these mbuf fields from 8 to 16 bits.
>>>>>>
>>>>>> I thought we also talked about removing the m->port from the
>>>>>> mbuf as it
>>>>> is not really needed.
>>>>>>
>>>>> Yes, this was mentioned, and also the option of moving the port
>>>>> value to the second cacheline, but it appears that NXP are using
>>>>> the port value in their NIC drivers for passing in metadata, so
>>>>> we'd need their agreement on any move (or removal).
>>>>>
>>>> If a single driver instance services multiple ports, so the ports
>>>> are not polled individually, the m->port member will be required to
>>>> identify
>>> the port. In that case, it might also used elsewhere in the ingress
>> path, and thus appropriate having in the first cache line.
>>
>> Ok, but these days most of devices have multiple rx queues.
>> So identify the RX source properly you need not only port, but
>> port+queue (at least 3 bytes).
>> But I suppose better to wait for NXP input here.
>>
>>> So yes, this needs
>>> further discussion with NXP.
>
> It seems that this concept might be somewhat similar to the Flow Director information, which already has plenty of bits in the first cache line. You should consider if this can be somehow folded into the m->hash union.
I'll tend to agree with Morten.
First, I think that having more than 255 virtual links is possible,
so increasing the port size to 16 bits is not a bad idea.
I think the port information is more useful for a network stack
than the queue_id, but it may not be the case for all applications.
On the other hand, the queue_id (or something else providing the same
level of information) could led in the flow director structure.
Now, the question is: do we really need the port to be inside the mbuf?
Indeed, the application can already associate a bulk of packet with a
port id.
The reason why I'll prefer to keep it is because I'm pretty sure that
some applications use it. At least in the examples/ directory, we can
find distributor, dpdk_qat, ipv4_multicast, load_balancer,
packet_ordering. I did not check these apps in detail, but it makes me
feel that other external applications could make use of the port field.
Regards,
Olivier
^ permalink raw reply
* Re: [PATCH] Revert "bonding: use existing enslaved device queues"
From: Bruce Richardson @ 2016-10-25 12:57 UTC (permalink / raw)
To: Declan Doherty
Cc: Jan Blunck, Eric Kinzie, Ilya Maximets, dev, Heetae Ahn,
Yuanhan Liu, Bernard Iremonger, stable, Thomas Monjalon
In-Reply-To: <ac86be29-61b2-11b3-5140-6004fec95ce2@intel.com>
On Mon, Oct 24, 2016 at 04:07:17PM +0100, Declan Doherty wrote:
> On 24/10/16 15:51, Jan Blunck wrote:
> > On Mon, Oct 24, 2016 at 7:02 AM, Declan Doherty
> > <declan.doherty@intel.com> wrote:
> > > On 14/10/16 00:37, Eric Kinzie wrote:
> > > >
> > > > On Wed Oct 12 16:24:21 +0100 2016, Bruce Richardson wrote:
> > > > >
> > > > > On Wed, Oct 12, 2016 at 04:24:54PM +0300, Ilya Maximets wrote:
> > > > > >
> > > > > > On 07.10.2016 05:02, Eric Kinzie wrote:
> > > > > > >
> > > > > > > On Wed Sep 07 15:28:10 +0300 2016, Ilya Maximets wrote:
> > > > > > > >
> > > > > > > > This reverts commit 5b7bb2bda5519b7800f814df64d4e015282140e5.
> > > > > > > >
> > > > > > > > It is necessary to reconfigure all queues every time because
> > > > > > > > configuration
> > > > > > > > can be changed.
> > > > > > > >
> > > > > > > > For example, if we're reconfiguring bonding device with new memory
> > > > > > > > pool,
> > > > > > > > already configured queues will still use the old one. And if the old
> > > > > > > > mempool be freed, application likely will panic in attempt to use
> > > > > > > > freed mempool.
> > > > > > > >
> > > > > > > > This happens when we use the bonding device with OVS 2.6 while MTU
> > > > > > > > reconfiguration:
> > > > > > > >
> > > > > > > > PANIC in rte_mempool_get_ops():
> > > > > > > > assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)"
> > > > > > > > failed
> > > > > > > >
> > > > > > > > Cc: <stable@dpdk.org>
> > > > > > > > Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> > > > > > > > ---
> > > > > > > > drivers/net/bonding/rte_eth_bond_pmd.c | 10 ++--------
> > > > > > > > 1 file changed, 2 insertions(+), 8 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c
> > > > > > > > b/drivers/net/bonding/rte_eth_bond_pmd.c
> > > > > > > > index b20a272..eb5b6d1 100644
> > > > > > > > --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> > > > > > > > +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> > > > > > > > @@ -1305,8 +1305,6 @@ slave_configure(struct rte_eth_dev
> > > > > > > > *bonded_eth_dev,
> > > > > > > > struct bond_rx_queue *bd_rx_q;
> > > > > > > > struct bond_tx_queue *bd_tx_q;
> > > > > > > >
> > > > > > > > - uint16_t old_nb_tx_queues = slave_eth_dev->data->nb_tx_queues;
> > > > > > > > - uint16_t old_nb_rx_queues = slave_eth_dev->data->nb_rx_queues;
> > > > > > > > int errval;
> > > > > > > > uint16_t q_id;
> > > > > > > >
> > > > > > > > @@ -1347,9 +1345,7 @@ slave_configure(struct rte_eth_dev
> > > > > > > > *bonded_eth_dev,
> > > > > > > > }
> > > > > > > >
> > > > > > > > /* Setup Rx Queues */
> > > > > > > > - /* Use existing queues, if any */
> > > > > > > > - for (q_id = old_nb_rx_queues;
> > > > > > > > - q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
> > > > > > > > + for (q_id = 0; q_id < bonded_eth_dev->data->nb_rx_queues;
> > > > > > > > q_id++) {
> > > > > > > > bd_rx_q = (struct bond_rx_queue
> > > > > > > > *)bonded_eth_dev->data->rx_queues[q_id];
> > > > > > > >
> > > > > > > > errval =
> > > > > > > > rte_eth_rx_queue_setup(slave_eth_dev->data->port_id, q_id,
> > > > > > > > @@ -1365,9 +1361,7 @@ slave_configure(struct rte_eth_dev
> > > > > > > > *bonded_eth_dev,
> > > > > > > > }
> > > > > > > >
> > > > > > > > /* Setup Tx Queues */
> > > > > > > > - /* Use existing queues, if any */
> > > > > > > > - for (q_id = old_nb_tx_queues;
> > > > > > > > - q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
> > > > > > > > + for (q_id = 0; q_id < bonded_eth_dev->data->nb_tx_queues;
> > > > > > > > q_id++) {
> > > > > > > > bd_tx_q = (struct bond_tx_queue
> > > > > > > > *)bonded_eth_dev->data->tx_queues[q_id];
> > > > > > > >
> > > > > > > > errval =
> > > > > > > > rte_eth_tx_queue_setup(slave_eth_dev->data->port_id, q_id,
> > > > > > > > --
> > > > > > > > 2.7.4
> > > > > > > >
> > > > > > >
> > > > > > > NAK
> > > > > > >
> > > > > > > There are still some users of this code. Let's give them a chance to
> > > > > > > comment before removing it.
> > > > > >
> > > > > >
> > > > > > Hi Eric,
> > > > > >
> > > > > > Are these users in CC-list? If not, could you, please, add them?
> > > > > > This patch awaits in mail-list already more than a month. I think, it's
> > > > > > enough
> > > > > > time period for all who wants to say something. Patch fixes a real bug
> > > > > > that
> > > > > > prevent using of DPDK bonding in all applications that reconfigures
> > > > > > devices
> > > > > > in runtime including OVS.
> > > > > >
> > > > > Agreed.
> > > > >
> > > > > Eric, does reverting this patch cause you problems directly, or is your
> > > > > concern
> > > > > just with regards to potential impact to others?
> > > > >
> > > > > Thanks,
> > > > > /Bruce
> > > >
> > > >
> > > > This won't impact me directly. The users are CCed (different thread)
> > > > and I haven't seen any comment, so I no longer have any objection to
> > > > reverting this change.
> > > >
> > > > Eric
> > > >
> > >
> > > As there has been no further objections and this reinstates the original
> > > expected behavior of the bonding driver. I'm re-ack'ing for inclusion in
> > > release.
> > >
> > > Acked-by: Declan Doherty <declan.doherty@intel.com>
> >
> > Ok, I can revert the revert for us.
> >
> > Do I read this correctly that you are not interested in fixing this properly?!
> >
> > Thanks,
> > Jan
> >
>
> Jan, sorry I missed the replies from last week due to the way my mail client
> was filtering the conversation. Let me have another look at this and I'll
> come back to the list.
>
> Thanks
> Declan
While this patch has already been applied to dpdk-next-net tree, it
appears that there is still some ongoing discussion about it. I'm
therefore planning to pull it back out of the tree for rc2. If a
subsequent consensus is reached we can see about including it in rc3.
Declan, as maintainer, does this seem reasonable to you.
Regards,
/Bruce
^ permalink raw reply
* Re: [PATCH v2 1/2] net/i40e: fix link status change interrupt
From: Bruce Richardson @ 2016-10-25 12:53 UTC (permalink / raw)
To: Qiming Yang; +Cc: dev, jingjing.wu, helin.zhang
In-Reply-To: <1477369146-63317-1-git-send-email-qiming.yang@intel.com>
On Tue, Oct 25, 2016 at 12:19:05PM +0800, Qiming Yang wrote:
> Previously, link status interrupt in i40e is achieved by checking
> LINK_STAT_CHANGE_MASK in PFINT_ICR0 register which is provided only
> for diagnostic use. Instead, drivers need to get the link status
> change notification by using LSE (Link Status Event).
>
> This patch enables LSE and calls LSC callback when the event is
> received. This patch also removes the processing on
> LINK_STAT_CHANGE_MASK.
>
> Fixes: 4861cde46116 ("i40e: new poll mode driver")
>
> Signed-off-by: Qiming Yang <qiming.yang@intel.com>
> ---
Thanks for the V2. Unfortunately, this conflicts with some other changes
to the driver, making the patch not apply cleanly, and a manual apply I
tried did not compile successfully. Can you please do a V3 rebased on
top of dpdk-next-net/rel_16_11.
Thanks,
/Bruce
^ permalink raw reply
* Re: mbuf changes
From: Morten Brørup @ 2016-10-25 12:49 UTC (permalink / raw)
To: Shreyansh Jain, Bruce Richardson; +Cc: Wiles, Keith, dev, Olivier Matz
In-Reply-To: <f2d7f77f-dc67-fce1-183a-7b57a60e4342@nxp.com>
Comments inline.
Med venlig hilsen / kind regards
- Morten Brørup
> -----Original Message-----
> From: Shreyansh Jain [mailto:shreyansh.jain@nxp.com]
> Sent: Tuesday, October 25, 2016 1:54 PM
> To: Bruce Richardson
> Cc: Wiles, Keith; Morten Brørup; dev@dpdk.org; Olivier Matz
> Subject: Re: mbuf changes
>
> On Monday 24 October 2016 09:55 PM, Bruce Richardson wrote:
> > On Mon, Oct 24, 2016 at 04:11:33PM +0000, Wiles, Keith wrote:
> >>
> >>> On Oct 24, 2016, at 10:49 AM, Morten Brørup
> <mb@smartsharesystems.com> wrote:
> >>>
> >>> First of all: Thanks for a great DPDK Userspace 2016!
> >>>
> >>>
> >>>
> >>> Continuing the Userspace discussion about Olivier Matz’s proposed
> mbuf changes...
> >
> > Thanks for keeping the discussion going!
> >>>
> >>>
> >>>
> >>> 1.
> >>>
> >>> Stephen Hemminger had a noteworthy general comment about keeping
> metadata for the NIC in the appropriate section of the mbuf: Metadata
> generated by the NIC’s RX handler belongs in the first cache line, and
> metadata required by the NIC’s TX handler belongs in the second cache
> line. This also means that touching the second cache line on ingress
> should be avoided if possible; and Bruce Richardson mentioned that for
> this reason m->next was zeroed on free().
> >>>
> > Thinking about it, I suspect there are more fields we can reset on
> free
> > to save time on alloc. Refcnt, as discussed below is one of them, but
> so
> > too could be the nb_segs field and possibly others.
> >
> >>>
> >>>
> >>> 2.
> >>>
> >>> There seemed to be consensus that the size of m->refcnt should
> match the size of m->port because a packet could be duplicated on all
> physical ports for L3 multicast and L2 flooding.
> >>>
> >>> Furthermore, although a single physical machine (i.e. a single
> server) with 255 physical ports probably doesn’t exist, it might
> contain more than 255 virtual machines with a virtual port each, so it
> makes sense extending these mbuf fields from 8 to 16 bits.
> >>
> >> I thought we also talked about removing the m->port from the mbuf as
> it is not really needed.
> >>
> > Yes, this was mentioned, and also the option of moving the port value
> to
> > the second cacheline, but it appears that NXP are using the port
> value
> > in their NIC drivers for passing in metadata, so we'd need their
> > agreement on any move (or removal).
>
> I am not sure where NXP's NIC came into picture on this, but now that
> it
> is highlighted, this field is required for libevent implementation [1].
>
> A scheduler sending an event, which can be a packet, would only have
> information of a flow_id. From this matching it back to a port, without
> mbuf->port, would be very difficult (costly). There may be way around
> this but at least in current proposal I think port would be important
> to
> have - even if in second cache line.
>
Since it's a natural part of the RX handler, it rightfully belongs in the first cache line.
Admitting that I haven't read the referenced proposal in detail, I would like to reiterate that perhaps the Flow Director fields in the mbuf could be used instead of the port field.
> But, off the top of my head, as of now it is not being used for any
> specific purpose in NXP's PMD implementation.
>
> Even the SoC patches don't necessarily rely on it except using it
> because it is available.
>
> @Bruce: where did you get the NXP context here from?
>
> [1] http://dpdk.org/ml/archives/dev/2016-October/048592.html
>
> >
> >>>
> >>>
> >>>
> >>> 3.
> >>>
> >>> Someone (Bruce Richardson?) suggested moving m->refcnt and m->port
> to the second cache line, which then generated questions from the
> audience about the real life purpose of m->port, and if m->port could
> be removed from the mbuf structure.
> >>>
> >>>
> >>>
> >>> 4.
> >>>
> >>> I suggested using offset -1 for m->refcnt, so m->refcnt becomes 0
> on first allocation. This is based on the assumption that other mbuf
> fields must be zeroed at alloc()/free() anyway, so zeroing m->refcnt is
> cheaper than setting it to 1.
> >>>
> >>> Furthermore (regardless of m->refcnt offset), I suggested that it
> is not required to modify m->refcnt when allocating and freeing the
> mbuf, thus saving one write operation on both alloc() and free().
> However, this assumes that m->refcnt debugging, e.g. underrun
> detection, is not required.
> >
> > I don't think it really matters what sentinal value is used for the
> > refcnt because it can't be blindly assigned on free like other
> fields.
> > However, I think 0 as first reference value becomes more awkward
> > than 1, because we need to deal with underflow. Consider the
> situation
> > where we have two references to the mbuf, so refcnt is 1, and both
> are
> > freed at the same time. Since the refcnt is not-zero, then both cores
> > will do an atomic decrement simultaneously giving a refcnt of -1. We
> can
> > then set this back to zero before freeing, however, I'd still prefer
> to
> > have refcnt be an accurate value so that it always stays positive,
> and
> > we can still set it to "one" on free to avoid having to set on alloc.
> >
> > Also, if we set refcnt on free rather than alloc, it does set itself
> up
> > as a good candidate for moving to the second cacheline. Fast-path
> > processing does not normally update the value.
> >
> >>>
> >>>
> >>>
> >>> 5.
> >>>
> >>> And here’s something new to think about:
> >>>
> >>> m->next already reveals if there are more segments to a packet.
> Which purpose does m->nb_segs serve that is not already covered by m-
> >next?
> >
> > It is duplicate info, but nb_segs can be used to check the validity
> of
> > the next pointer without having to read the second mbuf cacheline.
> >
> > Whether it's worth having is something I'm happy enough to discuss,
> > though.
> >
> > One other point I'll mention is that we need to have a discussion on
> > how/where to add in a timestamp value into the mbuf. Personally, I
> think
> > it can be in a union with the sequence number value, but I also
> suspect
> > that 32-bits of a timestamp is not going to be enough for many.
> >
> > Thoughts?
> >
> > /Bruce
> >
>
>
> --
> -
> Shreyansh
^ permalink raw reply
* Re: mbuf changes
From: Olivier Matz @ 2016-10-25 12:48 UTC (permalink / raw)
To: Bruce Richardson, Morten Brørup
Cc: Adrien Mazarguil, Wiles, Keith, dev, Oleg Kuporosov
In-Reply-To: <20161025124508.GA59784@bricha3-MOBL3.ger.corp.intel.com>
On 10/25/2016 02:45 PM, Bruce Richardson wrote:
> On Tue, Oct 25, 2016 at 02:33:55PM +0200, Morten Brørup wrote:
>> Comments at the end.
>>
>> Med venlig hilsen / kind regards
>> - Morten Brørup
>>
>>> -----Original Message-----
>>> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
>>> Sent: Tuesday, October 25, 2016 2:20 PM
>>> To: Morten Brørup
>>> Cc: Adrien Mazarguil; Wiles, Keith; dev@dpdk.org; Olivier Matz; Oleg
>>> Kuporosov
>>> Subject: Re: [dpdk-dev] mbuf changes
>>>
>>> On Tue, Oct 25, 2016 at 02:16:29PM +0200, Morten Brørup wrote:
>>>> Comments inline.
>>>>
>>>>> -----Original Message-----
>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce
>>>>> Richardson
>>>>> Sent: Tuesday, October 25, 2016 1:14 PM
>>>>> To: Adrien Mazarguil
>>>>> Cc: Morten Brørup; Wiles, Keith; dev@dpdk.org; Olivier Matz; Oleg
>>>>> Kuporosov
>>>>> Subject: Re: [dpdk-dev] mbuf changes
>>>>>
>>>>> On Tue, Oct 25, 2016 at 01:04:44PM +0200, Adrien Mazarguil wrote:
>>>>>> On Tue, Oct 25, 2016 at 12:11:04PM +0200, Morten Brørup wrote:
>>>>>>> Comments inline.
>>>>>>>
>>>>>>> Med venlig hilsen / kind regards
>>>>>>> - Morten Brørup
>>>>>>>
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
>>>>>>>> Sent: Tuesday, October 25, 2016 11:39 AM
>>>>>>>> To: Bruce Richardson
>>>>>>>> Cc: Wiles, Keith; Morten Brørup; dev@dpdk.org; Olivier Matz;
>>>>>>>> Oleg Kuporosov
>>>>>>>> Subject: Re: [dpdk-dev] mbuf changes
>>>>>>>>
>>>>>>>> On Mon, Oct 24, 2016 at 05:25:38PM +0100, Bruce Richardson
>>> wrote:
>>>>>>>>> On Mon, Oct 24, 2016 at 04:11:33PM +0000, Wiles, Keith
>>> wrote:
>>>>>>>> [...]
>>>>>>>>>>> On Oct 24, 2016, at 10:49 AM, Morten Brørup
>>>>>>>> <mb@smartsharesystems.com> wrote:
>>>>>>>> [...]
>>>>
>>>>>>>>> One other point I'll mention is that we need to have a
>>>>>>>>> discussion on how/where to add in a timestamp value into
>>> the
>>>>>>>>> mbuf. Personally, I think it can be in a union with the
>>>>> sequence
>>>>>>>>> number value, but I also suspect that 32-bits of a
>>> timestamp
>>>>>>>>> is not going to be enough for
>>>>>>>> many.
>>>>>>>>>
>>>>>>>>> Thoughts?
>>>>>>>>
>>>>>>>> If we consider that timestamp representation should use
>>>>> nanosecond
>>>>>>>> granularity, a 32-bit value may likely wrap around too
>>> quickly
>>>>>>>> to be useful. We can also assume that applications requesting
>>>>>>>> timestamps may care more about latency than throughput, Oleg
>>>>> found
>>>>>>>> that using the second cache line for this purpose had a
>>>>> noticeable impact [1].
>>>>>>>>
>>>>>>>> [1] http://dpdk.org/ml/archives/dev/2016-October/049237.html
>>>>>>>
>>>>>>> I agree with Oleg about the latency vs. throughput importance
>>>>>>> for
>>>>> such applications.
>>>>>>>
>>>>>>> If you need high resolution timestamps, consider them to be
>>>>> generated by the NIC RX driver, possibly by the hardware itself
>>>>> (http://w3new.napatech.com/features/time-precision/hardware-time-
>>>>> stamp), so the timestamp belongs in the first cache line. And I am
>>>>> proposing that it should have the highest possible accuracy, which
>>>>> makes the value hardware dependent.
>>>>>>>
>>>>>>> Furthermore, I am arguing that we leave it up to the
>>> application
>>>>>>> to
>>>>> keep track of the slowly moving bits (i.e. counting whole seconds,
>>>>> hours and calendar date) out of band, so we don't use precious
>>> space
>>>>> in the mbuf. The application doesn't need the NIC RX driver's fast
>>>>> path to capture which date (or even which second) a packet was
>>>>> received. Yes, it adds complexity to the application, but we can't
>>>>> set aside 64 bit for a generic timestamp. Or as a weird tradeoff:
>>>>> Put the fast moving 32 bit in the first cache line and the slow
>>>>> moving 32 bit in the second cache line, as a placeholder for the
>>> application to fill out if needed.
>>>>> Yes, it means that the application needs to check the time and
>>>>> update its variable holding the slow moving time once every second
>>>>> or so; but that should be doable without significant effort.
>>>>>>
>>>>>> That's a good point, however without a 64 bit value, elapsed time
>>>>>> between two arbitrary mbufs cannot be measured reliably due to
>>> not
>>>>>> enough context, one way or another the low resolution value is
>>>>>> also
>>>>> needed.
>>>>>>
>>>>>> Obviously latency-sensitive applications are unlikely to perform
>>>>>> lengthy buffering and require this but I'm not sure about all the
>>>>>> possible use-cases. Considering many NICs expose 64 bit
>>> timestaps,
>>>>>> I suggest we do not truncate them.
>>>>>>
>>>>>> I'm not a fan of the weird tradeoff either, PMDs will be tempted
>>>>>> to fill the extra 32 bits whenever they can and negate the
>>>>>> performance improvement of the first cache line.
>>>>>
>>>>> I would tend to agree, and I don't really see any convenient way to
>>>>> avoid putting in a 64-bit field for the timestamp in cache-line 0.
>>>>> If we are ok with having this overlap/partially overlap with
>>>>> sequence number, it will use up an extra 4B of storage in that
>>> cacheline.
>>>>
>>>> I agree about the lack of convenience! And Adrien certainly has a
>>> point about PMD temptations.
>>>>
>>>> However, I still don't think that a NICs ability to date-stamp a
>>> packet is sufficient reason to put a date-stamp in cache line 0 of the
>>> mbuf. Storing only the fast moving 32 bit in cache line 0 seems like a
>>> good compromise to me.
>>>>
>>>> Maybe you can find just one more byte, so it can hold 17 minutes with
>>>> nanosecond resolution. (I'm joking!)
>>>>
>>>> Please don't sacrifice the sequence number for the seconds/hours/days
>>> part a timestamp. Maybe it could be configurable to use a 32 bit or 64
>>> bit timestamp.
>>>>
>>> Do you see both timestamp and sequence numbers being used together? I
>>> would have thought that apps would either use one or the other?
>>> However, your suggestion is workable in any case, to allow the sequence
>>> number to overlap just the high 32 bits of the timestamp, rather than
>>> the low.
>>
>> In our case, I can foresee sequence numbers used for packet processing and timestamps for timing analysis (and possibly for packet capturing, when being used). For timing analysis, we don’t need long durations, e.g. 4 seconds with 32 bit nanosecond resolution suffices. And for packet capturing we are perfectly capable of adding the slowly moving 32 bit of the timestamp to our output data stream without fetching it from the mbuf.
>>
We should keep in mind that today we have the seqn field but it is
not used by any PMD. In case it is implemented, would it be a per-queue
sequence number? Is it useful from an application view?
This field is only used by the librte_reorder library, and in my
opinion, we should consider moving it in the second cache line since
it is not filled by the PMD.
> For the 32-bit timestamp case, it might be useful to have a right-shift
> value passed in to the ethdev driver. If we assume a NIC with nanosecond
> resolution, (or TSC value with resolution of that order of magnitude),
> then the app can choose to have 1 ns resolution with 4 second
> wraparound, or alternatively 4ns resolution with 16 second wraparound,
> or even microsecond resolution with wrap around of over an hour.
> The cost is obviously just a shift op in the driver code per packet -
> hopefully with multiple packets done at a time using vector operations.
About the timestamp, we can manage to find 64 bits in the first cache
line, without sacrifying any field we have today. The question is more
for the fields we may want to add later.
To answer to the question of the size of the timestamp, the first
question is to know what is the precision required for the
applications using it?
I don't quite like the idea of splitting the timestamp in the 2 cache
lines, I think it would not be easy to use.
Olivier
^ permalink raw reply
* Re: mbuf changes
From: Bruce Richardson @ 2016-10-25 12:45 UTC (permalink / raw)
To: Morten Brørup
Cc: Adrien Mazarguil, Wiles, Keith, dev, Olivier Matz, Oleg Kuporosov
In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC359EA8BB@smartserver.smartshare.dk>
On Tue, Oct 25, 2016 at 02:33:55PM +0200, Morten Brørup wrote:
> Comments at the end.
>
> Med venlig hilsen / kind regards
> - Morten Brørup
>
> > -----Original Message-----
> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > Sent: Tuesday, October 25, 2016 2:20 PM
> > To: Morten Brørup
> > Cc: Adrien Mazarguil; Wiles, Keith; dev@dpdk.org; Olivier Matz; Oleg
> > Kuporosov
> > Subject: Re: [dpdk-dev] mbuf changes
> >
> > On Tue, Oct 25, 2016 at 02:16:29PM +0200, Morten Brørup wrote:
> > > Comments inline.
> > >
> > > > -----Original Message-----
> > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce
> > > > Richardson
> > > > Sent: Tuesday, October 25, 2016 1:14 PM
> > > > To: Adrien Mazarguil
> > > > Cc: Morten Brørup; Wiles, Keith; dev@dpdk.org; Olivier Matz; Oleg
> > > > Kuporosov
> > > > Subject: Re: [dpdk-dev] mbuf changes
> > > >
> > > > On Tue, Oct 25, 2016 at 01:04:44PM +0200, Adrien Mazarguil wrote:
> > > > > On Tue, Oct 25, 2016 at 12:11:04PM +0200, Morten Brørup wrote:
> > > > > > Comments inline.
> > > > > >
> > > > > > Med venlig hilsen / kind regards
> > > > > > - Morten Brørup
> > > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > > > > > > Sent: Tuesday, October 25, 2016 11:39 AM
> > > > > > > To: Bruce Richardson
> > > > > > > Cc: Wiles, Keith; Morten Brørup; dev@dpdk.org; Olivier Matz;
> > > > > > > Oleg Kuporosov
> > > > > > > Subject: Re: [dpdk-dev] mbuf changes
> > > > > > >
> > > > > > > On Mon, Oct 24, 2016 at 05:25:38PM +0100, Bruce Richardson
> > wrote:
> > > > > > > > On Mon, Oct 24, 2016 at 04:11:33PM +0000, Wiles, Keith
> > wrote:
> > > > > > > [...]
> > > > > > > > > > On Oct 24, 2016, at 10:49 AM, Morten Brørup
> > > > > > > <mb@smartsharesystems.com> wrote:
> > > > > > > [...]
> > >
> > > > > > > > One other point I'll mention is that we need to have a
> > > > > > > > discussion on how/where to add in a timestamp value into
> > the
> > > > > > > > mbuf. Personally, I think it can be in a union with the
> > > > sequence
> > > > > > > > number value, but I also suspect that 32-bits of a
> > timestamp
> > > > > > > > is not going to be enough for
> > > > > > > many.
> > > > > > > >
> > > > > > > > Thoughts?
> > > > > > >
> > > > > > > If we consider that timestamp representation should use
> > > > nanosecond
> > > > > > > granularity, a 32-bit value may likely wrap around too
> > quickly
> > > > > > > to be useful. We can also assume that applications requesting
> > > > > > > timestamps may care more about latency than throughput, Oleg
> > > > found
> > > > > > > that using the second cache line for this purpose had a
> > > > noticeable impact [1].
> > > > > > >
> > > > > > > [1] http://dpdk.org/ml/archives/dev/2016-October/049237.html
> > > > > >
> > > > > > I agree with Oleg about the latency vs. throughput importance
> > > > > > for
> > > > such applications.
> > > > > >
> > > > > > If you need high resolution timestamps, consider them to be
> > > > generated by the NIC RX driver, possibly by the hardware itself
> > > > (http://w3new.napatech.com/features/time-precision/hardware-time-
> > > > stamp), so the timestamp belongs in the first cache line. And I am
> > > > proposing that it should have the highest possible accuracy, which
> > > > makes the value hardware dependent.
> > > > > >
> > > > > > Furthermore, I am arguing that we leave it up to the
> > application
> > > > > > to
> > > > keep track of the slowly moving bits (i.e. counting whole seconds,
> > > > hours and calendar date) out of band, so we don't use precious
> > space
> > > > in the mbuf. The application doesn't need the NIC RX driver's fast
> > > > path to capture which date (or even which second) a packet was
> > > > received. Yes, it adds complexity to the application, but we can't
> > > > set aside 64 bit for a generic timestamp. Or as a weird tradeoff:
> > > > Put the fast moving 32 bit in the first cache line and the slow
> > > > moving 32 bit in the second cache line, as a placeholder for the
> > application to fill out if needed.
> > > > Yes, it means that the application needs to check the time and
> > > > update its variable holding the slow moving time once every second
> > > > or so; but that should be doable without significant effort.
> > > > >
> > > > > That's a good point, however without a 64 bit value, elapsed time
> > > > > between two arbitrary mbufs cannot be measured reliably due to
> > not
> > > > > enough context, one way or another the low resolution value is
> > > > > also
> > > > needed.
> > > > >
> > > > > Obviously latency-sensitive applications are unlikely to perform
> > > > > lengthy buffering and require this but I'm not sure about all the
> > > > > possible use-cases. Considering many NICs expose 64 bit
> > timestaps,
> > > > > I suggest we do not truncate them.
> > > > >
> > > > > I'm not a fan of the weird tradeoff either, PMDs will be tempted
> > > > > to fill the extra 32 bits whenever they can and negate the
> > > > > performance improvement of the first cache line.
> > > >
> > > > I would tend to agree, and I don't really see any convenient way to
> > > > avoid putting in a 64-bit field for the timestamp in cache-line 0.
> > > > If we are ok with having this overlap/partially overlap with
> > > > sequence number, it will use up an extra 4B of storage in that
> > cacheline.
> > >
> > > I agree about the lack of convenience! And Adrien certainly has a
> > point about PMD temptations.
> > >
> > > However, I still don't think that a NICs ability to date-stamp a
> > packet is sufficient reason to put a date-stamp in cache line 0 of the
> > mbuf. Storing only the fast moving 32 bit in cache line 0 seems like a
> > good compromise to me.
> > >
> > > Maybe you can find just one more byte, so it can hold 17 minutes with
> > > nanosecond resolution. (I'm joking!)
> > >
> > > Please don't sacrifice the sequence number for the seconds/hours/days
> > part a timestamp. Maybe it could be configurable to use a 32 bit or 64
> > bit timestamp.
> > >
> > Do you see both timestamp and sequence numbers being used together? I
> > would have thought that apps would either use one or the other?
> > However, your suggestion is workable in any case, to allow the sequence
> > number to overlap just the high 32 bits of the timestamp, rather than
> > the low.
>
> In our case, I can foresee sequence numbers used for packet processing and timestamps for timing analysis (and possibly for packet capturing, when being used). For timing analysis, we don’t need long durations, e.g. 4 seconds with 32 bit nanosecond resolution suffices. And for packet capturing we are perfectly capable of adding the slowly moving 32 bit of the timestamp to our output data stream without fetching it from the mbuf.
>
For the 32-bit timestamp case, it might be useful to have a right-shift
value passed in to the ethdev driver. If we assume a NIC with nanosecond
resolution, (or TSC value with resolution of that order of magnitude),
then the app can choose to have 1 ns resolution with 4 second
wraparound, or alternatively 4ns resolution with 16 second wraparound,
or even microsecond resolution with wrap around of over an hour.
The cost is obviously just a shift op in the driver code per packet -
hopefully with multiple packets done at a time using vector operations.
/Bruce
^ permalink raw reply
* Re: [PATCH 1/3] mbuf: embedding timestamp into the packet
From: Pattan, Reshma @ 2016-10-25 12:39 UTC (permalink / raw)
To: Oleg Kuporosov, Olivier Matz
Cc: Richardson, Bruce, Ananyev, Konstantin, dev@dpdk.org
In-Reply-To: <AM4PR0501MB2785EE15A56BBE62012CF72CCFD20@AM4PR0501MB2785.eurprd05.prod.outlook.com>
Hi,
> -----Original Message-----
> From: Oleg Kuporosov [mailto:olegk@mellanox.com]
> Sent: Wednesday, October 19, 2016 6:40 PM
> To: Pattan, Reshma <reshma.pattan@intel.com>; Olivier Matz
> <olivier.matz@6wind.com>
> Cc: Richardson, Bruce <bruce.richardson@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH 1/3] mbuf: embedding timestamp into the
> packet
>
> Hello Reshma
>
> > I just read this mail chain, to make every one aware again, I am
> > emphasizing on the point that I am also adding new "time_arraived"
> > field to mbuf struct as part of below 17.02 Road map item.
>
> Thanks for your work for extending statistics support in DPDK.
>
> "time_arrived" points here for mostly RX path, while more common term used
> "timestamp"
> Allows also using it for TX path as well in the future.
>
I will take a note of this for next revision.
Thanks,
Reshma
^ permalink raw reply
* Re: [PATCH v5 06/21] eal/soc: introduce very essential SoC infra definitions
From: Shreyansh Jain @ 2016-10-25 12:38 UTC (permalink / raw)
To: Jan Viktorin; +Cc: david.marchand, dev, thomas.monjalon, Hemant Agrawal
In-Reply-To: <fa2b1051-568b-f697-4534-0ebc0d59717f@nxp.com>
On Tuesday 25 October 2016 11:06 AM, Shreyansh Jain wrote:
> Hello Jan,
>
> On Monday 24 October 2016 09:51 PM, Jan Viktorin wrote:
>> On Mon, 24 Oct 2016 17:29:25 +0530
>> Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
>>
>>> From: Jan Viktorin <viktorin@rehivetech.com>
>>>
>>> Define initial structures and functions for the SoC infrastructure.
>>> This patch supports only a very minimal functions for now.
>>> More features will be added in the following commits.
>>>
>>> Includes rte_device/rte_driver inheritance of
>>> rte_soc_device/rte_soc_driver.
>>>
>>> Signed-off-by: Jan Viktorin <viktorin@rehivetech.com>
>>> Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
>>> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
>>> ---
>>> app/test/Makefile | 1 +
>>> app/test/test_soc.c | 90 +++++++++++++++++++++
>>> lib/librte_eal/common/Makefile | 2 +-
>>> lib/librte_eal/common/eal_private.h | 4 +
>>> lib/librte_eal/common/include/rte_soc.h | 138
>>> ++++++++++++++++++++++++++++++++
>>> 5 files changed, 234 insertions(+), 1 deletion(-)
>>> create mode 100644 app/test/test_soc.c
>>> create mode 100644 lib/librte_eal/common/include/rte_soc.h
>>>
>>> diff --git a/app/test/Makefile b/app/test/Makefile
>>
>> [...]
>>
>>> +++ b/lib/librte_eal/common/include/rte_soc.h
>>> @@ -0,0 +1,138 @@
>>
>> [...]
>>
>>> +
>>> +#include <stdio.h>
>>> +#include <stdlib.h>
>>> +#include <stdint.h>
>>> +#include <inttypes.h>
>>> +#include <string.h>
>>> +
>>> +#include <rte_dev.h>
>>> +#include <rte_debug.h>
>>> +
>>> +struct rte_soc_id {
>>> + const char *compatible; /**< OF compatible specification */
>>> + uint64_t priv_data; /**< SoC Driver specific data */
>>
>> Do you expect this to be a pointer?
>
> A 64 bit entry, which can be typecasted to pointer by implementations,
> if required. Or, it might as well remain as a 64bit entry as ID.
>
>>
>>> +};
>>> +
>>
>> [...]
>>
>>> +
>>> +/**
>>> + * Initialization function for the driver called during SoC probing.
>>> + */
>>> +typedef int (soc_devinit_t)(struct rte_soc_driver *, struct
>>> rte_soc_device *);
>>> +
>>> +/**
>>> + * Uninitialization function for the driver called during hotplugging.
>>> + */
>>> +typedef int (soc_devuninit_t)(struct rte_soc_device *);
>>> +
>>> +/**
>>> + * A structure describing a SoC driver.
>>> + */
>>> +struct rte_soc_driver {
>>> + TAILQ_ENTRY(rte_soc_driver) next; /**< Next in list */
>>> + struct rte_driver driver; /**< Inherit core driver. */
>>> + soc_devinit_t *devinit; /**< Device initialization */
>>> + soc_devuninit_t *devuninit; /**< Device uninitialization */
>>
>> Shouldn't those functions be named probe/remove?
>
> Indeed. I think there was a comment on v4 as well - I thought I had
> fixed it but it seems I have mixed up my patches. I will send v6
> immediately with this fixed. Thanks for pointing out.
Ah, I just noticed that I did change it - but in Patch 11. Ideally, it
should have been done here itself. My bad.
>
>>
>>> + const struct rte_soc_id *id_table; /**< ID table, NULL
>>> terminated */
>>> +};
>>> +
>>
>> [...]
>>
>>> +#endif
>>
>>
>>
>
> -
> Shreyansh
>
-
Shreyansh
^ permalink raw reply
* Re: mbuf changes
From: Morten Brørup @ 2016-10-25 12:33 UTC (permalink / raw)
To: Bruce Richardson
Cc: Adrien Mazarguil, Wiles, Keith, dev, Olivier Matz, Oleg Kuporosov
In-Reply-To: <20161025122008.GA54704@bricha3-MOBL3.ger.corp.intel.com>
Comments at the end.
Med venlig hilsen / kind regards
- Morten Brørup
> -----Original Message-----
> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Tuesday, October 25, 2016 2:20 PM
> To: Morten Brørup
> Cc: Adrien Mazarguil; Wiles, Keith; dev@dpdk.org; Olivier Matz; Oleg
> Kuporosov
> Subject: Re: [dpdk-dev] mbuf changes
>
> On Tue, Oct 25, 2016 at 02:16:29PM +0200, Morten Brørup wrote:
> > Comments inline.
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce
> > > Richardson
> > > Sent: Tuesday, October 25, 2016 1:14 PM
> > > To: Adrien Mazarguil
> > > Cc: Morten Brørup; Wiles, Keith; dev@dpdk.org; Olivier Matz; Oleg
> > > Kuporosov
> > > Subject: Re: [dpdk-dev] mbuf changes
> > >
> > > On Tue, Oct 25, 2016 at 01:04:44PM +0200, Adrien Mazarguil wrote:
> > > > On Tue, Oct 25, 2016 at 12:11:04PM +0200, Morten Brørup wrote:
> > > > > Comments inline.
> > > > >
> > > > > Med venlig hilsen / kind regards
> > > > > - Morten Brørup
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > > > > > Sent: Tuesday, October 25, 2016 11:39 AM
> > > > > > To: Bruce Richardson
> > > > > > Cc: Wiles, Keith; Morten Brørup; dev@dpdk.org; Olivier Matz;
> > > > > > Oleg Kuporosov
> > > > > > Subject: Re: [dpdk-dev] mbuf changes
> > > > > >
> > > > > > On Mon, Oct 24, 2016 at 05:25:38PM +0100, Bruce Richardson
> wrote:
> > > > > > > On Mon, Oct 24, 2016 at 04:11:33PM +0000, Wiles, Keith
> wrote:
> > > > > > [...]
> > > > > > > > > On Oct 24, 2016, at 10:49 AM, Morten Brørup
> > > > > > <mb@smartsharesystems.com> wrote:
> > > > > > [...]
> >
> > > > > > > One other point I'll mention is that we need to have a
> > > > > > > discussion on how/where to add in a timestamp value into
> the
> > > > > > > mbuf. Personally, I think it can be in a union with the
> > > sequence
> > > > > > > number value, but I also suspect that 32-bits of a
> timestamp
> > > > > > > is not going to be enough for
> > > > > > many.
> > > > > > >
> > > > > > > Thoughts?
> > > > > >
> > > > > > If we consider that timestamp representation should use
> > > nanosecond
> > > > > > granularity, a 32-bit value may likely wrap around too
> quickly
> > > > > > to be useful. We can also assume that applications requesting
> > > > > > timestamps may care more about latency than throughput, Oleg
> > > found
> > > > > > that using the second cache line for this purpose had a
> > > noticeable impact [1].
> > > > > >
> > > > > > [1] http://dpdk.org/ml/archives/dev/2016-October/049237.html
> > > > >
> > > > > I agree with Oleg about the latency vs. throughput importance
> > > > > for
> > > such applications.
> > > > >
> > > > > If you need high resolution timestamps, consider them to be
> > > generated by the NIC RX driver, possibly by the hardware itself
> > > (http://w3new.napatech.com/features/time-precision/hardware-time-
> > > stamp), so the timestamp belongs in the first cache line. And I am
> > > proposing that it should have the highest possible accuracy, which
> > > makes the value hardware dependent.
> > > > >
> > > > > Furthermore, I am arguing that we leave it up to the
> application
> > > > > to
> > > keep track of the slowly moving bits (i.e. counting whole seconds,
> > > hours and calendar date) out of band, so we don't use precious
> space
> > > in the mbuf. The application doesn't need the NIC RX driver's fast
> > > path to capture which date (or even which second) a packet was
> > > received. Yes, it adds complexity to the application, but we can't
> > > set aside 64 bit for a generic timestamp. Or as a weird tradeoff:
> > > Put the fast moving 32 bit in the first cache line and the slow
> > > moving 32 bit in the second cache line, as a placeholder for the
> application to fill out if needed.
> > > Yes, it means that the application needs to check the time and
> > > update its variable holding the slow moving time once every second
> > > or so; but that should be doable without significant effort.
> > > >
> > > > That's a good point, however without a 64 bit value, elapsed time
> > > > between two arbitrary mbufs cannot be measured reliably due to
> not
> > > > enough context, one way or another the low resolution value is
> > > > also
> > > needed.
> > > >
> > > > Obviously latency-sensitive applications are unlikely to perform
> > > > lengthy buffering and require this but I'm not sure about all the
> > > > possible use-cases. Considering many NICs expose 64 bit
> timestaps,
> > > > I suggest we do not truncate them.
> > > >
> > > > I'm not a fan of the weird tradeoff either, PMDs will be tempted
> > > > to fill the extra 32 bits whenever they can and negate the
> > > > performance improvement of the first cache line.
> > >
> > > I would tend to agree, and I don't really see any convenient way to
> > > avoid putting in a 64-bit field for the timestamp in cache-line 0.
> > > If we are ok with having this overlap/partially overlap with
> > > sequence number, it will use up an extra 4B of storage in that
> cacheline.
> >
> > I agree about the lack of convenience! And Adrien certainly has a
> point about PMD temptations.
> >
> > However, I still don't think that a NICs ability to date-stamp a
> packet is sufficient reason to put a date-stamp in cache line 0 of the
> mbuf. Storing only the fast moving 32 bit in cache line 0 seems like a
> good compromise to me.
> >
> > Maybe you can find just one more byte, so it can hold 17 minutes with
> > nanosecond resolution. (I'm joking!)
> >
> > Please don't sacrifice the sequence number for the seconds/hours/days
> part a timestamp. Maybe it could be configurable to use a 32 bit or 64
> bit timestamp.
> >
> Do you see both timestamp and sequence numbers being used together? I
> would have thought that apps would either use one or the other?
> However, your suggestion is workable in any case, to allow the sequence
> number to overlap just the high 32 bits of the timestamp, rather than
> the low.
In our case, I can foresee sequence numbers used for packet processing and timestamps for timing analysis (and possibly for packet capturing, when being used). For timing analysis, we don’t need long durations, e.g. 4 seconds with 32 bit nanosecond resolution suffices. And for packet capturing we are perfectly capable of adding the slowly moving 32 bit of the timestamp to our output data stream without fetching it from the mbuf.
>
> /Bruce
^ permalink raw reply
* Re: [PATCH v4] net/i40e: fix hash filter invalid issue in X722
From: Bruce Richardson @ 2016-10-25 12:29 UTC (permalink / raw)
To: Wu, Jingjing; +Cc: Guo, Jia, Zhang, Helin, dev@dpdk.org
In-Reply-To: <9BB6961774997848B5B42BEC655768F80E294671@SHSMSX103.ccr.corp.intel.com>
On Tue, Oct 25, 2016 at 10:22:09AM +0000, Wu, Jingjing wrote:
>
>
> > -----Original Message-----
> > From: Guo, Jia
> > Sent: Tuesday, October 25, 2016 10:43 AM
> > To: Zhang, Helin <helin.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
> > Cc: dev@dpdk.org; Guo, Jia <jia.guo@intel.com>
> > Subject: [PATCH v4] net/i40e: fix hash filter invalid issue in X722
> >
> > When verifying the Hash filtering on X722, we found a problem that
> > the hash value in descriptor is incorrect. The root caused is X722
> > uses different way of hash key word selection comparing with X710/XL710.
> > This patch fixes it by setting X722 specific key selection.
> >
> > Signed-off-by: Jeff Guo <jia.guo@intel.com>
>
> If we use default configuration on HW, this issue will not exist. But
> our software sets the default value through i40e_filter_input_set_init.
> So I think fix line need to be the commit which introduced input set
> changing feature.
>
> Acked-by: Jingjing Wu <jingjing.wu@intel.com> with additional line:
>
> Fixes: commit 98f055707685 ("i40e: configure input fields for RSS or flow director ")
>
Applied, with fixes line, to dpdk-next-net/rel_16_11
/Bruce
^ permalink raw reply
* [PATCH] doc: release note for ixgbe PMD API's
From: Bernard Iremonger @ 2016-10-25 12:24 UTC (permalink / raw)
To: dev, john.mcnamara; +Cc: Bernard Iremonger
Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
---
doc/guides/rel_notes/release_16_11.rst | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/doc/guides/rel_notes/release_16_11.rst b/doc/guides/rel_notes/release_16_11.rst
index 26cdd62..8253614 100644
--- a/doc/guides/rel_notes/release_16_11.rst
+++ b/doc/guides/rel_notes/release_16_11.rst
@@ -131,6 +131,10 @@ New Features
The GCC 4.9 ``-march`` option supports the Intel processor code names.
The config option ``RTE_MACHINE`` can be used to pass code names to the compiler as ``-march`` flag.
+* **Added API's for VF management to the ixgbe PMD.**
+
+ Eight new API's have been added to the ixgbe PMD for VF management from the PF.
+ The declarations for the API's can be found in ``rte_pmd_ixgbe.h``.
Resolved Issues
---------------
--
2.10.1
^ permalink raw reply related
* Re: [PATCH v2] net/i40e: fix fdir configure failed issue in X710
From: Bruce Richardson @ 2016-10-25 12:23 UTC (permalink / raw)
To: Wu, Jingjing; +Cc: Guo, Jia, Zhang, Helin, dev@dpdk.org
In-Reply-To: <9BB6961774997848B5B42BEC655768F80E29468E@SHSMSX103.ccr.corp.intel.com>
On Tue, Oct 25, 2016 at 10:28:22AM +0000, Wu, Jingjing wrote:
>
>
> > -----Original Message-----
> > From: Guo, Jia
> > Sent: Tuesday, October 25, 2016 10:26 AM
> > To: Zhang, Helin <helin.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
> > Cc: dev@dpdk.org; Guo, Jia <jia.guo@intel.com>
> > Subject: [PATCH v2] net/i40e: fix fdir configure failed issue in X710
> >
> > Because of some register is only supported by X722, such as I40E_GLQF_FD_PCTYPES,
> > so it need to use the mac type to distinguish the behavior of X722 from X710 and other
> > NICs, or it would result X710 functional failed.
> >
> > Fixes: 8c5cb3c11513 (“net/i40e: add packet type translation for X722”)
> > Signed-off-by: Jeff Guo <jia.guo@intel.com>
>
> Acked-by: Jingjing Wu <jingjing.wu@intel.com>
>
Applied to dpdk-next-net/rel_16_11
/Bruce
^ permalink raw reply
* Re: mbuf changes
From: Bruce Richardson @ 2016-10-25 12:20 UTC (permalink / raw)
To: Morten Brørup
Cc: Adrien Mazarguil, Wiles, Keith, dev, Olivier Matz, Oleg Kuporosov
In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC359EA8BA@smartserver.smartshare.dk>
On Tue, Oct 25, 2016 at 02:16:29PM +0200, Morten Brørup wrote:
> Comments inline.
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
> > Sent: Tuesday, October 25, 2016 1:14 PM
> > To: Adrien Mazarguil
> > Cc: Morten Brørup; Wiles, Keith; dev@dpdk.org; Olivier Matz; Oleg
> > Kuporosov
> > Subject: Re: [dpdk-dev] mbuf changes
> >
> > On Tue, Oct 25, 2016 at 01:04:44PM +0200, Adrien Mazarguil wrote:
> > > On Tue, Oct 25, 2016 at 12:11:04PM +0200, Morten Brørup wrote:
> > > > Comments inline.
> > > >
> > > > Med venlig hilsen / kind regards
> > > > - Morten Brørup
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > > > > Sent: Tuesday, October 25, 2016 11:39 AM
> > > > > To: Bruce Richardson
> > > > > Cc: Wiles, Keith; Morten Brørup; dev@dpdk.org; Olivier Matz; Oleg
> > > > > Kuporosov
> > > > > Subject: Re: [dpdk-dev] mbuf changes
> > > > >
> > > > > On Mon, Oct 24, 2016 at 05:25:38PM +0100, Bruce Richardson wrote:
> > > > > > On Mon, Oct 24, 2016 at 04:11:33PM +0000, Wiles, Keith wrote:
> > > > > [...]
> > > > > > > > On Oct 24, 2016, at 10:49 AM, Morten Brørup
> > > > > <mb@smartsharesystems.com> wrote:
> > > > > [...]
> > > > > > > > 5.
> > > > > > > >
> > > > > > > > And here’s something new to think about:
> > > > > > > >
> > > > > > > > m->next already reveals if there are more segments to a
> > packet.
> > > > > Which purpose does m->nb_segs serve that is not already covered
> > by
> > > > > m-
> > > > > >next?
> > > > > >
> > > > > > It is duplicate info, but nb_segs can be used to check the
> > > > > > validity
> > > > > of
> > > > > > the next pointer without having to read the second mbuf
> > cacheline.
> > > > > >
> > > > > > Whether it's worth having is something I'm happy enough to
> > > > > > discuss, though.
> > > > >
> > > > > Although slower in some cases than a full blown "next packet"
> > > > > pointer, nb_segs can also be conveniently abused to link several
> > > > > packets and their segments in the same list without wasting
> > space.
> > > >
> > > > I don’t understand that; can you please elaborate? Are you abusing
> > m->nb_segs as an index into an array in your application? If that is
> > the case, and it is endorsed by the community, we should get rid of m-
> > >nb_segs and add a member for application specific use instead.
> > >
> > > Well, that's just an idea, I'm not aware of any application using
> > > this, however the ability to link several packets with segments seems
> > > useful to me (e.g. buffering packets). Here's a diagram:
> > >
> > > .-----------. .-----------. .-----------. .-----------. .---
> > ---
> > > | pkt 0 | | seg 1 | | seg 2 | | pkt 1 | |
> > pkt 2
> > > | next --->| next --->| next --->| next --->|
> > ...
> > > | nb_segs 3 | | nb_segs 1 | | nb_segs 1 | | nb_segs 1 | |
> > > `-----------' `-----------' `-----------' `-----------' `---
> > ---
>
> I see. It makes it possible to refer to a burst of packets (with segments or not) by a single mbuf reference, as an alternative to the current design pattern of using an array and length (struct rte_mbuf **mbufs, unsigned count).
>
> This would require implementation in the PMDs etc.
>
> And even in this case, m->nb_segs does not need to be an integer, but could be replaced by a single bit indicating if the segment is a continuation of a packet or the beginning (alternatively the end) of a packet, i.e. the bit can be set for either the first or the last segment in the packet.
>
> It is an almost equivalent alternative to the fundamental design pattern of using an array of mbuf with count, which is widely implemented in DPDK. And m->next still lives in the second cache line, so I don't see any gain by this.
>
> I still don't get how m->nb_segs can be abused without m->next.
>
>
> > > > > > One other point I'll mention is that we need to have a
> > > > > > discussion on how/where to add in a timestamp value into the
> > > > > > mbuf. Personally, I think it can be in a union with the
> > sequence
> > > > > > number value, but I also suspect that 32-bits of a timestamp is
> > > > > > not going to be enough for
> > > > > many.
> > > > > >
> > > > > > Thoughts?
> > > > >
> > > > > If we consider that timestamp representation should use
> > nanosecond
> > > > > granularity, a 32-bit value may likely wrap around too quickly to
> > > > > be useful. We can also assume that applications requesting
> > > > > timestamps may care more about latency than throughput, Oleg
> > found
> > > > > that using the second cache line for this purpose had a
> > noticeable impact [1].
> > > > >
> > > > > [1] http://dpdk.org/ml/archives/dev/2016-October/049237.html
> > > >
> > > > I agree with Oleg about the latency vs. throughput importance for
> > such applications.
> > > >
> > > > If you need high resolution timestamps, consider them to be
> > generated by the NIC RX driver, possibly by the hardware itself
> > (http://w3new.napatech.com/features/time-precision/hardware-time-
> > stamp), so the timestamp belongs in the first cache line. And I am
> > proposing that it should have the highest possible accuracy, which
> > makes the value hardware dependent.
> > > >
> > > > Furthermore, I am arguing that we leave it up to the application to
> > keep track of the slowly moving bits (i.e. counting whole seconds,
> > hours and calendar date) out of band, so we don't use precious space in
> > the mbuf. The application doesn't need the NIC RX driver's fast path to
> > capture which date (or even which second) a packet was received. Yes,
> > it adds complexity to the application, but we can't set aside 64 bit
> > for a generic timestamp. Or as a weird tradeoff: Put the fast moving 32
> > bit in the first cache line and the slow moving 32 bit in the second
> > cache line, as a placeholder for the application to fill out if needed.
> > Yes, it means that the application needs to check the time and update
> > its variable holding the slow moving time once every second or so; but
> > that should be doable without significant effort.
> > >
> > > That's a good point, however without a 64 bit value, elapsed time
> > > between two arbitrary mbufs cannot be measured reliably due to not
> > > enough context, one way or another the low resolution value is also
> > needed.
> > >
> > > Obviously latency-sensitive applications are unlikely to perform
> > > lengthy buffering and require this but I'm not sure about all the
> > > possible use-cases. Considering many NICs expose 64 bit timestaps, I
> > > suggest we do not truncate them.
> > >
> > > I'm not a fan of the weird tradeoff either, PMDs will be tempted to
> > > fill the extra 32 bits whenever they can and negate the performance
> > > improvement of the first cache line.
> >
> > I would tend to agree, and I don't really see any convenient way to
> > avoid putting in a 64-bit field for the timestamp in cache-line 0. If
> > we are ok with having this overlap/partially overlap with sequence
> > number, it will use up an extra 4B of storage in that cacheline.
>
> I agree about the lack of convenience! And Adrien certainly has a point about PMD temptations.
>
> However, I still don't think that a NICs ability to date-stamp a packet is sufficient reason to put a date-stamp in cache line 0 of the mbuf. Storing only the fast moving 32 bit in cache line 0 seems like a good compromise to me.
>
> Maybe you can find just one more byte, so it can hold 17 minutes with nanosecond resolution. (I'm joking!)
>
> Please don't sacrifice the sequence number for the seconds/hours/days part a timestamp. Maybe it could be configurable to use a 32 bit or 64 bit timestamp.
>
Do you see both timestamp and sequence numbers being used together? I
would have thought that apps would either use one or the other? However,
your suggestion is workable in any case, to allow the sequence number to
overlap just the high 32 bits of the timestamp, rather than the low.
/Bruce
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox