* Re: [PATCH] net/virtio: include ipv4 cksum to support cksum offload capability
2022-01-07 11:53 [PATCH] net/virtio: include ipv4 cksum to support cksum offload capability Harold Huang
@ 2022-01-31 10:03 ` Maxime Coquelin
2022-02-07 2:54 ` Harold Huang
2022-02-15 20:08 ` David Marchand
1 sibling, 1 reply; 4+ messages in thread
From: Maxime Coquelin @ 2022-01-31 10:03 UTC (permalink / raw)
To: Harold Huang, dev; +Cc: Chenbo Xia
Hi Harold,
On 1/7/22 12:53, Harold Huang wrote:
> Device cksum offload capability usually include ipv4 cksum, tcp and udp
> cksum offload capability. The application such as OVS usually negotiate
> with the drive like this to enable cksum offload.
>
> Signed-off-by: Harold Huang <baymaxhuang@gmail.com>
> ---
> drivers/net/virtio/virtio_ethdev.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index c2588369b2..65b03bf0e4 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -3041,6 +3041,7 @@ virtio_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
> dev_info->rx_offload_capa |= RTE_ETH_RX_OFFLOAD_SCATTER;
> if (host_features & (1ULL << VIRTIO_NET_F_GUEST_CSUM)) {
> dev_info->rx_offload_capa |=
> + RTE_ETH_RX_OFFLOAD_IPV4_CKSUM |
> RTE_ETH_RX_OFFLOAD_TCP_CKSUM |
> RTE_ETH_RX_OFFLOAD_UDP_CKSUM;
> }
> @@ -3055,6 +3056,7 @@ virtio_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
> RTE_ETH_TX_OFFLOAD_VLAN_INSERT;
> if (host_features & (1ULL << VIRTIO_NET_F_CSUM)) {
> dev_info->tx_offload_capa |=
> + RTE_ETH_TX_OFFLOAD_IPV4_CKSUM |
> RTE_ETH_TX_OFFLOAD_UDP_CKSUM |
> RTE_ETH_TX_OFFLOAD_TCP_CKSUM;
> }
I'm not sure to understand why this is needed, as Vhost lib will always
ensure the IP csum has been calculated. Could you please elaborate?
Thanks,
Maxime
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] net/virtio: include ipv4 cksum to support cksum offload capability
2022-01-07 11:53 [PATCH] net/virtio: include ipv4 cksum to support cksum offload capability Harold Huang
2022-01-31 10:03 ` Maxime Coquelin
@ 2022-02-15 20:08 ` David Marchand
1 sibling, 0 replies; 4+ messages in thread
From: David Marchand @ 2022-02-15 20:08 UTC (permalink / raw)
To: Harold Huang, Olivier Matz; +Cc: dev, Maxime Coquelin, Chenbo Xia
Adding Olivier who, iirc, worked on those offloads a long time ago.
On Fri, Jan 7, 2022 at 12:54 PM Harold Huang <baymaxhuang@gmail.com> wrote:
>
> Device cksum offload capability usually include ipv4 cksum, tcp and udp
> cksum offload capability. The application such as OVS usually negotiate
> with the drive like this to enable cksum offload.
OVS checksum offloading is still being worked on.
By this, I mean neither that it is fully functional nor completely broken.
But important to keep in mind that it is still a work in progress.
>
> Signed-off-by: Harold Huang <baymaxhuang@gmail.com>
> ---
> drivers/net/virtio/virtio_ethdev.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index c2588369b2..65b03bf0e4 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -3041,6 +3041,7 @@ virtio_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
> dev_info->rx_offload_capa |= RTE_ETH_RX_OFFLOAD_SCATTER;
> if (host_features & (1ULL << VIRTIO_NET_F_GUEST_CSUM)) {
> dev_info->rx_offload_capa |=
> + RTE_ETH_RX_OFFLOAD_IPV4_CKSUM |
> RTE_ETH_RX_OFFLOAD_TCP_CKSUM |
> RTE_ETH_RX_OFFLOAD_UDP_CKSUM;
> }
Adding this capability alone probably has no effect visible to a dpdk
application.
For L3, virtio rx handlers never sets anything but
RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN.
https://git.dpdk.org/dpdk/tree/drivers/net/virtio/virtio_rxtx.c#n920
As a consequence, the application can't tell if the ip checksum is
correct and the application must validate the checksum of packets on
its own.
Maybe we could add some RTE_MBUF_F_RX_IP_CKSUM_GOOD in case of TCP/UDP (?)
https://git.dpdk.org/dpdk/tree/drivers/net/virtio/virtio_rxtx.c#n932
But simply reporting the capability looks wrong to me.
> @@ -3055,6 +3056,7 @@ virtio_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
> RTE_ETH_TX_OFFLOAD_VLAN_INSERT;
> if (host_features & (1ULL << VIRTIO_NET_F_CSUM)) {
> dev_info->tx_offload_capa |=
> + RTE_ETH_TX_OFFLOAD_IPV4_CKSUM |
> RTE_ETH_TX_OFFLOAD_UDP_CKSUM |
> RTE_ETH_TX_OFFLOAD_TCP_CKSUM;
> }
For the tx part, I am a bit unsure, but here is what I understand.
With such a capability, the application may think it can send the
packet with no checksum, neither ip, nor tcp.
The virtio specification writes that the driver must make sure the ip
checksum is correct if it wants to ask for L4 checksum.
"""
If the VIRTIO_NET_F_CSUM feature has been negotiated, the driver MAY
set the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in flags, if so:
the driver MUST validate the packet checksum at offset csum_offset
from csum_start as well as all preceding offsets;
the driver MUST set the packet checksum stored in the buffer to the
TCP/UDP pseudo header;
the driver MUST set csum_start and csum_offset such that calculating a
ones’ complement checksum from csum_start up until the end of the
packet and storing the result at offset csum_offset from csum_start
will result in a fully checksummed packet;
"""
But, nothing in the virtio tx handlers deals with ip cheksum offloading.
https://git.dpdk.org/dpdk/tree/drivers/net/virtio/virtqueue.h#n664
So here too, I think it is wrong to report such a capability.
--
David Marchand
^ permalink raw reply [flat|nested] 4+ messages in thread