Hi Olivier,

I wanted to retest the patch on latest main, but it no longer applies, could you please rebase it?

Thanks,
Ali

-----Original Message-----
From: Morten Brørup <redacted>
Sent: Tuesday, September 28, 2021 12:40 PM
To: Slava Ovsiienko <redacted>; NBU-Contact-Thomas
Monjalon [off-list ref]; Olivier Matz [off-list ref];
Ali Alnubani [off-list ref]
Cc: dev@dpdk.org; David Marchand <redacted>; Alexander
Kozyrev [off-list ref]; Ferruh Yigit [off-list ref];
zhaoyan.chen@intel.com; Andrew Rybchenko
[off-list ref]; Ananyev, Konstantin
[off-list ref]; Ajit Khaparde
[off-list ref]; jerinj@marvell.com
Subject: RE: [dpdk-dev] [dpdk-stable] [PATCH v4] mbuf: fix reset on mbuf free

quoted

From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Slava Ovsiienko
Sent: Tuesday, 28 September 2021 11.01

Hi,

I've re-read the entire thread.
If I understand correctly, the root problem was (in initial patch):

quoted

  m1 = rte_pktmbuf_alloc(mp);
  rte_pktmbuf_append(m1, 500);
  m2 = rte_pktmbuf_alloc(mp);
  rte_pktmbuf_append(m2, 500);
  rte_pktmbuf_chain(m1, m2);
  m0 = rte_pktmbuf_alloc(mp);
  rte_pktmbuf_append(m0, 500);
  rte_pktmbuf_chain(m0, m1);

As rte_pktmbuf_chain() does not reset nb_seg in the initial m1

segment

quoted

(this is not required), after this code the mbuf chain have 3
segments:
  - m0: next=m1, nb_seg=3
  - m1: next=m2, nb_seg=2
  - m2: next=NULL, nb_seg=1

The proposed fix was to ALWAYS set next and nb_seg fields on
mbuf_free(), regardless next field content. That would perform
unconditional write to mbuf, and might affect the configurations,
where are no multi- segment packets at al. mbuf_free() is "backbone"
API, it is used by all cases, all scenaries are affected.

As far as I know, the current approach for nb_seg field - it contains
other value than 1 only in the first mbuf , for the following
segments,  it should not be considered at all (only the first segment
fields are valid), and it is supposed to contain 1, as it was
initially allocated from the pool.

In the example above the problem was introduced by
rte_pktmbuf_chain(). Could we consider fixing the rte_pktmbuf_chain()
(used in potentially fewer common sceneries)  instead of touching the
extremely common rte_mbuf_free() ?

With best regards,
Slava

Great idea, Slava!

Changing the invariant for 'nb_segs', so it must be 1, except in the first segment
of a segmented packet.

Thinking further about it, perhaps we can achieve even higher performance by a
minor additional modification: Use 0 instead of 1? Or offset 'nb_segs' by -1, so it
reflects the number of additional segments?

And perhaps combining the invariants for 'nb_segs' and 'next' could provide even
more performance improvements. I don't know, just sharing a thought.

Anyway, I vote for fixing the bug. One way or the other!

-Morten

quoted

quoted

-----Original Message-----
From: Thomas Monjalon <redacted>
Sent: Tuesday, September 28, 2021 11:29

Follow-up again:
We have added a note in 21.08, we should fix it in 21.11.
If there are no counter proposal, I suggest applying this patch, no

matter the

quoted

performance regression.


30/07/2021 16:54, Thomas Monjalon:

quoted

30/07/2021 16:35, Morten Brørup:

quoted

quoted

From: Olivier Matz [mailto:olivier.matz@6wind.com]
Sent: Friday, 30 July 2021 14.37

Hi Thomas,

On Sat, Jul 24, 2021 at 10:47:34AM +0200, Thomas Monjalon

wrote:

quoted

quoted

quoted

quoted

quoted

What's the follow-up for this patch?

Unfortunatly, I still don't have the time to work on this
topic

yet.

quoted

quoted

quoted

quoted

In my initial tests, in our lab, I didn't notice any

performance

quoted

quoted

quoted

quoted

regression, but Ali has seen an impact (0.5M PPS, but I don't

know

quoted

quoted

quoted

quoted

how much in percent).

quoted

19/01/2021 15:04, Slava Ovsiienko:

quoted

Hi, All

Could we postpose this patch at least to rc2? We would
like

to

quoted

quoted

quoted

quoted

conduct more investigations?

quoted

quoted

With best regards, Slava

From: Olivier Matz <redacted>

quoted

On Mon, Jan 18, 2021 at 05:52:32PM +0000, Ali Alnubani

wrote:

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

Hi,
(Sorry had to resend this to some recipients due to

mail

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

server

problems).

quoted

quoted

quoted

quoted

Just confirming that I can still reproduce the

regression

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

with

single core and

quoted

quoted

quoted

64B frames on other servers.

Many thanks for the feedback. Can you please detail what

is

quoted

quoted

quoted

quoted

quoted

quoted

quoted

the

amount of

quoted

quoted

quoted

performance loss in percent, and confirm the test case?

(I

quoted

quoted

quoted

quoted

suppose it is

quoted

quoted

quoted

testpmd io forward).

Unfortunatly, I won't be able to spend a lot of time on

this

quoted

quoted

quoted

quoted

quoted

quoted

quoted

soon

(sorry for

quoted

quoted

quoted

that). So I see at least these 2 options:

- postpone the patch again, until I can find more time
to

analyze

quoted

quoted

quoted

quoted

quoted

quoted

quoted

  and optimize
- apply the patch if the performance loss is acceptable
compared

to

quoted

quoted

quoted

  the added value of fixing a bug

[...]

Statu quo...

Olivier

The decision should be simple:

Does the DPDK project support segmented packets?
If yes, then apply the patch to fix the bug!

If anyone seriously cares about the regression it introduces,

optimization

quoted

patches are welcome later. We shouldn't wait for it.

quoted

You're right, but the regression is flagged to a 4-years old
patch, that's why I don't consider it as urgent.

quoted

If the patch is not applied, the documentation must be updated
to

mention that we are releasing DPDK with a known bug: that segmented
packets are handled incorrectly in the scenario described in this

patch.

quoted

quoted

Yes, would be good to document the known issue, no matter how old

it

quoted

quoted

is.

quoted

Generally, there could be some performance to gain by not

supporting

quoted

segmented packets at all, as a compile time option. But that is a

different

quoted

discussion.