On Tue, 24 Jan 2017 15:59:08 +0000, Bruce Richardson
[off-list ref] wrote:

On Tue, Jan 24, 2017 at 04:19:25PM +0100, Olivier Matz wrote:

quoted

Based on discussion done in [1], this patchset reorganizes the mbuf.
  

Hi Olivier,

thanks for all the work on this. From a quick scan of the patches, and
the description below, it looks like a good set of changes. Comments
below to see about kick-starting some further discussion about some of
the other changes you propose.

quoted

The main changes are:
- reorder structure to increase vector performance on some non-ia
  platforms.
- add a 64bits timestamp field in the 1st cache line
- m->next, m->nb_segs, and m->refcnt are always initialized for
mbufs in the pool, avoiding the need of setting m->next (located in
the 2nd cache line) in the Rx path for mono-segment packets.
- change port and nb_segs to 16 bits
- move seqn in the 2nd cache line

Things discussed but not done in the patchset:
- move refcnt and nb_segs to the 2nd cache line: many drivers sets
  them in the Rx path, so it could introduce a performance
regression, or it would require to change all the drivers, which is
not an easy task.  

But if we do make this change and update the drivers, some of them
should perform a little better, since they do fewer writes. I don't
think the fastest vector drivers will be affected, since they already
coalesce the writes to these fields with other writes, but others
drivers may well be improved by the change.

Yes, that's something I forgot to say in the cover letter: after this
patchset, the Rx path of drivers could be optimized a bit by removing
writes to m->next, m->nb_segs and m->refcnt. The patch 4/8 gives an
idea of what could be done.

Once most drivers are updated, we could reconsider moving nb_segs and
refcnt in the second cache line.

quoted

- remove the m->port field: too much impact on many examples and
libraries, and some people highlighted they are using it.
- moving m->next in the 1st cache line: there is not enough room,
and having it set to NULL for unused mbuf should remove the need
for it.  

I agree.

quoted

- merge seqn and timestamp together in a union: we could imagine
use cases were both are activated. There is no flag indicating the
presence of seqn, so it looks preferable to keep them separated for
now.  

What were the use-cases? If we have a timestamp, surely sequence can
be determined from that? Even if you use the TSC as a timestamp per
burst, you can still sequence the packets cheaply by just adding 1 to
each subsequent value.

Assuming the timestamp is in nanosecond, it is not a sequence number,
so I'm not sure it should be hijacked for this purpose. A timestamp can
be used to reorder packets, but having a sequence number is better
because you can be sure that when you get packets 1, 3, 2, 0 that no
packet is missing between 0 and 3.

For that reason, I guess both features could be used at the same time.

Regards,
Olivier