Skip to content

Conversation

@rjarry
Copy link
Collaborator

@rjarry rjarry commented Jun 25, 2025

In order to support receiving packets larger than 2048 bytes, we need to enable multi-segment rx and tx offload in the drivers.

Enable RTE_ETH_RX_OFFLOAD_SCATTER and RTE_ETH_TX_OFFLOAD_MULTI_SEGS in the requested offload flags when configuring DPDK ports.

Update the basic IP forward test to check we are indeed receiving 5000 bytes packets with a configured MTU of 9000 bytes.

Suggested-by: David Marchand [email protected]

@rjarry rjarry requested a review from david-marchand June 25, 2025 20:49
@rjarry rjarry marked this pull request as ready for review June 25, 2025 20:50
return errno_log(-ret, "rte_eth_dev_info_get");

if (strcmp(info.driver_name, "net_tap") == 0) {
p->n_txq = RTE_MAX(p->n_txq, p->n_rxq);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This patch has nothing to do with the series about larger packets.

In any case, I am not following.
Why remove this part above?

With the previous code, if a user requests more rxq, this would result in unneeded txq, but I don't see a problem with this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll drop this patch for now.

Copy link
Member

@david-marchand david-marchand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last patch lgtm.

In order to support receiving packets larger than 2048 bytes, we need to
enable multi-segment rx and tx offload in the drivers.

Enable RTE_ETH_RX_OFFLOAD_SCATTER and RTE_ETH_TX_OFFLOAD_MULTI_SEGS in
the requested offload flags when configuring DPDK ports.

Update the basic IP forward test to check we are indeed receiving 5000
bytes packets with a configured MTU of 9000 bytes.

Suggested-by: David Marchand <[email protected]>
Signed-off-by: Robin Jarry <[email protected]>
@david-marchand
Copy link
Member

Just a note that accepting larger frames is fine, but there is nothing in grout atm for fragmenting in case of heterogenous mtu interfaces.

@rjarry
Copy link
Collaborator Author

rjarry commented Jun 26, 2025

yep, we don't even check for MTU when sending packets.

@david-marchand
Copy link
Member

Yes, that's part of what I had in mind when commenting.

Copy link
Collaborator

@maxime-leroy maxime-leroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be very careful on that.

Most of code doing operation on mbuf supposes there is only one segment, data are contiguous.

Most of time network header will be in first segment, but anyway to have safe code if we active mutli segs, you can not anymore present that all data are in the first segments.

@david-marchand
Copy link
Member

Use of rte_pktmbuf_read would be the safest, but blindly adding checks would impact performance.

If multiseg scares you, another approach is to allow bigger frames in the packet pool (for which there is no configuration knob atm).

@rjarry rjarry merged commit 62071f3 into DPDK:main Jun 26, 2025
8 checks passed
@rjarry rjarry deleted the multiseg branch June 26, 2025 09:06
@maxime-leroy
Copy link
Collaborator

Use of rte_pktmbuf_read would be the safest, but blindly adding checks would impact performance.

If multiseg scares you, another approach is to allow bigger frames in the packet pool (for which there is no configuration knob atm).
rte_pktmbuf_read is just for read. You need to modify the packet, when grout doing memmove in a pktmbuf data without knowing if data is spread on several segment, it's just a dangerous.

Of course, we can use jumbo mbuf to support jumbo packet. The side effect is increased memory consumption.

Else it's to support multi segs. In this case, doing the check about multi segments when writing a mbuf or reading this one must be done. Of course, the side effect is an extra cost for cpu.

Nothing is free, you must choose if you want to increase memory cpu cycles or cpu cost to support jumbo frames.

Just enabling the feature without wondering if some operations done on muf are safe or not with multiseg is the perfect way to get crash.

I am against this commit, It should not have been merged without answering properly about my point.

Prove me that all operations are safe in grout with multi segments or else fix it.

@rjarry
Copy link
Collaborator Author

rjarry commented Jun 26, 2025

Just enabling the feature without wondering if some operations done on muf are safe or not with multiseg is the perfect way to get crash.

I am against this commit, It should not have been merged without answering properly about my point.

Prove me that all operations are safe in grout with multi segments or else fix it.

Operations aren't safe for sure.

I don't have any specific preference. The only thing I want is for jumbo MTUs to work.

We could use bigger MBUFs. Is there any downside to this besides high memory consumption?

@maxime-leroy
Copy link
Collaborator

Just enabling the feature without wondering if some operations done on muf are safe or not with multiseg is the perfect way to get crash.
I am against this commit, It should not have been merged without answering properly about my point.
Prove me that all operations are safe in grout with multi segments or else fix it.

Operations aren't safe for sure.

I don't have any specific preference. The only thing I want is for jumbo MTUs to work.

We could use bigger MBUFs. Is there any downside to this besides high memory consumption?

This commits will drop performance (at least > 10% au doigt mouillé). Don't have time to do the test. But by enabling scatter in rx and multi segment in tx, you use less performance function of driver for RX/TX (i.e. vector vs non vector).

By using jumbo mbuf, you don't have the problem. I don't see any sideeffect, except the increaze of memory consumption. We should do the math to see if it's acceptable or not. Maybe an option --jumbo could be added at grout. To not force any one to have these big mbuf.

At least, if we need multi segments, we should enable only when mtu is > mbuf headroom. To keep the best performance for standard mtu.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants