On Mon, Jan 20, 2025 at 01:28:42PM +0530, Vipul Ashri via dev wrote:
> RCA: This issue was reported in multi-bridge OVS environment, having
> dynamic bridges. Each VM is tied with one bridge via vhu and a VM
> add/del causes bridge add/del. We found individual bridge deletion,
> while deleting VM was impacting overall OVS traffic, causing temp
> traffic outage even for other unrelated VMs. The packets are dropped
> with datapath_drop_lock_error coverage counter. We found that when
> deleting a bridge, we disable upcall for some time and flush entire
> dpctl flows. As we donot have per bridge dpctl flow, so any packets
> coming during this period of time will miss dpctl flow and go for
> upcall, which being disabled now will be dropped.
>
> Fix is to bypass "ofproto->ofproto_class->flush(ofproto)", once
> this callback holds the lock and dp flows flushed, leads to complete
> traffic outage. As per my analysis no need to do udpif_flush() to
> flush all datapath flows instead needed flows will be auto-deleted
> for required bridge with port delete or destroy APIs already called
> further in codeflow and bridge add/delete paths.
>
> Signed-off-by: Vipul Ashri <[email protected]>
> ---
> ofproto/ofproto.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 3df64efb9..0f0e20269 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -1839,7 +1839,12 @@ ofproto_destroy(struct ofproto *p, bool del)
> return;
> }
>
> - ofproto_flush__(p, del);
> + /* Changed arg from "del" -> "false" to fix traffic outage during
> + * individual bridge deletion.
Hi Vipul.
We don't need this log-type comment. Git will take care of showing what
was the previous value.
> + * DP flows cleanup shall be taken care automatically during individual
> + * port delete/destruct while processing evicting flows.
> + */
> + ofproto_flush__(p, false);
I understand your use-case and the inconvenience of bridge deletion
causing a full datapath flush. In this case, it does make sense to just
remove the ofproto part of the bridge and let the revalidators take care
of the datapath flows.
However, I think there is a bit more to it.
To start with, this change will result is "ofproto_flush__" to always be
called with "false", which makes me wonder if we'd need an argument at
all.
Also, I think we should look carefully at the other case: when we're
tearing down the entire datapath to see if we could create a race or
some strange behavior by leaving the datapath flows while we tear
_everything_ down. I have not done enough tests (nor have seen them done
by someone else) be confident here.
Third, this patch tries to skip calling:
ofproto->ofproto_class->flush(ofproto);
This function seems only called here and its commend seems to me rather
vague:
/* Every "struct rule" in 'ofproto' is about to be deleted, one by one.
* This function may prepare for that, for example by clearing state in
* advance. It should *not* actually delete any "struct rule"s from
* 'ofproto', only prepare for it.
*
* This function is optional; it's really just for optimization in case
* it's cheaper to delete all the flows from your hardware in a single pass
* than to do it one by one. */
The ideal scenario would be for this function to actually remove the
flows that belong to this ofproto but it might not be very clean to do
this and, unless we add support of bulk flushing in the datapath,
would be done "one by one".
'Every "struct rule" in 'ofproto' is about to be deleted, one by one.'
This happens in ofproto_class->destruct. Which means we should be able
to safely assume this is called inmediately before destruct() which ends
up calling "close_dpif_backer" who, smartly, does not whipe out the
entire datapath if there is some other ofproto (i.e: bridge) using it:
static void
close_dpif_backer(struct dpif_backer *backer, bool del)
{
struct simap_node *node;
ovs_assert(backer->refcount > 0);
if (--backer->refcount) {
return;
}
An idea would be to do the same, i.e: only flush if "backer->refcount > 1".
Final comment: this patch should have a test.
Thanks.
Adrián
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
