Re: [ovs-dev] [PATCH] netdev-offload-tc: Flush rules on ingress block when init tc flow api
On Tue, Mar 03, 2020 at 07:37:45PM +0200, Dmytro Linkin wrote:
> On Tue, Mar 03, 2020 at 04:42:12PM +0100, Ilya Maximets wrote:
> > On 3/3/20 4:29 PM, Dmytro Linkin wrote:
> > > 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
> > >>>>
> > >>>> 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
> > >>>> Acked-by: Raed Salem
> > >>>> Acked-by: Roi Dayan
> > >>>> ---
> > >>>> 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();
> > >>>> +
> > >>>> /* 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(_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.
> >
> > Hmm. OK.
> >
> > Shouldn't we unconditionally flush rules from qdisk inside the
> > tc_add_del_qdisc() if deletion is requested? From your explanation
> > it seems like a prerequisite for that and qdisk will not be deleted
> > if we will not flush rules.
>
> Flushing rules needed only for shared block, 'cause like in our case,
> there are ifaces not attached to the bridge, so OVS don't ask kernel to
> delete qdisc. In other known cases kernel flush rules by itself on qdisc
> deletion, so flush inside tc_add_del_qdisc() mostly not needed.
> Mb, better do it for shared blocks only.
>
> > BTW, one thing in this patch that doesn't look good is that you're
> > using block_id before probing the support.
>
> Probably yes. What You suggest here? By case, it work. Probing triggers
> on internal OVS ifaces at first, so later calls to get block_id return
> valid id.
>
> Regards, Dmytro
Ping
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] netdev-offload-tc: Flush rules on ingress block when init tc flow api
On Tue, Mar 03, 2020 at 04:42:12PM +0100, Ilya Maximets wrote:
> On 3/3/20 4:29 PM, Dmytro Linkin wrote:
> > 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
> >>>>
> >>>> 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
> >>>> Acked-by: Raed Salem
> >>>> Acked-by: Roi Dayan
> >>>> ---
> >>>> 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();
> >>>> +
> >>>> /* 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(_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.
>
> Hmm. OK.
>
> Shouldn't we unconditionally flush rules from qdisk inside the
> tc_add_del_qdisc() if deletion is requested? From your explanation
> it seems like a prerequisite for that and qdisk will not be deleted
> if we will not flush rules.
Flushing rules needed only for shared block, 'cause like in our case,
there are ifaces not attached to the bridge, so OVS don't ask kernel to
delete qdisc. In other known cases kernel flush rules by itself on qdisc
deletion, so flush inside tc_add_del_qdisc() mostly not needed.
Mb, better do it for shared blocks only.
> BTW, one thing in this patch that doesn't look good is that you're
> using block_id before probing the support.
Probably yes. What You suggest here? By case, it work. Probing triggers
on internal OVS ifaces at first, so later calls to get block_id return
valid id.
Regards, Dmytro
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] netdev-offload-tc: Flush rules on ingress block when init tc flow api
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
> >>
> >> 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
> >> Acked-by: Raed Salem
> >> Acked-by: Roi Dayan
> >> ---
> >> 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();
> >> +
> >> /* 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(_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.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
