On Thu, Mar 21, 2024 at 05:08:06PM -0400, Mike Pattrick wrote:
> This patch adopts the proposed RFC 6935 by allowing null UDP checksums
> even if the tunnel protocol is IPv6. This is already supported by Linux
> through the udp6zerocsumtx tunnel option. It is disabled by default and
> IPv6 tunnels are flagged as requiring a checksum, but this patch enables
> the user to set csum=false on IPv6 tunnels.
>
> Signed-off-by: Mike Pattrick <[email protected]>
> ---
> v2: Changed documentation, and added a NEWS item
> v3: NEWS file merge conflict
> v4: Better comments, new test
Thanks Mike,
FWIIW I believe v4 addresses Ilya's review of v3.
Please find some minor comments from my side below.
...
> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
...
> @@ -850,6 +852,15 @@ set_tunnel_config(struct netdev *dev_, const struct smap
> *args, char **errp)
> }
> }
>
> + /* The default csum state for GRE is special as it does have an optional
> + * checksum but the default configuration isn't correlated with IP
> version
> + * like UDP tunnels are. Likewise, tunnels with checksum at all must be
> in
> + * this state. */
nit: with checksum -> with no checksum
> + if (tnl_cfg.csum == NETDEV_TNL_CSUM_DEFAULT &&
> + (!has_csum || strstr(type, "gre"))) {
> + tnl_cfg.csum = NETDEV_TNL_DEFAULT_NO_CSUM;
> + }
> +
> enum tunnel_layers layers = tunnel_supported_layers(type, &tnl_cfg);
> const char *full_type = (strcmp(type, "vxlan") ? type
> : (tnl_cfg.exts & (1 << OVS_VXLAN_EXT_GPE)
...
> diff --git a/ofproto/tunnel.c b/ofproto/tunnel.c
> index 80ddee78a..6f462874e 100644
> --- a/ofproto/tunnel.c
> +++ b/ofproto/tunnel.c
> @@ -465,9 +465,14 @@ tnl_port_send(const struct ofport_dpif *ofport, struct
> flow *flow,
>
> flow->tunnel.flags &= ~(FLOW_TNL_F_MASK & ~FLOW_TNL_PUB_F_MASK);
> flow->tunnel.flags |= (cfg->dont_fragment ? FLOW_TNL_F_DONT_FRAGMENT : 0)
> - | (cfg->csum ? FLOW_TNL_F_CSUM : 0)
> | (cfg->out_key_present ? FLOW_TNL_F_KEY : 0);
>
> + if (cfg->csum == NETDEV_TNL_CSUM_ENABLED) {
> + flow->tunnel.flags |= FLOW_TNL_F_CSUM;
> + } else if (cfg->csum == NETDEV_TNL_CSUM_DEFAULT && !flow->tunnel.ip_dst)
> {
> + flow->tunnel.flags |= FLOW_TNL_F_CSUM;
> + }
> +
nit: As the action is the same (or alternatively my eyes deceive me)
for both arms of the condition above, I would have written it as
(completely untested!):
if (cfg->csum == NETDEV_TNL_CSUM_ENABLED ||
(cfg->csum == NETDEV_TNL_CSUM_DEFAULT && !flow->tunnel.ip_dst)) {
flow->tunnel.flags |= FLOW_TNL_F_CSUM;
}
> if (cfg->set_egress_pkt_mark) {
> flow->pkt_mark = cfg->egress_pkt_mark;
> wc->masks.pkt_mark = UINT32_MAX;
...
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index 8a1b607d7..5c3aa2a2a 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -3207,9 +3207,15 @@
> <column name="options" key="csum" type='{"type": "boolean"}'>
> <p>
> Optional. Compute encapsulation header (either GRE or UDP)
> - checksums on outgoing packets. Default is disabled, set to
> - <code>true</code> to enable. Checksums present on incoming
> - packets will be validated regardless of this setting.
> + checksums on outgoing packets. When unset (the default value),
> + checksum computing for outgoing packets is enabled for UDP IPv6
> + tunnels, and disabled for IPv4 UDP and GRE tunnels. When set to
nit: I'm assuming this applies to both IPv4 and IPv6 GRE tunnels.
If so perhaps the following is slightly clearer.
tunnels, and disabled for GRE and IPv4 UDP tunnels. When set to
> + <code>false</code>, no checksums will be computed for outgoing
> + tunnel encapsulation headers. When <code>true</code>, checksums
> + will be computed for all outgoing tunnel encapsulation headers.
> + Checksums present on incoming packets will be validated
> + regardless of this setting. Incoming packets without a checksum
> + will also be accepted regardless of this setting.
> </p>
>
> <p>
...
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
