[ovs-dev] [PATCH V3 05/14] netdev-offload-dpdk: Implement HW miss packet recover for vport

[Eli Britstein]

On 3/15/2021 11:54 PM, Ivan Malov wrote:
> External email: Use caution opening links or attachments
>
>
> Hi Eli,
>
> On 02/03/2021 14:25, Eli Britstein wrote:
> > +        /* For tunnel recovery (RTE_FLOW_RESTORE_INFO_TUNNEL), it is
> possible
> > +         * to have the packet to still be encapsulated, or not
> > +         * (RTE_FLOW_RESTORE_INFO_ENCAPSULATED).
> > +         * In the case it is on, the packet is still encapsulated,
> and we do
> > +         * the pop in SW.
> > +         * In the case it is off, the packet is already decapsulated
> by HW, and
> > +         * the tunnel info is provided in the tunnel struct. For
> this case we
> > +         * take it to OVS metadata.
> > +         */
> > +        if (rte_restore_info.flags &
> RTE_FLOW_RESTORE_INFO_ENCAPSULATED) {
>
> The highlighted code is located *after*
> netdev_dpdk_rte_flow_get_restore_info() invocation. The comment says
> that the packet may *still* be encapsulated. Is the packet in fact
> allowed to have encapsulation header in it even *before*
> netdev_dpdk_rte_flow_get_restore_info()? I.e. right from the point where
> the packet is read from an ethdev's Rx queue. Is that legitimate?

The get_restore_info does not change the packet, but just provides the info.

Whether a packet with tunnel info (RTE_FLOW_RESTORE_INFO_TUNNEL) to 
still be encapsulated or not (RTE_FLOW_RESTORE_INFO_ENCAPSULATED) 
depends on PMD.

Thus, yes, it is legitimate.

>
> What concerns me is that in patch [06/14] TCP flags get parsed before
> the miss recovery:
> > +    /* Restore the packet if HW processing was terminated before
> completion. */
> > +    *tcp_flags = parse_tcp_flags(packet);
> > +    p = pmd_send_port_cache_lookup(pmd, port_no);
> > +    if (OVS_LIKELY(p)) {
> > +        int err = netdev_hw_miss_packet_recover(p->port->netdev,
> packet);
>
> So, I wonder, if the packet is indeed allowed to have encapsulation
> header at that point, can parse_tcp_flags() tolerate that? Will the
> flags lookup still be fine?
Thanks. I'll fix in v4.
>
> (I apologise for asking too many questions).
NP. Thanks for reviewing.
>
> -- 
> Ivan M