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

[Olivier Matz <olivier.matz@6wind.com>]

On Tue, 28 Feb 2017 11:48:20 +0000, "Ananyev, Konstantin"
<konstantin.ananyev@intel.com> wrote:
> > 
> > On Tue, 28 Feb 2017 10:29:41 +0000, "Ananyev, Konstantin"
> > <konstantin.ananyev@intel.com> wrote:  
> > > >
> > > > Hi,
> > > >
> > > > On Tue, 28 Feb 2017 09:05:07 +0000, "Ananyev, Konstantin"
> > > > <konstantin.ananyev@intel.com> wrote:  
> > > > > Hi everyone,
> > > > >  
> > > > > > >
> > > > > > > In my opinion, if we have the room in the first cache
> > > > > > > line, we should put it there. The only argument I see
> > > > > > > against is "we may find something more important in the
> > > > > > > future, and we won't have room for it in the first cache
> > > > > > > line". I don't feel we should penalize today's use cases
> > > > > > > for hypothetic future use cases.
> > > > > > >
> > > > > > >
> > > > > > >  
> > > > > > >> 2. timestamp normalization point
> > > > > > >>      inside PMD RX vs somewhere later as user needs it
> > > > > > >> (extra function in dev_ops?).  
> > > > > > >
> > > > > > > This point could be changed. My initial proposition tries
> > > > > > > to provide a generic API for timestamp. Let me remind it
> > > > > > > here:
> > > > > > >
> > > > > > > a- the timestamp is in nanosecond
> > > > > > > b- the reference is always the same for a given path: if
> > > > > > > the timestamp is set in a PMD, all the packets for this
> > > > > > > PMD will have the same reference, but for 2 different
> > > > > > > PMDs (or a sw lib), the reference would not be the same.
> > > > > > >
> > > > > > > We may remove a-, and just have:
> > > > > > >  - the reference and the unit are always the same for a
> > > > > > > given path: if the timestamp is set in a PMD, all the
> > > > > > > packets for this PMD will have the same reference and
> > > > > > > unit, but for 2 different PMDs (or a sw lib), they would
> > > > > > > not be the same.
> > > > > > >
> > > > > > > In both cases, we would need a conversion code (maybe in a
> > > > > > > library) if the application wants to work with timestamps
> > > > > > > from several sources. The second solution removes the
> > > > > > > normalization code in the PMD when not needed, it is
> > > > > > > probably better.  
> > > > > >
> > > > > > I agree.  
> > > > >
> > > > > One question - does that mean that application would need to
> > > > > keep a track from what PMD each particular packet came to do
> > > > > the normalization? Konstantin  
> > > >
> > > > I'd say yes. It does not look very difficult to do, since the
> > > > mbuf contains the input port id.
> > > >  
> > >
> > > I understand that we can use mbuf->port here, but it means that
> > > we'll introduce new implicit dependency between timestamp and
> > > port values. From my point that introduces new implications:
> > > 1. all PMDs that do set a timestamp would also have to set port
> > > value too. Probably not a big deal as most of PMDs do set port
> > > value anyway right now, but it means it would be hard to get
> > > rid/change mbuf->port in future.  
> > 
> > Currently, all PMDs must set m->port.
> > If in the future we remove m->port, the applications that use it
> > will need to store the value in a mbuf metadata, or pass it as
> > arguments through function calls.
> > 
> >   
> > > 2. Applications would not allowed to change mbuf->port value
> > > before normalization is done (from what I heard some apps do
> > > update mbuf->port to store routing decisions). BTW, how the app
> > > would keep track which mbufs were already normalized, and which
> > > were not?  
> > 
> > I don't think it should be allowed to change m->port value.  
> 
> As far as I know it is allowed right now.
> PMD RX routine sets mbuf->port, after that application is free to use
> it in a way it likes.

The descriptor or m->port is "Input port". If the applications stores
something else than the input port, it is its responsibility if it
breaks something else. Like changing any other field to put something
that does not match the description.


> What we are introducing here is basically a new dependency between 2
> mbuf fields and new restriction. 

On the other hand, there is no strong dependency: the API to do the
normalization can take the port as a parameter.


> 
> 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.

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.


> 
> > 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 worst if you
do a detach + attach.

So, I think it is already the responsibility of the application to do
the sync (flush retrieved packets before detaching a port).

> 
> >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).

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.

Having a non-normalized timestamp as of today would allow applications
to take advantage of it for many use cases, even without the
normalization library that could come later (and that may probably
be more complex than expected).


Olivier