[dpdk-dev] [RFC 0/8] mbuf: structure reorganization

[Ananyev, Konstantin]

Hi Olivier,

> > > > > > 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().
> 
> I don't see a real advantage now, but I think this is something that
> could be added once we have the normalization code.
> 
> 
> 
> > > 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.
> 
> It's similar to what I proposed for the timestamp normalization: for both
> functions, you need to call an ethdev function with a port_id as a parameter.
> Either you get the port from the mbuf (this is my initial suggestion that you
> don't like), either you know it because you retrieved your mbuf with
> rte_eth_rx_burst(port_id, ...) (this is what is done in examples/ptpclient).
> 
> So, do you still see an issue with having a function to normalize/synchronize
> the timestamp that takes a port id as a parameter?

If it hasn't rely on mbuf->port value, then probably  not.

> 
> 
> > 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)
> 
> I think it needs to be raw now, because we don't have any normalization code
> at the moment. Maybe we could add a "normalized" flag if it makes sense in
> the future, once we have decided what normalized means, in a context where several
> PMDs/libs can have their own timestamp.
> 
> But once we have a clear definition of what normalized means + an example of
> normalization code, we may have this NORM flag.
> 
> > 4. PMD can provide an optional routine inside devops:
> > uint64_t dev_ops->timestamp_normalise(uint64_t timestamps);
> 
> I think (but I'm not sure, it's really out of scope of this patchset),
> that the timestamp synchronization API will be more complex than that.
> 
> My current idea:
> 
> - a rte_timestamp library holds the normalization code
> - we decide, for instance, that "normalized" means:
>   - unit: nanosecond
>   - based on system clock
>   - reference: 0 = time when rte_timestamp_init() was called
> - the PMD provides an API to get its clock
> - the lib provides something like:
>   uint64_t rte_timestamp_normalize(unsigned int port_id, uint64_t timestamp)
> 
> 
> > 5. If the user wants to use that function it would be his responsibility to map mbuf
> > to the port it was received from.
> 
> Yes, if the application uses a port_id, it's its responsibility to ensure
> that this port exists.

Ok, so for 17.05 we'll have:
- raw timestamp value inside mbuf
- ol_flag bit to represenet is mbuf->timestamp value valid or not.
That's it, correct?

Konstantin