On Sun, Jan 10, 2021 at 12:25 PM Eli Britstein wrote:
>
>
> On 1/7/2021 9:10 PM, Sriharsha Basavapatna wrote:
> > Hi Eli,
> >
> > On Mon, Dec 28, 2020 at 11:43 PM Eli Britstein wrote:
> >> Netdev datapath offloads are done in a separate thread, using messaging
> >> between the threads. With port removal there is a race between the offload
> >> thread removing the offloaded rules and the actual port removal, so some
> >> rules will not be removed.
> > Can you provide details on this sequence, to get more clarity on how
> > some rules will not be removed ?
>
> A deletion sequence from dpif point of view starts with dpif_port_del.
> It calls dpif->dpif_class->port_del (implemented by
> dpif_netdev_port_del) and netdev_ports_remove.
>
> flow_mark_flush is called
> (dpif_netdev_port_del->do_del_port->reconfigure_datapath->reload_affected_pmds).
> This function only posts deletion requests to the queue
> (queue_netdev_flow_del), to be later handled by the offload thread.
>
> When the offload thread wakes up to handle the request
> (dp_netdev_flow_offload_del->mark_to_flow_disassociate) it looks for the
> netdev object by netdev_ports_get in port_to_netdev map, racing the
> execution of netdev_ports_remove that removes that mapping.
>
> If the mapping is removed before the handling, the removal of the HW
> rule won't take place, leaking it and the related allocated memory.
Thanks for the details; the above commit message could be reworded to
be more clear like this:
With port removal there is a race -->
"With port removal, there is a race due to which by the time the flow
delete request is processed by the offload thread, the corresponding
netdev would have already been removed (in dpif_port_del()). And the
offload thread fails to find the netdev in
mark_to_flow_disassociate(). Thus the flow is not deleted from HW."
> >> thread removing the offloaded rules and the actual port removal, so some
> >> rules will not be removed.
>
> >
> >> In OVS the offload objects are not freed (memory
> >> leak).
> > Since dp_netdev_free_flow_offload() frees the offload object, not sure
> > how the memory leak happens ?
> As eventually netdev_offload_dpdk_flow_del is not called, the objects in
> ufid_to_rte_flow that should have been freed by
> ufid_to_rte_flow_disassociate are leaked (as well as the rte_flows not
> destroyed).
Ok; the commit message could be more specific about the objects that
are leaked.
> >
> >> In HW the remining of the rules depend on PMD behavior.
> > A few related questions:
> >
> > 1. In Patch-1, netdev_flow_flush() is called from do_del_port().
> > Doesn't it violate the layering rules, since port deletion shouldn't
> > be directly involved with netdev/flow related operations (that would
> > otherwise be processed as a part of datapath reconfig) ?
>
> netdev/flow operations are called from dpif-netdev layer, that includes
> do_del_port. We can call the offload functions under dp->port_mutex
> which is locked in that function.
>
> Which layering rules are violated?
With this change, a port directly operates on the flows, as opposed to
letting the normal datapath routines delete the flow in the context of
a specific pmd thread.
Before this change, queue_netdev_flow_del() was the only entry point
for flow deletion and it was called by either flow_mark_flush() for a
given pmd thread or by dp_netdev_pmd_remove_flow() again for a given
pmd thread. But with the new change, the flow gets deleted without the
context of a pmd thread. Also, we are now entirely bypassing the
offload thread.
>
> >
> > 2. Since the rte_flow is deleted first, doesn't it leave the
> > dpif_netdev_flow in an inconsistent state, since the flow is still
> > active from the pmd thread's perspective (megaflow_to_mark and
> > mark_to_flow cmap entries are still valid, indicating that it is
> > offloaded).
>
> That flow is a sequence not related to this patch set, that occurs normally.
>
> Currently there are two types of offloads (both are ingress):
>
> - Partial. Packets are handled by SW with or without offload. Packets
> are fully processed correctly by the SW either if they don't have mark
> or have mark that is not found by mark_to_flow_find.
>
> - Full. Packets will be handled correctly by the SW as they arrive with
> no mark.
>
> The mark<->megaflow mapping disassociation is handled correctly even if
> the rte_flow is not removed.
>
> However, indeed, this is a good point to take into consideration when
> implementing egress offloading.
I understand how full and partial offloads work w.r.t mark. That was
not my point here. For an offloaded flow (partial or full), entries
are added to the "megaflow_to_mark" and "mark_to_flow" cmaps after the
flow is successfully offloaded by rte. Those entries get removed, when
the flow gets deleted, through dp_netdev_flow_offload_del() . The
cmap entries are removed at the same time the flow is deleted from
rte. But with this change, the deletion of a flow occurs in two
separate sequenc
