On Wed, May 15, 2024 at 2:11 PM Kevin Traynor <ktray...@redhat.com> wrote:
> > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> > index 1dad2ef833..31dd6f1d5a 100644
> > --- a/lib/netdev-dpdk.c
> > +++ b/lib/netdev-dpdk.c
> > @@ -2584,6 +2584,9 @@ static bool
> > netdev_dpdk_prep_hwol_packet(struct netdev_dpdk *dev, struct rte_mbuf
> > *mbuf)
> > {
> > struct dp_packet *pkt = CONTAINER_OF(mbuf, struct dp_packet, mbuf);
> > + void *l2;
> > + void *l3;
> > + void *l4;
> >
> > const uint64_t all_inner_requests = (RTE_MBUF_F_TX_IP_CKSUM |
> > RTE_MBUF_F_TX_L4_MASK |
> > @@ -2613,11 +2616,6 @@ netdev_dpdk_prep_hwol_packet(struct netdev_dpdk
> > *dev, struct rte_mbuf *mbuf)
> > return true;
> > }
> >
> > - ovs_assert(dp_packet_l4(pkt));
> > -
> > - /* If packet is vxlan or geneve tunnel packet, calculate outer
> > - * l2 len and outer l3 len. Inner l2/l3/l4 len are calculated
> > - * before. */
> > const uint64_t tunnel_type = mbuf->ol_flags &
> > RTE_MBUF_F_TX_TUNNEL_MASK;
> > if (OVS_UNLIKELY(tunnel_type &&
> > tunnel_type != RTE_MBUF_F_TX_TUNNEL_GENEVE &&
> > @@ -2635,6 +2633,11 @@ netdev_dpdk_prep_hwol_packet(struct netdev_dpdk
> > *dev, struct rte_mbuf *mbuf)
> > (char *) dp_packet_eth(pkt);
> > mbuf->outer_l3_len = (char *) dp_packet_l4(pkt) -
> > (char *) dp_packet_l3(pkt);
> > +
>
> > + /* Inner L2 length must account for the tunnel header length.
> > */
> > + l2 = dp_packet_l4(pkt);
>
> Code looks ok to me, but it's tricky and the L2 settings with inner
> requests are a bit unintuitive without a notepad and thinking from the
> driver perspective backwards. Not sure there is much can be done to
> mitigate that here, other than the comment you added.
Unfortunately, I don't have a better idea.
It was already unintuitive before this patch, but to make it worse,
the logic was split across lib/netdev-dpdk.c and
lib/netdev-native-tnl.c.
Like for example this comment in dp_packet_tnl_ol_process(), which is
DPDK specific.
/* Attention please, tunnel inner l2 len is consist of udp header
* len and tunnel header len and inner l2 len. */
>
> Did you manage to test to confirm they're working as expected ?
In general, I tested the series with CX6, E810 and ixgbe, with ipv4
traffic, ipv4 traffic tunneled in ipv4/ipv6 vxlan and ipv4 traffic
tunneled in ipv4/ipv6 geneve.
But I am not sure I covered every possible combinations.
Specifically for this case you point at (outer + inner offloads), I
tested CX6 with IPv4/IPv6 VxLAN and Geneve (for which I have traces in
my bash history).
With E810, I remember testing the same with the DPDK fixes, but I
don't have a trace of it.
I'll double check before sending a next revision.
>
> > + l3 = dp_packet_inner_l3(pkt);
> > + l4 = dp_packet_inner_l4(pkt);
>
> see below
>
> > } else {
> > /* If no outer offloading is requested, clear outer marks. */
> > mbuf->ol_flags &= ~all_outer_marks;
> > @@ -2642,8 +2645,9 @@ netdev_dpdk_prep_hwol_packet(struct netdev_dpdk *dev,
> > struct rte_mbuf *mbuf)
> > mbuf->outer_l3_len = 0;
> >
> > /* Skip outer headers. */
> > - mbuf->l2_len += (char *) dp_packet_l4(pkt) -
> > - (char *) dp_packet_eth(pkt);
> > + l2 = dp_packet_eth(pkt);
>
> > + l3 = dp_packet_inner_l3(pkt);
> > + l4 = dp_packet_inner_l4(pkt);
>
> You could move these outside the inner (pardon the pun) if else, but I
> could understand if you prefer to set l2/l3/l4 together for better
> readability ?
Well, as you noted, this code is not trivial.
I preferred to have all 3 pointers grouped, with a comment relating to
the group.
--
David Marchand
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
