From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
To: Olivier Matz <olivier.matz@6wind.com>
Cc: Jan Blunck <jblunck@infradead.org>,
"Richardson, Bruce" <bruce.richardson@intel.com>,
"dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [RFC 0/8] mbuf: structure reorganization
Date: Wed, 8 Mar 2017 11:11:23 +0000 [thread overview]
Message-ID: <2601191342CEEE43887BDE71AB9772583FACBF10@IRSMSX109.ger.corp.intel.com> (raw)
In-Reply-To: <20170302174623.268592a7@platinum>
Hi Olivier,
>
> Hi Konstantin,
>
> On Tue, 28 Feb 2017 22:53:55 +0000, "Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:
> > > > Another thing that doesn't look very convenient to me here -
> > > > We can have 2 different values of timestamp (both normalized and not)
> > > > and there is no clear way for the application to know which one is in
> > > > use right now. So each app writer would have to come-up with his own
> > > > solution.
> > >
> > > It depends:
> > > - the solution you describe is to have the application storing the
> > > normalized value in its private metadata.
> > > - another solution would be to store the normalized value in
> > > m->timestamp. In this case, we would need a flag to tell if the
> > > timestamp value is normalized.
> >
> > My first thought also was about second flag to specify was timestamp
> > already normalized or not.
> > Though I still in doubt - is it all really worth it: extra ol_flag, new function in eth_dev API.
> > My feeling that we trying to overcomplicate things.
>
> I don't see what is so complicated. The idea is just to let the
> application do the normalization if it is required.
I meant 2 ol_flags and special function just to treat properly one of the mbuf field
seems too much.
Though after second thought might be 2 ol_flags is not a bad idea -
it gives PMD writer a freedom to choose provide a normalized or raw value
on return from rx_burst().
>
> If the time is normalized in nanosecond in the PMD, we would still
> need to normalized the time reference (the 0). And for that we'd need
> a call to a synchronization code as well.
>
>
>
> > > The problem pointed out by Jan is that doing the timestamp
> > > normalization may take some CPU cycles, even if a small part of packets
> > > requires it.
> >
> > I understand that point, but from what I've seen with real example:
> > http://dpdk.org/ml/archives/dev/2016-October/048810.html
> > the amount of calculations at RX is pretty small.
> > I don't think it would affect performance in a noticeable way
> > (though I don't have any numbers here to prove it).
>
> I think we can consider by default that adding code in the data path
> impacts performance.
>
>
> > From other side, if user doesn't want a timestamp he can always disable
> > that feature anad save cycles, right?
> >
> > BTW, you and Jan both mention that not every packet would need a timestamp.
> > Instead we need sort of a timestamp for the group of packets?
>
> I think that for many applications the timestamp should be as precise
> as possible for each packet.
>
>
> > Is that really the only foreseen usage model?
>
> No, but it could be one.
>
>
> > If so, then why not to have a special function that would extract 'latest' timestamp
> > from the dev?
> > Or even have tx_burst_extra() that would return a latest timestamp (extra parameter or so).
> > Then there is no need to put timestamp into mbuf at all.
>
> Doing that will give a poor precision for the timestamp.
>
>
> > > > > Applications that
> > > > > are doing this are responsible of what they change.
> > > > >
> > > > >
> > > > > > 3. In theory with eth_dev_detach() - mbuf->port value might be
> > > > > > not valid at the point when application would decide to do
> > > > > > normalization.
> > > > > >
> > > > > > So to me all that approach with delayed normalization seems
> > > > > > unnecessary overcomplicated. Original one suggested by Olivier,
> > > > > > when normalization is done in PMD at RX look much cleaner and
> > > > > > more manageable.
> > > > >
> > > > > Detaching a device requires a synchronization between control and
> > > > > data plane, and not only for this use case.
> > > >
> > > > Of course it does.
> > > > But right now it is possible to do:
> > > >
> > > > eth_rx_burst(port=0, ..., &mbuf, 1);
> > > > eth_dev_detach(port=0, ...);
> > > > ...
> > > > /*process previously received mbuf */
> > > >
> > > > With what you are proposing it would be not always possible any more.
> > >
> > > With your example, it does not work even without the timestamp feature,
> > > since the mbuf input port would reference an invalid port.
> > > This port is usually used in the application to do a lookup for an port structure,
> > > so it is expected that the entry is valid. It would be even worse if you
> > > do a detach + attach.
> >
> > I am not talking about the mbuf->port value usage.
> > Right now user can access/interpret all metadata fields set by PMD RX routines
> > (vlan, rss hash, ol_flags, ptype, etc.) without need to accessing the device data or
> > calling device functions.
> > With that change it wouldn't be the case anymore.
>
> That's the same for some other functions. If in my application I want
> to call eth_rx_queue_count(m->port), I will have the same problem.
Yes, but here you are trying to get extra information about device/queue based
on port value stored inside mbuf.
I am talking about information that already stored inside particular mbuf itself.
About m->port itself - as I said before my preference would be to remove it at all
(partly because of that implication - we can't guarantee that m->port information
would be valid though all mbuf lifetime).
But that's probably subject of another discussion.
>
> I think we also have something quite similar in examples/ptpclient:
>
> rte_eth_rx_burst(portid, 0, &m, 1);
> ...
> parse_ptp_frames(portid, m);
> ...
> ptp_data.portid = portid;
> ...
> rte_eth_timesync_read_tx_timestamp(ptp_data->portid, ...)
>
>
> So, really, I think it's an application issue: when the app deletes
> a port, it should ask itself if there are remaining references to
> it (m->port).
Hmm, and where in the example below do you see the reference to the m->port?
As I can see, what that the code above does:
- it deduces portid value from global variable - not from m->port
- saves portid info (not from m->port) inside global variable ptp_data.portid
- later inside same function it used that value to call rte_ethdev functions
(via parse_fup or parse_drsp).
So I am not sure how it relates to the topic we are discussing.
Anyway, to summarize how the proposal looks right now:
1. m->timestamp value after rx_burst() could be either in raw or normalized format.
2. validity of m->timesamp and the it's format should be determined by 2 ol_flags
(something like: RX_TIMESTAMP, RX_TIMESTAMP_NORM).
3. PMD is free to choose what timestamp value to return (raw/normalized)
4. PMD can provide an optional routine inside devops:
uint64_t dev_ops->timestamp_normalise(uint64_t timestamps);
5. If the user wants to use that function it would be his responsibility to map mbuf
to the port it was received from.
Is that correct?
Thanks
Konstantin
>
> > > So, I think it is already the responsibility of the application to do
> > > the sync (flush retrieved packets before detaching a port).
> >
> > The packets are not in RX or TX queue of detaching device any more.
> > I received a packet, after that I expect to have all its data and metadata inside mbuf.
> > So I can store mbufs somewhere and process them much later.
> > Or might be I would like to pass it to the secondary process for logging/analyzing, etc.
>
> Yes, but that's still an app problem for me.
>
>
> > > > >In the first solution, the normalization is
> > > > > partial: unit is nanosecond, but the time reference is different.
> > > >
> > > > Not sure I get you here...
> > >
> > > In the first solution I described, each PMD had to convert its unit
> > > into nanosecond. This is easy because we assume the PMD knows the
> > > value of its clock. But to get a fully normalized value, it also has to
> > > use the same time reference, so we would also need to manage an offset
> > > (we need a new API to give this value to the PMD).
> >
> > Yes, I suppose we do need an start timestamp and sort of factor() to convert
> > HW value, something like:
> >
> > mbuf->timestamp = rxq->start_timestamp + factor(hw_timestamp);
> >
> > Right?
> > Why passing start_timestamp at the configure() phase will be a problem?
> >
> > >
> > > I have another fear related to hardware clocks: if clocks are not
> > > synchronized between PMDs, the simple operation "t * ratio - offset"
> > > won't work. That's why I think we could delegate this job in a specific
> > > library that would manage this.
> >
> > But then that library would need to account all PMDs inside the system,
> > and be aware about each HW clock skew, etc.
> > Again, doesn't' sound like an simple task to me.
>
> Exactly, that's also why I want to let the specialists take care of
> it. Having non-normalized timestamps now allow to do the job later
> when required, while allowing basic usages as required by metrics
> libraries and mlx pmd.
>
>
>
> Olivier
next prev parent reply other threads:[~2017-03-08 11:13 UTC|newest]
Thread overview: 155+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-01-24 15:19 Olivier Matz
2017-01-24 15:19 ` [dpdk-dev] [RFC 1/8] mbuf: make segment prefree function public Olivier Matz
2017-01-24 15:19 ` [dpdk-dev] [RFC 2/8] mbuf: make raw free " Olivier Matz
2017-01-24 15:19 ` [dpdk-dev] [RFC 3/8] mbuf: set mbuf fields while in pool Olivier Matz
2017-01-24 15:50 ` Bruce Richardson
2017-02-28 14:51 ` Olivier Matz
2017-01-24 15:19 ` [dpdk-dev] [RFC 4/8] net: don't touch mbuf next or nb segs on Rx Olivier Matz
2017-01-24 15:19 ` [dpdk-dev] [RFC 5/8] mbuf: make rearm data address naturally aligned Olivier Matz
2017-01-24 15:19 ` [dpdk-dev] [RFC 6/8] mbuf: use 2 bytes for port and nb segments Olivier Matz
2017-01-24 15:19 ` [dpdk-dev] [RFC 7/8] mbuf: move sequence number in second cache line Olivier Matz
2017-01-24 15:19 ` [dpdk-dev] [RFC 8/8] mbuf: add a timestamp field Olivier Matz
2017-01-24 15:59 ` [dpdk-dev] [RFC 0/8] mbuf: structure reorganization Bruce Richardson
2017-01-24 16:16 ` Olivier MATZ
2017-02-06 18:41 ` Ananyev, Konstantin
2017-02-09 16:20 ` Morten Brørup
2017-02-09 16:56 ` Ananyev, Konstantin
2017-02-16 13:48 ` Olivier Matz
2017-02-16 15:46 ` Bruce Richardson
2017-02-16 16:14 ` Olivier Matz
2017-02-21 14:20 ` Morten Brørup
2017-02-21 14:28 ` Bruce Richardson
2017-02-21 15:04 ` Olivier MATZ
2017-02-21 15:18 ` Bruce Richardson
2017-02-21 15:18 ` Morten Brørup
2017-02-19 19:04 ` Chilikin, Andrey
2017-02-21 9:53 ` Olivier MATZ
2017-02-16 17:26 ` Jan Blunck
2017-02-17 10:51 ` Olivier Matz
2017-02-17 12:49 ` Nélio Laranjeiro
2017-02-17 13:51 ` Jan Blunck
2017-02-18 5:48 ` Andrew Rybchenko
2017-02-17 13:38 ` Jan Blunck
2017-02-17 14:17 ` Olivier Matz
2017-02-17 18:42 ` Ananyev, Konstantin
2017-02-21 9:53 ` Olivier MATZ
2017-02-21 10:28 ` Ananyev, Konstantin
2017-02-20 9:27 ` Jan Blunck
2017-02-21 9:54 ` Olivier MATZ
2017-02-21 16:12 ` Jan Blunck
2017-02-21 16:38 ` Bruce Richardson
2017-02-21 17:04 ` Jan Blunck
2017-02-21 17:26 ` Ananyev, Konstantin
2017-02-21 19:17 ` Jan Blunck
2017-02-21 20:30 ` Ananyev, Konstantin
2017-02-21 21:51 ` Morten Brørup
2017-02-24 14:11 ` Olivier Matz
2017-02-24 14:00 ` Olivier Matz
2017-02-24 14:21 ` Bruce Richardson
2017-02-28 8:55 ` Jan Blunck
2017-02-28 9:05 ` Ananyev, Konstantin
2017-02-28 9:23 ` Olivier Matz
2017-02-28 9:33 ` Jan Blunck
2017-02-28 10:29 ` Ananyev, Konstantin
2017-02-28 10:50 ` Olivier Matz
2017-02-28 11:48 ` Ananyev, Konstantin
2017-02-28 12:28 ` Olivier Matz
2017-02-28 22:53 ` Ananyev, Konstantin
2017-03-02 16:46 ` Olivier Matz
2017-03-08 11:11 ` Ananyev, Konstantin [this message]
2017-03-20 9:00 ` Olivier Matz
2017-03-22 17:42 ` Ananyev, Konstantin
2017-03-24 8:35 ` Jerin Jacob
2017-03-24 13:35 ` Olivier Matz
2017-02-28 9:25 ` Jan Blunck
2017-02-19 23:45 ` Ananyev, Konstantin
2017-02-21 9:22 ` Morten Brørup
2017-02-21 9:54 ` Olivier MATZ
2017-03-08 9:41 ` [dpdk-dev] [PATCH 0/9] " Olivier Matz
2017-03-08 9:41 ` [dpdk-dev] [PATCH 1/9] mbuf: make segment prefree function public Olivier Matz
2017-03-08 9:41 ` [dpdk-dev] [PATCH 2/9] mbuf: make raw free " Olivier Matz
2017-03-08 9:41 ` [dpdk-dev] [PATCH 3/9] mbuf: set mbuf fields while in pool Olivier Matz
2017-03-31 11:21 ` Bruce Richardson
2017-03-31 11:51 ` Ananyev, Konstantin
2017-03-08 9:41 ` [dpdk-dev] [PATCH 4/9] drivers/net: don't touch mbuf next or nb segs on Rx Olivier Matz
2017-03-08 9:41 ` [dpdk-dev] [PATCH 5/9] mbuf: make rearm data address naturally aligned Olivier Matz
2017-03-08 9:41 ` [dpdk-dev] [PATCH 6/9] mbuf: use 2 bytes for port and nb segments Olivier Matz
2017-03-08 9:41 ` [dpdk-dev] [PATCH 7/9] mbuf: move sequence number in second cache line Olivier Matz
2017-03-08 9:42 ` [dpdk-dev] [PATCH 8/9] mbuf: add a timestamp field Olivier Matz
2017-04-04 10:29 ` [dpdk-dev] [PATCH 0/2] reduce writes to mbuf in ixgbe vRX Konstantin Ananyev
2017-04-07 15:13 ` Ferruh Yigit
2017-04-07 15:44 ` Ferruh Yigit
2017-04-09 22:56 ` Ananyev, Konstantin
2017-04-04 10:29 ` [dpdk-dev] [PATCH 1/2] net/ixgbe: eliminate mbuf write on rearm Konstantin Ananyev
2017-04-10 15:59 ` [dpdk-dev] [PATCH v2 0/2] reduce writes to mbuf in ixgbe vRX Konstantin Ananyev
2017-04-10 16:17 ` Ferruh Yigit
2017-04-10 15:59 ` [dpdk-dev] [PATCH v2 1/2] net/ixgbe: eliminate mbuf write on rearm Konstantin Ananyev
2017-04-10 15:59 ` [dpdk-dev] [PATCH v2 2/2] net/ixgbe: remove option to disable offload flags Konstantin Ananyev
2017-04-04 10:29 ` [dpdk-dev] [PATCH " Konstantin Ananyev
2017-03-08 9:42 ` [dpdk-dev] [PATCH 9/9] mbuf: reorder VLAN tci and buffer len fields Olivier Matz
2017-03-29 15:56 ` [dpdk-dev] [PATCH 0/9] mbuf: structure reorganization Olivier Matz
2017-03-29 16:03 ` Morten Brørup
2017-03-29 20:09 ` Bruce Richardson
2017-03-30 9:31 ` Bruce Richardson
2017-03-30 12:02 ` Olivier Matz
2017-03-30 12:23 ` Bruce Richardson
2017-03-30 16:45 ` Ananyev, Konstantin
2017-03-30 16:47 ` Ananyev, Konstantin
2017-03-30 18:06 ` Ananyev, Konstantin
2017-03-31 8:41 ` Olivier Matz
2017-03-31 9:58 ` Ananyev, Konstantin
2017-03-31 1:00 ` Ananyev, Konstantin
2017-03-31 7:21 ` Morten Brørup
2017-03-31 8:26 ` Olivier Matz
2017-03-31 8:41 ` Bruce Richardson
2017-03-31 8:59 ` Olivier Matz
2017-03-31 9:18 ` Ananyev, Konstantin
2017-03-31 9:36 ` Olivier Matz
2017-04-03 16:15 ` Thomas Monjalon
2017-04-04 7:58 ` Olivier MATZ
2017-04-04 8:53 ` Bruce Richardson
2017-03-31 9:23 ` Bruce Richardson
2017-03-31 11:18 ` Nélio Laranjeiro
2017-03-30 14:54 ` Andrew Rybchenko
2017-03-30 15:12 ` Jerin Jacob
2017-04-04 16:27 ` [dpdk-dev] [PATCH v2 0/8] " Olivier Matz
2017-04-04 16:28 ` [dpdk-dev] [PATCH v2 1/8] mbuf: make segment prefree function public Olivier Matz
2017-04-04 16:28 ` [dpdk-dev] [PATCH v2 2/8] mbuf: make raw free " Olivier Matz
2017-04-04 16:28 ` [dpdk-dev] [PATCH v2 3/8] mbuf: set mbuf fields while in pool Olivier Matz
2017-04-04 16:28 ` [dpdk-dev] [PATCH v2 4/8] drivers/net: don't touch mbuf next or nb segs on Rx Olivier Matz
2017-04-04 16:28 ` [dpdk-dev] [PATCH v2 5/8] mbuf: make rearm data address naturally aligned Olivier Matz
2017-04-04 16:28 ` [dpdk-dev] [PATCH v2 6/8] mbuf: use 2 bytes for port and nb segments Olivier Matz
2017-04-06 5:45 ` Yuanhan Liu
2017-04-18 13:03 ` Olivier MATZ
2017-07-04 7:54 ` Wang, Zhihong
2017-07-10 8:00 ` Olivier Matz
2017-07-10 8:15 ` Morten Brørup
2017-07-11 13:25 ` Wiles, Keith
2017-07-11 13:30 ` Morten Brørup
2017-07-11 15:05 ` Thomas Monjalon
2017-07-11 15:23 ` [dpdk-dev] [PATCH v2 6/8] mbuf: use 2 bytes for port and nbsegments Morten Brørup
2017-07-11 16:48 ` Wiles, Keith
2017-07-12 7:25 ` Morten Brørup
2017-07-12 9:02 ` Yang, Zhiyong
2017-07-12 9:50 ` [dpdk-dev] [PATCH v2 6/8] mbuf: use 2 bytes for port andnbsegments Morten Brørup
2017-07-12 15:35 ` Stephen Hemminger
2017-07-12 15:57 ` Morten Brørup
2017-07-12 16:23 ` Thomas Monjalon
2017-07-12 18:20 ` Wiles, Keith
2017-07-21 15:03 ` Bruce Richardson
2017-07-12 15:34 ` [dpdk-dev] [PATCH v2 6/8] mbuf: use 2 bytes for port and nbsegments Wiles, Keith
2017-07-11 13:34 ` [dpdk-dev] [PATCH v2 6/8] mbuf: use 2 bytes for port and nb segments Wiles, Keith
2017-07-11 13:46 ` Olivier MATZ
2017-04-04 16:28 ` [dpdk-dev] [PATCH v2 7/8] mbuf: move sequence number in second cache line Olivier Matz
2017-04-04 16:28 ` [dpdk-dev] [PATCH v2 8/8] mbuf: add a timestamp field Olivier Matz
2017-04-05 9:37 ` [dpdk-dev] [PATCH v2 0/8] mbuf: structure reorganization Thomas Monjalon
2017-04-05 9:46 ` Olivier MATZ
2017-04-05 9:48 ` Richardson, Bruce
2017-04-05 12:06 ` Ferruh Yigit
2017-04-14 13:10 ` Ferruh Yigit
2017-04-18 13:04 ` Olivier MATZ
2017-04-19 9:39 ` Thomas Monjalon
2017-04-19 12:28 ` Olivier MATZ
2017-04-19 12:56 ` Thomas Monjalon
2017-04-19 13:03 ` Ferruh Yigit
2017-04-19 13:12 ` Thomas Monjalon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2601191342CEEE43887BDE71AB9772583FACBF10@IRSMSX109.ger.corp.intel.com \
--to=konstantin.ananyev@intel.com \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=jblunck@infradead.org \
--cc=olivier.matz@6wind.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).