Hello Kevin,
Thanks for reviewing.
On Fri, May 10, 2024 at 11:50 PM Kevin Traynor <ktray...@redhat.com> wrote:
>
> On 19/04/2024 15:06, David Marchand wrote:
> > Replace check on th == NULL with an assert() because dp_packet_l4(pkt)
> > is priorly used to compute (outer) L3 length.
> >
> > Besides, filling l4_len and tso_segsz only matters to TSO, so there is
> > no need to check for other L4 checksum offloading requests.
> >
> > Signed-off-by: David Marchand <david.march...@redhat.com>
> > ---
> > lib/netdev-dpdk.c | 36 +++++++++++-------------------------
> > 1 file changed, 11 insertions(+), 25 deletions(-)
> >
> > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> > index 8b6a3ed189..661269e4b6 100644
> > --- a/lib/netdev-dpdk.c
> > +++ b/lib/netdev-dpdk.c
> > @@ -2584,7 +2584,6 @@ 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);
> > - struct tcp_header *th;
> >
> > const uint64_t all_inner_requests = (RTE_MBUF_F_TX_IP_CKSUM |
> > RTE_MBUF_F_TX_L4_MASK |
> > @@ -2614,6 +2613,8 @@ netdev_dpdk_prep_hwol_packet(struct netdev_dpdk *dev,
> > struct rte_mbuf *mbuf)
> > return true;
> > }
> >
> > + ovs_assert(dp_packet_l4(pkt));
>
> I'm not clear why you want to change this from a warning/return
> fail/drop to an assert ?
From this point in the function, there is at least one request for
checksum offloading pending.
Any L3 (or higher) checksum requested by OVS means that the packet has
been parsed/composed as either IP or IPv6 and packet->l4_ofs was set
to point after the l3 header (with miniflow_extract / *_compose()
helpers).
So getting a NULL pointer for l4 here indicates a bug in OVS.
An assert seems better than a warn/return that probably nobody notice(d).
Did I miss a case where l4_ofs can be unset?
>
> Nit: should this be in the previous patch instead ? and I see it is
> removed in a later patch.
It is not supposed to be removed in the series.
The last patch moves it later in the function.
--
David Marchand
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
