Re: [RFC 0/8] mbuf: structure reorganization

On Fri, Feb 24, 2017 at 03:00:53PM +0100, Olivier Matz wrote:

Hi,

On Tue, 21 Feb 2017 20:30:57 +0000, "Ananyev, Konstantin"
[off-list ref] wrote:

quoted

quoted

-----Original Message-----
From: jblunck@gmail.com [mailto:jblunck@gmail.com] On Behalf Of Jan
Blunck Sent: Tuesday, February 21, 2017 7:18 PM
To: Ananyev, Konstantin <redacted>
Cc: Richardson, Bruce <redacted>; Olivier MATZ
[off-list ref]; dev@dpdk.org Subject: Re: [dpdk-dev] [RFC
0/8] mbuf: structure reorganization

On Tue, Feb 21, 2017 at 6:26 PM, Ananyev, Konstantin
[off-list ref] wrote:  

quoted

Hi Jan,
 

quoted

-----Original Message-----
From: jblunck@gmail.com [mailto:jblunck@gmail.com] On Behalf Of
Jan Blunck Sent: Tuesday, February 21, 2017 5:05 PM
To: Richardson, Bruce <redacted>
Cc: Olivier MATZ <redacted>; Ananyev, Konstantin
[off-list ref]; dev@dpdk.org Subject: Re:
[dpdk-dev] [RFC 0/8] mbuf: structure reorganization

On Tue, Feb 21, 2017 at 5:38 PM, Bruce Richardson
[off-list ref] wrote:  

quoted

On Tue, Feb 21, 2017 at 05:12:12PM +0100, Jan Blunck wrote:  

quoted

Access through PMD specific function pointers should be
relatively fast on access. Modern architecture optimize that
use case well enough.
 

The cost of doing a function call per packet to access data
starts to add up very, very fast. For the app, once the data
is written to the mbuf, it should be in the L1 cache, giving
very fast access to it in a few cycles. However, if a function
call has to be made in order to do the read, that makes the
read of that field many times more expensive. 

Exactly. Right now the timestamp normalization is done before
writing to each mbuf. Timestamps are usually read at most
once ... if at all.  

Well we don't know for sure right?
Someone can argue that there are plenty of scenarios  when
other fields might also never be used/updated (rss, vlan, etc).

So, are you suggesting to do normalization later?
If so, then what would be the benefit (data still need to be in
mbuf)?  

Yes, postponing normalization prevents you from doing unnecessary
work upfront. AFAIK not all NICs store timestamp data OOB, e.g. in
CQ.  

Yes, postponing normalization might help a bit (though I don't think
much) in terms of calculations performed inside PMD.
But we still need 8B inside mbuf to store the timestamp value,
either normalized or raw one.
So to clarify where is the disagreement:
1. timestamp position:
    mbufs 1-st cacheline vs 2-nd cacheline

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.

quoted

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.


About having the timestamp in the packet data, I don't think it is
a good solution for a generic API in DPDK. The timestamp is a metadata,
it has to go in the mbuf metadata. The packet data should not be
modified when the timestamp is enabled.

But this would not prevent to have driver-specific features to do that.
In that case, the application will be aware that it is using this
specific driver and that it will receive a timestamp in the packet data.

To summarize, the generic API could be:
- an ethdev API to enable the timestamp in a PMD for received packets
- a mbuf flag "timestamp present"
- a mbuf 64b field to store the timestamp value

Additionally, a driver-specific API can be added for a given PMD.
Example:
- the generic timestamp ethdev is disabled (or not supported)
- a driver-specific feature "put timestamp in packet" is enabled
It would have no additional cost compared to what we have today, since
the timestamp in mbuf is not read/written.

All seems reasonable to me.
/Bruce