[ovs-dev] [PATCH] netdev-offload-tc: Flush rules on ingress block when init tc flow api
Dmytro Linkin
dmitrolin at mellanox.com
Tue Mar 3 15:29:37 UTC 2020
On Tue, Mar 03, 2020 at 02:15:21PM +0100, Ilya Maximets wrote:
> On 3/3/20 8:58 AM, Roi Dayan wrote:
> >
> >
> > On 2020-02-27 5:22 PM, Roi Dayan wrote:
> >> From: Dmytro Linkin <dmitrolin at mellanox.com>
> >>
> >> OVS can fail to attach ingress block on iface when init tc flow api,
> >> if block already exist with rules on it and is shared with other iface.
> >> Fix by flush all existing rules on the ingress block prior to deleting
> >> it.
> >>
> >> Fixes: 093c9458fb02 ("tc: allow offloading of block ids")
> >> Signed-off-by: Dmytro Linkin <dmitrolin at mellanox.com>
> >> Acked-by: Raed Salem <raeds at mellanox.com>
> >> Acked-by: Roi Dayan <roid at mellanox.com>
> >> ---
> >> lib/netdev-offload-tc.c | 10 +++++++++-
> >> 1 file changed, 9 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> >> index 550e440b3a45..b5ff6ccca55e 100644
> >> --- a/lib/netdev-offload-tc.c
> >> +++ b/lib/netdev-offload-tc.c
> >> @@ -1907,6 +1907,7 @@ netdev_tc_init_flow_api(struct netdev *netdev)
> >> static struct ovsthread_once block_once = OVSTHREAD_ONCE_INITIALIZER;
> >> enum tc_qdisc_hook hook = get_tc_qdisc_hook(netdev);
> >> uint32_t block_id = 0;
> >> + struct tcf_id id;
> >> int ifindex;
> >> int error;
> >>
> >> @@ -1917,6 +1918,14 @@ netdev_tc_init_flow_api(struct netdev *netdev)
> >> return -ifindex;
> >> }
> >>
> >> + block_id = get_block_id_from_netdev(netdev);
> >> +
> >> + /* Flush rules explicitly needed when we work with ingress_block,
> >> + * so we will not fail with reattaching block to bond iface, for ex.
> >> + */
> >> + id = tc_make_tcf_id(ifindex, block_id, 0, hook);
> >> + tc_del_filter(&id);
> >> +
> >> /* make sure there is no ingress/egress qdisc */
> >> tc_add_del_qdisc(ifindex, false, 0, hook);
> >>
> >> @@ -1930,7 +1939,6 @@ netdev_tc_init_flow_api(struct netdev *netdev)
> >> ovsthread_once_done(&multi_mask_once);
> >> }
> >>
> >> - block_id = get_block_id_from_netdev(netdev);
> >> error = tc_add_del_qdisc(ifindex, true, block_id, hook);
> >>
> >> if (error && error != EEXIST) {
> >>
> >
> > +ilya
> >
> > Hi Ilya,
> >
> > can you help review/ack this?
>
> Hi. I'm not an expert in linux tc, but since you're asking...
>
> IIUC, the issue is that tc_add_del_qdisc(ifindex, true, block_id, hook)
> fails on bond iface. Is it correct?
At first tc_add_del_qdisc() fails on deletion, because qdisc, which is
added to block, is in use (rules are exist). We just don't care about
any error returned. Then tc_add_del_qdisc() fail to add it, because it
wasn't deleted and in use.
> In this case I see one strange thing. We're clearing ingress qdisk
> for block_id == 0, but after that trying to create new one with
> block_id == ifindex (for LAG interface). Will it help if we delete
> ingress qdisk providing correct block_id? This sounds like something
> sane to do.
Deleting block_id != 0 will fail, due to existing rules. But actually,
deleting qdisc with block_id == 0 still delete correct block.
Anyway, the point here is to flush rules on specified ingress block.
Regards, Dmytro.
More information about the dev
mailing list