From: Cong Wang <hidden> Date: 2018-12-04 07:30:01
On Mon, Dec 3, 2018 at 11:08 PM Eric Dumazet [off-list ref] wrote:
The hardware has probably validated the L3 & L4 checksum just fine.
Note that if ip_summed is CHECKSUM_UNNECESSARY, the padding bytes (if any)
have no impact on the csum that has been verified by the NIC.
Why? Why does the hardware validates L3/L4 checksum when it
supplies a full-packet checksum? What's its point here?
If it really validates L3/L4 checksum, then a full-packet checksum
is not needed.
If a full-packet checksum is supplied, the software is able to use
it to validate L3/L4 checksum, then the hardware doesn't need to
validate it.
I see no reason it provides both at the same time. If it really does,
then all CHECKSUM_COMPLETE code here could be just removed
and would be faster.
Something must be wrong with your argument.
From: Eric Dumazet <edumazet@google.com> Date: 2018-12-04 07:51:55
On Mon, Dec 3, 2018 at 11:30 PM Cong Wang [off-list ref] wrote:
On Mon, Dec 3, 2018 at 11:08 PM Eric Dumazet [off-list ref] wrote:
quoted
The hardware has probably validated the L3 & L4 checksum just fine.
Note that if ip_summed is CHECKSUM_UNNECESSARY, the padding bytes (if any)
have no impact on the csum that has been verified by the NIC.
Why? Why does the hardware validates L3/L4 checksum when it
supplies a full-packet checksum? What's its point here?
The point is that the driver author can decide what is best.
For native IP+TCP or IP+UDP, the NIC has the ability to fully
understand the packet and fully validate the checksum.
If it really validates L3/L4 checksum, then a full-packet checksum
is not needed.
Yes, this is exactly what CHECKSUM_UNNECESSARY means.
linux stack does not have to perform the check another time.
For example, no call to csum_partial() is needed, even for IPv6+TCP or IPv6+UDP
On Mon, Dec 3, 2018 at 11:52 PM Eric Dumazet [off-list ref] wrote:
On Mon, Dec 3, 2018 at 11:30 PM Cong Wang [off-list ref] wrote:
quoted
On Mon, Dec 3, 2018 at 11:08 PM Eric Dumazet [off-list ref] wrote:
quoted
The hardware has probably validated the L3 & L4 checksum just fine.
Note that if ip_summed is CHECKSUM_UNNECESSARY, the padding bytes (if any)
have no impact on the csum that has been verified by the NIC.
Why? Why does the hardware validates L3/L4 checksum when it
supplies a full-packet checksum? What's its point here?
The point is that the driver author can decide what is best.
For native IP+TCP or IP+UDP, the NIC has the ability to fully
understand the packet and fully validate the checksum.
Also for Native IP4 and IP6 plain L3 packets.
The Hardware validates the csum when it can, and always provides
checksum complete for all packets.
One of the reason to validate is that sometimes we want to skip
checksum complete, but still leverage the hw validation,
like in your patch :), or LRO case, or many other cases in other
kernels/OSes/drivers.
So i agree with Eric, let's jump to checksum_unnecessary for short packets.
quoted
If it really validates L3/L4 checksum, then a full-packet checksum
is not needed.
Yes, this is exactly what CHECKSUM_UNNECESSARY means.
linux stack does not have to perform the check another time.
For example, no call to csum_partial() is needed, even for IPv6+TCP or IPv6+UDP
net_device *netdev,
return;
}
- if (unlikely(test_bit(MLX5E_RQ_STATE_NO_CSUM_COMPLETE, &rq->state)))
- goto csum_unnecessary;
-
- if (likely(is_last_ethertype_ip(skb, &network_depth, &proto))) {
- if (unlikely(get_ip_proto(skb, network_depth, proto)
== IPPROTO_SCTP))
- goto csum_unnecessary;
-
- skb->ip_summed = CHECKSUM_COMPLETE;
- skb->csum = csum_unfold((__force __sum16)cqe->check_sum);
- if (network_depth > ETH_HLEN)
- /* CQE csum is calculated from the IP header and does
- * not cover VLAN headers (if present). This will add
- * the checksum manually.
- */
- skb->csum = csum_partial(skb->data + ETH_HLEN,
- network_depth - ETH_HLEN,
- skb->csum);
- if (unlikely(netdev->features & NETIF_F_RXFCS))
- skb->csum = csum_block_add(skb->csum,
- (__force
__wsum)mlx5e_get_fcs(skb),
- skb->len - ETH_FCS_LEN);
- stats->csum_complete++;
- return;
- }
-
-csum_unnecessary:
For example, no call to csum_partial() is needed, even for IPv6+TCP or IPv6+UDP
Sure, if you mean to get rid of CHECKSUM_COMPLETE.
Remember you fixed CHECKSUM_COMPLETE too when you touch it,
see d48051c5b837 ("net/mlx5e: fix csum adjustments caused by RXFCS").
So why didn't you remove CHECKSUM_COMPLETE but fixed it instead?
Thanks.
From: Cong Wang <hidden> Date: 2018-12-04 20:35:46
On Tue, Dec 4, 2018 at 11:17 AM Saeed Mahameed
[off-list ref] wrote:
On Mon, Dec 3, 2018 at 11:52 PM Eric Dumazet [off-list ref] wrote:
quoted
On Mon, Dec 3, 2018 at 11:30 PM Cong Wang [off-list ref] wrote:
quoted
On Mon, Dec 3, 2018 at 11:08 PM Eric Dumazet [off-list ref] wrote:
quoted
The hardware has probably validated the L3 & L4 checksum just fine.
Note that if ip_summed is CHECKSUM_UNNECESSARY, the padding bytes (if any)
have no impact on the csum that has been verified by the NIC.
Why? Why does the hardware validates L3/L4 checksum when it
supplies a full-packet checksum? What's its point here?
The point is that the driver author can decide what is best.
For native IP+TCP or IP+UDP, the NIC has the ability to fully
understand the packet and fully validate the checksum.
Also for Native IP4 and IP6 plain L3 packets.
The Hardware validates the csum when it can, and always provides
checksum complete for all packets.
One of the reason to validate is that sometimes we want to skip
checksum complete, but still leverage the hw validation,
like in your patch :), or LRO case, or many other cases in other
kernels/OSes/drivers.
This sounds wrong to me too.
If the HW already validates it, the software doesn't need to do it,
therefore must skip hw csum for performance gain.,
So i agree with Eric, let's jump to checksum_unnecessary for short packets.
This is odd, if Eric is right, then we should completely get rid of
CHECKSUM_COMPLETE. Short packets are not exceptions.
I still don't understand why people including Eric kept fixing this
thing which could be just removed from the very beginning.
Sounds like nobody even looked into it until my patch.
Thanks.
From: Eric Dumazet <hidden> Date: 2018-12-04 21:16:39
On 12/04/2018 12:35 PM, Cong Wang wrote:
On Tue, Dec 4, 2018 at 11:17 AM Saeed Mahameed
[off-list ref] wrote:
quoted
On Mon, Dec 3, 2018 at 11:52 PM Eric Dumazet [off-list ref] wrote:
quoted
On Mon, Dec 3, 2018 at 11:30 PM Cong Wang [off-list ref] wrote:
quoted
On Mon, Dec 3, 2018 at 11:08 PM Eric Dumazet [off-list ref] wrote:
quoted
The hardware has probably validated the L3 & L4 checksum just fine.
Note that if ip_summed is CHECKSUM_UNNECESSARY, the padding bytes (if any)
have no impact on the csum that has been verified by the NIC.
Why? Why does the hardware validates L3/L4 checksum when it
supplies a full-packet checksum? What's its point here?
The point is that the driver author can decide what is best.
For native IP+TCP or IP+UDP, the NIC has the ability to fully
understand the packet and fully validate the checksum.
Also for Native IP4 and IP6 plain L3 packets.
The Hardware validates the csum when it can, and always provides
checksum complete for all packets.
One of the reason to validate is that sometimes we want to skip
checksum complete, but still leverage the hw validation,
like in your patch :), or LRO case, or many other cases in other
kernels/OSes/drivers.
This sounds wrong to me too.
If the HW already validates it, the software doesn't need to do it,
therefore must skip hw csum for performance gain.,
quoted
So i agree with Eric, let's jump to checksum_unnecessary for short packets.
This is odd, if Eric is right, then we should completely get rid of
CHECKSUM_COMPLETE. Short packets are not exceptions.
I still don't understand why people including Eric kept fixing this
thing which could be just removed from the very beginning.
Sounds like nobody even looked into it until my patch.
Erm I never suggested to get rid of CHECKSUM_COMPLETE...
My suggestion was to reorder the mlx5 logic to match mlx4 one.
CHECKSUM_COMPLETE is very nice _when_/_if_ the NIC is unable to
fully dissect a packet and validate L4, as a fallback.
I am pretty sure for example that IP reassembly can benefit from CHECKSUM_COMPLETE.
(Although for some reason mlx4 code does not handle IPv6 fragments in its CHECKSUM_COMPLETE path)
From: Cong Wang <hidden> Date: 2018-12-04 21:21:10
On Tue, Dec 4, 2018 at 1:16 PM Eric Dumazet [off-list ref] wrote:
Erm I never suggested to get rid of CHECKSUM_COMPLETE...
My suggestion was to reorder the mlx5 logic to match mlx4 one.
CHECKSUM_COMPLETE is very nice _when_/_if_ the NIC is unable to
fully dissect a packet and validate L4, as a fallback.
Quote from Eric:
"For native IP+TCP or IP+UDP, the NIC has the ability to fully
understand the packet and fully validate the checksum."
Therefore CHECKSUM_COMPLETE is not nice here.
I am pretty sure for example that IP reassembly can benefit from CHECKSUM_COMPLETE.
(Although for some reason mlx4 code does not handle IPv6 fragments in its CHECKSUM_COMPLETE path)
I am pretty sure mlx5 driver code doesn't check for IP fragments.
My patch is dropped, let's keep the current code as it is and pretend
everything just works fine.
Thanks a lot!
On Tue, 2018-12-04 at 12:35 -0800, Cong Wang wrote:
On Tue, Dec 4, 2018 at 11:17 AM Saeed Mahameed
[off-list ref] wrote:
quoted
On Mon, Dec 3, 2018 at 11:52 PM Eric Dumazet [off-list ref]
wrote:
quoted
On Mon, Dec 3, 2018 at 11:30 PM Cong Wang <
xiyou.wangcong@gmail.com> wrote:
quoted
On Mon, Dec 3, 2018 at 11:08 PM Eric Dumazet <
edumazet@google.com> wrote:
quoted
The hardware has probably validated the L3 & L4 checksum just
fine.
Note that if ip_summed is CHECKSUM_UNNECESSARY, the padding
bytes (if any)
have no impact on the csum that has been verified by the NIC.
Why? Why does the hardware validates L3/L4 checksum when it
supplies a full-packet checksum? What's its point here?
The point is that the driver author can decide what is best.
For native IP+TCP or IP+UDP, the NIC has the ability to fully
understand the packet and fully validate the checksum.
Also for Native IP4 and IP6 plain L3 packets.
The Hardware validates the csum when it can, and always provides
checksum complete for all packets.
One of the reason to validate is that sometimes we want to skip
checksum complete, but still leverage the hw validation,
like in your patch :), or LRO case, or many other cases in other
kernels/OSes/drivers.
This sounds wrong to me too.
If the HW already validates it, the software doesn't need to do it,
therefore must skip hw csum for performance gain.,
Cong, you are missing some important details, hardware can only parse a
handful of protocols IP/TCP/UDP some tunneling like vxlan,GRE, etc..
but for complicated protocols and new generic tunneling protocols,
which the hardware wasn't built to understand, the checksum complete
comes to the rescue.
not only that, imagine you are doing some proprietary tunneling and you
want to encapsulate the rx traffic and send it back to wire, how would
you want to recalculate the csum before txing ? manually on the whole
new packet or use the csum complete and just figure out the differences
? i am sure you gonna want to use the checksum complete of the original
packet.
So again csum complete is not only for validation, it is more powerful
than that.
quoted
So i agree with Eric, let's jump to checksum_unnecessary for short
packets.
This is odd, if Eric is right, then we should completely get rid of
CHECKSUM_COMPLETE. Short packets are not exceptions.
short packets are exception,
1. because of the small packet padding issue
2. because they are short enough not to feel the performance hit of
recalculating their whole csum (if necessary) .
Again still it is nice to report csum unnecessary for those packets, if
possible.
for large packets csum complete is favorable because of the above
reasons.
if you don't like checksum complete you have a way to totally disable
it in mlx5 via ethtool private flags.
I hope this helps.
I still don't understand why people including Eric kept fixing this
thing which could be just removed from the very beginning.
Sounds like nobody even looked into it until my patch.
Thanks.
From: Cong Wang <hidden> Date: 2018-12-05 02:48:34
On Tue, Dec 4, 2018 at 4:59 PM Saeed Mahameed [off-list ref] wrote:
Cong, you are missing some important details, hardware can only parse a
handful of protocols IP/TCP/UDP some tunneling like vxlan,GRE, etc..
but for complicated protocols and new generic tunneling protocols,
which the hardware wasn't built to understand, the checksum complete
comes to the rescue.
not only that, imagine you are doing some proprietary tunneling and you
want to encapsulate the rx traffic and send it back to wire, how would
you want to recalculate the csum before txing ? manually on the whole
new packet or use the csum complete and just figure out the differences
? i am sure you gonna want to use the checksum complete of the original
packet.
So again csum complete is not only for validation, it is more powerful
than that.
OK, so how do you detect this? Clearly not by just checking skb->len,
right?
Let me show you why your arguments are non-sense.
1. From your description of how mlx5 hardware works, the driver logic
must be something like this:
if (hardware can checksum L4)
use CHECKSUM_UNNECESSARY
else
use CHECKSUM_COMPLETE
This obviously makes sense. Does the current code base match this?
No. The current code base only checks for IP/IPv6 and sets
CHECKSUM_COMPLETE (unless SCTP), far from matching what
you are trying to sell.
2. What you and Eric suggest to change to:
if (the frame is short)
use CHECKSUM_UNNECESSARY
else
use CHECKSUM_COMPLETE
Does this match what you describe? No, because short frame does not
necessarily mean hardware can't check its L4 checksum. It only misleads
people to believe so, it helps nothing.
3. What my patch actually does:
if (the frame is short)
use CHECKSUM_NONE
else
use CHECKSUM_COMPLETE
Why this makes sense? Because when skb->csum is not correct,
treating it as none is reasonable. And, checking skb->len doesn't
have indication of whether hardware can checksum L4 checksum
or not. This is _good_ when the rest code remains unchanged,
we have no clue whether hardware has done L4 checksum validation
or not, therefore choose to pass it to software to validate.
Of course you can eventually change all the code to match what
you are describing, but _until_ that happens CHECKSUM_NONE
makes more sense than CHECKSUM_UNNECESSARY here.
quoted
quoted
So i agree with Eric, let's jump to checksum_unnecessary for short
packets.
This is odd, if Eric is right, then we should completely get rid of
CHECKSUM_COMPLETE. Short packets are not exceptions.
short packets are exception,
1. because of the small packet padding issue
Why padding issue makes it CHECKSUM_UNNECESSARY not
CHECKSUM_NONE? Isn't the hardware's capability what makes the
difference?
2. because they are short enough not to feel the performance hit of
recalculating their whole csum (if necessary) .
With CHECKSUM_UNNECESSARY, checksum is not recalculated
or validated at L4.
Again still it is nice to report csum unnecessary for those packets, if
possible.
Please define "if possible". You are hiding a lot of things behind
"if possible" to favor your own argument.
for large packets csum complete is favorable because of the above
reasons.
Only if hardware can checksum all large packets.
if you don't like checksum complete you have a way to totally disable
it in mlx5 via ethtool private flags.
Thanks but we are very happy to carry the fix locally.
This is why I stop arguing with this non-sense. Why not we all stop
here and agree to an disagreement? Why are we wasting our time?
Thanks!