* [PATCH net v3] net/sched: cls_api: Fix nooffloaddevcnt counter when indr block call success
@ 2019-09-19 8:37 wenxu
2019-09-19 8:40 ` wenxu
2019-09-19 12:50 ` Or Gerlitz
0 siblings, 2 replies; 7+ messages in thread
From: wenxu @ 2019-09-19 8:37 UTC (permalink / raw)
To: davem; +Cc: netdev
From: wenxu <wenxu@ucloud.cn>
A vxlan or gretap device offload through indr block methord. If the device
successfully bind with a real hw through indr block call, It also add
nooffloadcnt counter. This counter will lead the rule add failed in
fl_hw_replace_filter-->tc_setup_cb_call with skip_sw flags.
In the tc_setup_cb_call will check the nooffloaddevcnt and skip_sw flags
as following:
if (block->nooffloaddevcnt && err_stop)
return -EOPNOTSUPP;
So with this patch, if the indr block call success, it will not modify
the nooffloaddevcnt counter.
Fixes: 7f76fa36754b ("net: sched: register callbacks for indirect tc block binds")
Signed-off-by: wenxu <wenxu@ucloud.cn>
---
v3: rebase to the net
net/sched/cls_api.c | 30 +++++++++++++++++-------------
1 file changed, 17 insertions(+), 13 deletions(-)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 32577c2..c980127 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -607,11 +607,11 @@ static void tc_indr_block_get_and_ing_cmd(struct net_device *dev,
tc_indr_block_ing_cmd(dev, block, cb, cb_priv, command);
}
-static void tc_indr_block_call(struct tcf_block *block,
- struct net_device *dev,
- struct tcf_block_ext_info *ei,
- enum flow_block_command command,
- struct netlink_ext_ack *extack)
+static int tc_indr_block_call(struct tcf_block *block,
+ struct net_device *dev,
+ struct tcf_block_ext_info *ei,
+ enum flow_block_command command,
+ struct netlink_ext_ack *extack)
{
struct flow_block_offload bo = {
.command = command,
@@ -621,10 +621,15 @@ static void tc_indr_block_call(struct tcf_block *block,
.block_shared = tcf_block_shared(block),
.extack = extack,
};
+
INIT_LIST_HEAD(&bo.cb_list);
flow_indr_block_call(dev, &bo, command);
- tcf_block_setup(block, &bo);
+
+ if (list_empty(&bo.cb_list))
+ return -EOPNOTSUPP;
+
+ return tcf_block_setup(block, &bo);
}
static bool tcf_block_offload_in_use(struct tcf_block *block)
@@ -681,8 +686,6 @@ static int tcf_block_offload_bind(struct tcf_block *block, struct Qdisc *q,
goto no_offload_dev_inc;
if (err)
goto err_unlock;
-
- tc_indr_block_call(block, dev, ei, FLOW_BLOCK_BIND, extack);
up_write(&block->cb_lock);
return 0;
@@ -691,9 +694,10 @@ static int tcf_block_offload_bind(struct tcf_block *block, struct Qdisc *q,
err = -EOPNOTSUPP;
goto err_unlock;
}
+ err = tc_indr_block_call(block, dev, ei, FLOW_BLOCK_BIND, extack);
+ if (err)
+ block->nooffloaddevcnt++;
err = 0;
- block->nooffloaddevcnt++;
- tc_indr_block_call(block, dev, ei, FLOW_BLOCK_BIND, extack);
err_unlock:
up_write(&block->cb_lock);
return err;
@@ -706,8 +710,6 @@ static void tcf_block_offload_unbind(struct tcf_block *block, struct Qdisc *q,
int err;
down_write(&block->cb_lock);
- tc_indr_block_call(block, dev, ei, FLOW_BLOCK_UNBIND, NULL);
-
if (!dev->netdev_ops->ndo_setup_tc)
goto no_offload_dev_dec;
err = tcf_block_offload_cmd(block, dev, ei, FLOW_BLOCK_UNBIND, NULL);
@@ -717,7 +719,9 @@ static void tcf_block_offload_unbind(struct tcf_block *block, struct Qdisc *q,
return;
no_offload_dev_dec:
- WARN_ON(block->nooffloaddevcnt-- == 0);
+ err = tc_indr_block_call(block, dev, ei, FLOW_BLOCK_UNBIND, NULL);
+ if (err)
+ WARN_ON(block->nooffloaddevcnt-- == 0);
up_write(&block->cb_lock);
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH net v3] net/sched: cls_api: Fix nooffloaddevcnt counter when indr block call success 2019-09-19 8:37 [PATCH net v3] net/sched: cls_api: Fix nooffloaddevcnt counter when indr block call success wenxu @ 2019-09-19 8:40 ` wenxu 2019-09-19 12:50 ` Or Gerlitz 1 sibling, 0 replies; 7+ messages in thread From: wenxu @ 2019-09-19 8:40 UTC (permalink / raw) To: jiri; +Cc: netdev sorry forget cc to jiri. On 9/19/2019 4:37 PM, wenxu@ucloud.cn wrote: > From: wenxu <wenxu@ucloud.cn> > > A vxlan or gretap device offload through indr block methord. If the device > successfully bind with a real hw through indr block call, It also add > nooffloadcnt counter. This counter will lead the rule add failed in > fl_hw_replace_filter-->tc_setup_cb_call with skip_sw flags. > > In the tc_setup_cb_call will check the nooffloaddevcnt and skip_sw flags > as following: > if (block->nooffloaddevcnt && err_stop) > return -EOPNOTSUPP; > > So with this patch, if the indr block call success, it will not modify > the nooffloaddevcnt counter. > > Fixes: 7f76fa36754b ("net: sched: register callbacks for indirect tc block binds") > Signed-off-by: wenxu <wenxu@ucloud.cn> > --- > v3: rebase to the net > > net/sched/cls_api.c | 30 +++++++++++++++++------------- > 1 file changed, 17 insertions(+), 13 deletions(-) > > diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c > index 32577c2..c980127 100644 > --- a/net/sched/cls_api.c > +++ b/net/sched/cls_api.c > @@ -607,11 +607,11 @@ static void tc_indr_block_get_and_ing_cmd(struct net_device *dev, > tc_indr_block_ing_cmd(dev, block, cb, cb_priv, command); > } > > -static void tc_indr_block_call(struct tcf_block *block, > - struct net_device *dev, > - struct tcf_block_ext_info *ei, > - enum flow_block_command command, > - struct netlink_ext_ack *extack) > +static int tc_indr_block_call(struct tcf_block *block, > + struct net_device *dev, > + struct tcf_block_ext_info *ei, > + enum flow_block_command command, > + struct netlink_ext_ack *extack) > { > struct flow_block_offload bo = { > .command = command, > @@ -621,10 +621,15 @@ static void tc_indr_block_call(struct tcf_block *block, > .block_shared = tcf_block_shared(block), > .extack = extack, > }; > + > INIT_LIST_HEAD(&bo.cb_list); > > flow_indr_block_call(dev, &bo, command); > - tcf_block_setup(block, &bo); > + > + if (list_empty(&bo.cb_list)) > + return -EOPNOTSUPP; > + > + return tcf_block_setup(block, &bo); > } > > static bool tcf_block_offload_in_use(struct tcf_block *block) > @@ -681,8 +686,6 @@ static int tcf_block_offload_bind(struct tcf_block *block, struct Qdisc *q, > goto no_offload_dev_inc; > if (err) > goto err_unlock; > - > - tc_indr_block_call(block, dev, ei, FLOW_BLOCK_BIND, extack); > up_write(&block->cb_lock); > return 0; > > @@ -691,9 +694,10 @@ static int tcf_block_offload_bind(struct tcf_block *block, struct Qdisc *q, > err = -EOPNOTSUPP; > goto err_unlock; > } > + err = tc_indr_block_call(block, dev, ei, FLOW_BLOCK_BIND, extack); > + if (err) > + block->nooffloaddevcnt++; > err = 0; > - block->nooffloaddevcnt++; > - tc_indr_block_call(block, dev, ei, FLOW_BLOCK_BIND, extack); > err_unlock: > up_write(&block->cb_lock); > return err; > @@ -706,8 +710,6 @@ static void tcf_block_offload_unbind(struct tcf_block *block, struct Qdisc *q, > int err; > > down_write(&block->cb_lock); > - tc_indr_block_call(block, dev, ei, FLOW_BLOCK_UNBIND, NULL); > - > if (!dev->netdev_ops->ndo_setup_tc) > goto no_offload_dev_dec; > err = tcf_block_offload_cmd(block, dev, ei, FLOW_BLOCK_UNBIND, NULL); > @@ -717,7 +719,9 @@ static void tcf_block_offload_unbind(struct tcf_block *block, struct Qdisc *q, > return; > > no_offload_dev_dec: > - WARN_ON(block->nooffloaddevcnt-- == 0); > + err = tc_indr_block_call(block, dev, ei, FLOW_BLOCK_UNBIND, NULL); > + if (err) > + WARN_ON(block->nooffloaddevcnt-- == 0); > up_write(&block->cb_lock); > } > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net v3] net/sched: cls_api: Fix nooffloaddevcnt counter when indr block call success 2019-09-19 8:37 [PATCH net v3] net/sched: cls_api: Fix nooffloaddevcnt counter when indr block call success wenxu 2019-09-19 8:40 ` wenxu @ 2019-09-19 12:50 ` Or Gerlitz 2019-09-23 4:19 ` wenxu 1 sibling, 1 reply; 7+ messages in thread From: Or Gerlitz @ 2019-09-19 12:50 UTC (permalink / raw) To: wenxu, Pieter Jansen van Vuuren, Oz Shlomo Cc: David Miller, Linux Netdev List, Roi Dayan, Paul Blakey, Jiri Pirko On Thu, Sep 19, 2019 at 11:39 AM <wenxu@ucloud.cn> wrote: > > From: wenxu <wenxu@ucloud.cn> > > A vxlan or gretap device offload through indr block methord. If the device nit: method --> method > successfully bind with a real hw through indr block call, It also add > nooffloadcnt counter. This counter will lead the rule add failed in > fl_hw_replace_filter-->tc_setup_cb_call with skip_sw flags. wait.. indirect tc callbacks are typically used to do hw offloading for decap rules (tunnel key unset action) set on SW devices (gretap, vxlan). However, AFAIK, it's been couple of years since the kernel doesn't support skip_sw for such rules. Did we enable it again? when? I am somehow far from the details, so copied some folks.. Or. > > In the tc_setup_cb_call will check the nooffloaddevcnt and skip_sw flags > as following: > if (block->nooffloaddevcnt && err_stop) > return -EOPNOTSUPP; > > So with this patch, if the indr block call success, it will not modify > the nooffloaddevcnt counter. > > Fixes: 7f76fa36754b ("net: sched: register callbacks for indirect tc block binds") > Signed-off-by: wenxu <wenxu@ucloud.cn> > --- > v3: rebase to the net > > net/sched/cls_api.c | 30 +++++++++++++++++------------- > 1 file changed, 17 insertions(+), 13 deletions(-) > > diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c > index 32577c2..c980127 100644 > --- a/net/sched/cls_api.c > +++ b/net/sched/cls_api.c > @@ -607,11 +607,11 @@ static void tc_indr_block_get_and_ing_cmd(struct net_device *dev, > tc_indr_block_ing_cmd(dev, block, cb, cb_priv, command); > } > > -static void tc_indr_block_call(struct tcf_block *block, > - struct net_device *dev, > - struct tcf_block_ext_info *ei, > - enum flow_block_command command, > - struct netlink_ext_ack *extack) > +static int tc_indr_block_call(struct tcf_block *block, > + struct net_device *dev, > + struct tcf_block_ext_info *ei, > + enum flow_block_command command, > + struct netlink_ext_ack *extack) > { > struct flow_block_offload bo = { > .command = command, > @@ -621,10 +621,15 @@ static void tc_indr_block_call(struct tcf_block *block, > .block_shared = tcf_block_shared(block), > .extack = extack, > }; > + > INIT_LIST_HEAD(&bo.cb_list); > > flow_indr_block_call(dev, &bo, command); > - tcf_block_setup(block, &bo); > + > + if (list_empty(&bo.cb_list)) > + return -EOPNOTSUPP; > + > + return tcf_block_setup(block, &bo); > } > > static bool tcf_block_offload_in_use(struct tcf_block *block) > @@ -681,8 +686,6 @@ static int tcf_block_offload_bind(struct tcf_block *block, struct Qdisc *q, > goto no_offload_dev_inc; > if (err) > goto err_unlock; > - > - tc_indr_block_call(block, dev, ei, FLOW_BLOCK_BIND, extack); > up_write(&block->cb_lock); > return 0; > > @@ -691,9 +694,10 @@ static int tcf_block_offload_bind(struct tcf_block *block, struct Qdisc *q, > err = -EOPNOTSUPP; > goto err_unlock; > } > + err = tc_indr_block_call(block, dev, ei, FLOW_BLOCK_BIND, extack); > + if (err) > + block->nooffloaddevcnt++; > err = 0; > - block->nooffloaddevcnt++; > - tc_indr_block_call(block, dev, ei, FLOW_BLOCK_BIND, extack); > err_unlock: > up_write(&block->cb_lock); > return err; > @@ -706,8 +710,6 @@ static void tcf_block_offload_unbind(struct tcf_block *block, struct Qdisc *q, > int err; > > down_write(&block->cb_lock); > - tc_indr_block_call(block, dev, ei, FLOW_BLOCK_UNBIND, NULL); > - > if (!dev->netdev_ops->ndo_setup_tc) > goto no_offload_dev_dec; > err = tcf_block_offload_cmd(block, dev, ei, FLOW_BLOCK_UNBIND, NULL); > @@ -717,7 +719,9 @@ static void tcf_block_offload_unbind(struct tcf_block *block, struct Qdisc *q, > return; > > no_offload_dev_dec: > - WARN_ON(block->nooffloaddevcnt-- == 0); > + err = tc_indr_block_call(block, dev, ei, FLOW_BLOCK_UNBIND, NULL); > + if (err) > + WARN_ON(block->nooffloaddevcnt-- == 0); > up_write(&block->cb_lock); > } > > -- > 1.8.3.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net v3] net/sched: cls_api: Fix nooffloaddevcnt counter when indr block call success 2019-09-19 12:50 ` Or Gerlitz @ 2019-09-23 4:19 ` wenxu 2019-09-23 9:42 ` John Hurley 0 siblings, 1 reply; 7+ messages in thread From: wenxu @ 2019-09-23 4:19 UTC (permalink / raw) To: Or Gerlitz, Pieter Jansen van Vuuren, Oz Shlomo, John Hurley, Jakub Kicinski Cc: David Miller, Linux Netdev List, Roi Dayan, Paul Blakey, Jiri Pirko Hi John & Jakub There are some limitations for indirect tc callback work with skip_sw ? BR wenxu On 9/19/2019 8:50 PM, Or Gerlitz wrote: > >> successfully bind with a real hw through indr block call, It also add >> nooffloadcnt counter. This counter will lead the rule add failed in >> fl_hw_replace_filter-->tc_setup_cb_call with skip_sw flags. > wait.. indirect tc callbacks are typically used to do hw offloading > for decap rules (tunnel key unset action) set on SW devices (gretap, vxlan). > > However, AFAIK, it's been couple of years since the kernel doesn't support > skip_sw for such rules. Did we enable it again? when? I am somehow > far from the details, so copied some folks.. > > Or. > > >> In the tc_setup_cb_call will check the nooffloaddevcnt and skip_sw flags >> as following: >> if (block->nooffloaddevcnt && err_stop) >> return -EOPNOTSUPP; >> >> So with this patch, if the indr block call success, it will not modify >> the nooffloaddevcnt counter. >> >> Fixes: 7f76fa36754b ("net: sched: register callbacks for indirect tc block binds") >> Signed-off-by: wenxu <wenxu@ucloud.cn> >> --- >> v3: rebase to the net >> >> net/sched/cls_api.c | 30 +++++++++++++++++------------- >> 1 file changed, 17 insertions(+), 13 deletions(-) >> >> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c >> index 32577c2..c980127 100644 >> --- a/net/sched/cls_api.c >> +++ b/net/sched/cls_api.c >> @@ -607,11 +607,11 @@ static void tc_indr_block_get_and_ing_cmd(struct net_device *dev, >> tc_indr_block_ing_cmd(dev, block, cb, cb_priv, command); >> } >> >> -static void tc_indr_block_call(struct tcf_block *block, >> - struct net_device *dev, >> - struct tcf_block_ext_info *ei, >> - enum flow_block_command command, >> - struct netlink_ext_ack *extack) >> +static int tc_indr_block_call(struct tcf_block *block, >> + struct net_device *dev, >> + struct tcf_block_ext_info *ei, >> + enum flow_block_command command, >> + struct netlink_ext_ack *extack) >> { >> struct flow_block_offload bo = { >> .command = command, >> @@ -621,10 +621,15 @@ static void tc_indr_block_call(struct tcf_block *block, >> .block_shared = tcf_block_shared(block), >> .extack = extack, >> }; >> + >> INIT_LIST_HEAD(&bo.cb_list); >> >> flow_indr_block_call(dev, &bo, command); >> - tcf_block_setup(block, &bo); >> + >> + if (list_empty(&bo.cb_list)) >> + return -EOPNOTSUPP; >> + >> + return tcf_block_setup(block, &bo); >> } >> >> static bool tcf_block_offload_in_use(struct tcf_block *block) >> @@ -681,8 +686,6 @@ static int tcf_block_offload_bind(struct tcf_block *block, struct Qdisc *q, >> goto no_offload_dev_inc; >> if (err) >> goto err_unlock; >> - >> - tc_indr_block_call(block, dev, ei, FLOW_BLOCK_BIND, extack); >> up_write(&block->cb_lock); >> return 0; >> >> @@ -691,9 +694,10 @@ static int tcf_block_offload_bind(struct tcf_block *block, struct Qdisc *q, >> err = -EOPNOTSUPP; >> goto err_unlock; >> } >> + err = tc_indr_block_call(block, dev, ei, FLOW_BLOCK_BIND, extack); >> + if (err) >> + block->nooffloaddevcnt++; >> err = 0; >> - block->nooffloaddevcnt++; >> - tc_indr_block_call(block, dev, ei, FLOW_BLOCK_BIND, extack); >> err_unlock: >> up_write(&block->cb_lock); >> return err; >> @@ -706,8 +710,6 @@ static void tcf_block_offload_unbind(struct tcf_block *block, struct Qdisc *q, >> int err; >> >> down_write(&block->cb_lock); >> - tc_indr_block_call(block, dev, ei, FLOW_BLOCK_UNBIND, NULL); >> - >> if (!dev->netdev_ops->ndo_setup_tc) >> goto no_offload_dev_dec; >> err = tcf_block_offload_cmd(block, dev, ei, FLOW_BLOCK_UNBIND, NULL); >> @@ -717,7 +719,9 @@ static void tcf_block_offload_unbind(struct tcf_block *block, struct Qdisc *q, >> return; >> >> no_offload_dev_dec: >> - WARN_ON(block->nooffloaddevcnt-- == 0); >> + err = tc_indr_block_call(block, dev, ei, FLOW_BLOCK_UNBIND, NULL); >> + if (err) >> + WARN_ON(block->nooffloaddevcnt-- == 0); >> up_write(&block->cb_lock); >> } >> >> -- >> 1.8.3.1 >> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net v3] net/sched: cls_api: Fix nooffloaddevcnt counter when indr block call success 2019-09-23 4:19 ` wenxu @ 2019-09-23 9:42 ` John Hurley 2019-09-23 14:18 ` wenxu 0 siblings, 1 reply; 7+ messages in thread From: John Hurley @ 2019-09-23 9:42 UTC (permalink / raw) To: wenxu Cc: Or Gerlitz, Pieter Jansen van Vuuren, Oz Shlomo, Jakub Kicinski, David Miller, Linux Netdev List, Roi Dayan, Paul Blakey, Jiri Pirko On Mon, Sep 23, 2019 at 5:20 AM wenxu <wenxu@ucloud.cn> wrote: > > Hi John & Jakub > > There are some limitations for indirect tc callback work with skip_sw ? > Hi Wenxu, This is not really a limitation. As Or points out, indirect block offload is not supposed to work with skip_sw. Indirect offload allows us to hook onto existing kernel devices (for TC events we may which to offload) that are out of the control of the offload driver and, therefore, should always accept software path rules. For example, the vxlan driver does not implement a setup_tc ndo so it does not expect to run rules in hw - it should always handle associated rules in the software datapath as a minimum. I think accepting skip_sw rules for devices with no in-built concept of hardware offload would be wrong. Do you have a use case that requires skip_sw rules for such devices? John > > BR > > wenxu > > On 9/19/2019 8:50 PM, Or Gerlitz wrote: > > > >> successfully bind with a real hw through indr block call, It also add > >> nooffloadcnt counter. This counter will lead the rule add failed in > >> fl_hw_replace_filter-->tc_setup_cb_call with skip_sw flags. > > wait.. indirect tc callbacks are typically used to do hw offloading > > for decap rules (tunnel key unset action) set on SW devices (gretap, vxlan). > > > > However, AFAIK, it's been couple of years since the kernel doesn't support > > skip_sw for such rules. Did we enable it again? when? I am somehow > > far from the details, so copied some folks.. > > > > Or. > > > > > >> In the tc_setup_cb_call will check the nooffloaddevcnt and skip_sw flags > >> as following: > >> if (block->nooffloaddevcnt && err_stop) > >> return -EOPNOTSUPP; > >> > >> So with this patch, if the indr block call success, it will not modify > >> the nooffloaddevcnt counter. > >> > >> Fixes: 7f76fa36754b ("net: sched: register callbacks for indirect tc block binds") > >> Signed-off-by: wenxu <wenxu@ucloud.cn> > >> --- > >> v3: rebase to the net > >> > >> net/sched/cls_api.c | 30 +++++++++++++++++------------- > >> 1 file changed, 17 insertions(+), 13 deletions(-) > >> > >> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c > >> index 32577c2..c980127 100644 > >> --- a/net/sched/cls_api.c > >> +++ b/net/sched/cls_api.c > >> @@ -607,11 +607,11 @@ static void tc_indr_block_get_and_ing_cmd(struct net_device *dev, > >> tc_indr_block_ing_cmd(dev, block, cb, cb_priv, command); > >> } > >> > >> -static void tc_indr_block_call(struct tcf_block *block, > >> - struct net_device *dev, > >> - struct tcf_block_ext_info *ei, > >> - enum flow_block_command command, > >> - struct netlink_ext_ack *extack) > >> +static int tc_indr_block_call(struct tcf_block *block, > >> + struct net_device *dev, > >> + struct tcf_block_ext_info *ei, > >> + enum flow_block_command command, > >> + struct netlink_ext_ack *extack) > >> { > >> struct flow_block_offload bo = { > >> .command = command, > >> @@ -621,10 +621,15 @@ static void tc_indr_block_call(struct tcf_block *block, > >> .block_shared = tcf_block_shared(block), > >> .extack = extack, > >> }; > >> + > >> INIT_LIST_HEAD(&bo.cb_list); > >> > >> flow_indr_block_call(dev, &bo, command); > >> - tcf_block_setup(block, &bo); > >> + > >> + if (list_empty(&bo.cb_list)) > >> + return -EOPNOTSUPP; > >> + > >> + return tcf_block_setup(block, &bo); > >> } > >> > >> static bool tcf_block_offload_in_use(struct tcf_block *block) > >> @@ -681,8 +686,6 @@ static int tcf_block_offload_bind(struct tcf_block *block, struct Qdisc *q, > >> goto no_offload_dev_inc; > >> if (err) > >> goto err_unlock; > >> - > >> - tc_indr_block_call(block, dev, ei, FLOW_BLOCK_BIND, extack); > >> up_write(&block->cb_lock); > >> return 0; > >> > >> @@ -691,9 +694,10 @@ static int tcf_block_offload_bind(struct tcf_block *block, struct Qdisc *q, > >> err = -EOPNOTSUPP; > >> goto err_unlock; > >> } > >> + err = tc_indr_block_call(block, dev, ei, FLOW_BLOCK_BIND, extack); > >> + if (err) > >> + block->nooffloaddevcnt++; > >> err = 0; > >> - block->nooffloaddevcnt++; > >> - tc_indr_block_call(block, dev, ei, FLOW_BLOCK_BIND, extack); > >> err_unlock: > >> up_write(&block->cb_lock); > >> return err; > >> @@ -706,8 +710,6 @@ static void tcf_block_offload_unbind(struct tcf_block *block, struct Qdisc *q, > >> int err; > >> > >> down_write(&block->cb_lock); > >> - tc_indr_block_call(block, dev, ei, FLOW_BLOCK_UNBIND, NULL); > >> - > >> if (!dev->netdev_ops->ndo_setup_tc) > >> goto no_offload_dev_dec; > >> err = tcf_block_offload_cmd(block, dev, ei, FLOW_BLOCK_UNBIND, NULL); > >> @@ -717,7 +719,9 @@ static void tcf_block_offload_unbind(struct tcf_block *block, struct Qdisc *q, > >> return; > >> > >> no_offload_dev_dec: > >> - WARN_ON(block->nooffloaddevcnt-- == 0); > >> + err = tc_indr_block_call(block, dev, ei, FLOW_BLOCK_UNBIND, NULL); > >> + if (err) > >> + WARN_ON(block->nooffloaddevcnt-- == 0); > >> up_write(&block->cb_lock); > >> } > >> > >> -- > >> 1.8.3.1 > >> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net v3] net/sched: cls_api: Fix nooffloaddevcnt counter when indr block call success 2019-09-23 9:42 ` John Hurley @ 2019-09-23 14:18 ` wenxu 2019-09-24 10:33 ` Or Gerlitz 0 siblings, 1 reply; 7+ messages in thread From: wenxu @ 2019-09-23 14:18 UTC (permalink / raw) To: John Hurley Cc: Or Gerlitz, Pieter Jansen van Vuuren, Oz Shlomo, Jakub Kicinski, David Miller, Linux Netdev List, Roi Dayan, Paul Blakey, Jiri Pirko 在 2019/9/23 17:42, John Hurley 写道: > On Mon, Sep 23, 2019 at 5:20 AM wenxu <wenxu@ucloud.cn> wrote: >> Hi John & Jakub >> >> There are some limitations for indirect tc callback work with skip_sw ? >> > Hi Wenxu, > This is not really a limitation. > As Or points out, indirect block offload is not supposed to work with skip_sw. > Indirect offload allows us to hook onto existing kernel devices (for > TC events we may which to offload) that are out of the control of the > offload driver and, therefore, should always accept software path > rules. > For example, the vxlan driver does not implement a setup_tc ndo so it > does not expect to run rules in hw - it should always handle > associated rules in the software datapath as a minimum. > I think accepting skip_sw rules for devices with no in-built concept > of hardware offload would be wrong. > Do you have a use case that requires skip_sw rules for such devices? > > John When we use ovs to control the tc offload. The ovs kernel already provide the software path rules so maybe user don't want other soft path. And with skip_sw it can be easily distinguish offloaded and non-offloaded rules. > > >> BR >> >> wenxu >> >> On 9/19/2019 8:50 PM, Or Gerlitz wrote: >>>> successfully bind with a real hw through indr block call, It also add >>>> nooffloadcnt counter. This counter will lead the rule add failed in >>>> fl_hw_replace_filter-->tc_setup_cb_call with skip_sw flags. >>> wait.. indirect tc callbacks are typically used to do hw offloading >>> for decap rules (tunnel key unset action) set on SW devices (gretap, vxlan). >>> >>> However, AFAIK, it's been couple of years since the kernel doesn't support >>> skip_sw for such rules. Did we enable it again? when? I am somehow >>> far from the details, so copied some folks.. >>> >>> Or. >>> >>> >>>> In the tc_setup_cb_call will check the nooffloaddevcnt and skip_sw flags >>>> as following: >>>> if (block->nooffloaddevcnt && err_stop) >>>> return -EOPNOTSUPP; >>>> >>>> So with this patch, if the indr block call success, it will not modify >>>> the nooffloaddevcnt counter. >>>> >>>> Fixes: 7f76fa36754b ("net: sched: register callbacks for indirect tc block binds") >>>> Signed-off-by: wenxu <wenxu@ucloud.cn> >>>> --- >>>> v3: rebase to the net >>>> >>>> net/sched/cls_api.c | 30 +++++++++++++++++------------- >>>> 1 file changed, 17 insertions(+), 13 deletions(-) >>>> >>>> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c >>>> index 32577c2..c980127 100644 >>>> --- a/net/sched/cls_api.c >>>> +++ b/net/sched/cls_api.c >>>> @@ -607,11 +607,11 @@ static void tc_indr_block_get_and_ing_cmd(struct net_device *dev, >>>> tc_indr_block_ing_cmd(dev, block, cb, cb_priv, command); >>>> } >>>> >>>> -static void tc_indr_block_call(struct tcf_block *block, >>>> - struct net_device *dev, >>>> - struct tcf_block_ext_info *ei, >>>> - enum flow_block_command command, >>>> - struct netlink_ext_ack *extack) >>>> +static int tc_indr_block_call(struct tcf_block *block, >>>> + struct net_device *dev, >>>> + struct tcf_block_ext_info *ei, >>>> + enum flow_block_command command, >>>> + struct netlink_ext_ack *extack) >>>> { >>>> struct flow_block_offload bo = { >>>> .command = command, >>>> @@ -621,10 +621,15 @@ static void tc_indr_block_call(struct tcf_block *block, >>>> .block_shared = tcf_block_shared(block), >>>> .extack = extack, >>>> }; >>>> + >>>> INIT_LIST_HEAD(&bo.cb_list); >>>> >>>> flow_indr_block_call(dev, &bo, command); >>>> - tcf_block_setup(block, &bo); >>>> + >>>> + if (list_empty(&bo.cb_list)) >>>> + return -EOPNOTSUPP; >>>> + >>>> + return tcf_block_setup(block, &bo); >>>> } >>>> >>>> static bool tcf_block_offload_in_use(struct tcf_block *block) >>>> @@ -681,8 +686,6 @@ static int tcf_block_offload_bind(struct tcf_block *block, struct Qdisc *q, >>>> goto no_offload_dev_inc; >>>> if (err) >>>> goto err_unlock; >>>> - >>>> - tc_indr_block_call(block, dev, ei, FLOW_BLOCK_BIND, extack); >>>> up_write(&block->cb_lock); >>>> return 0; >>>> >>>> @@ -691,9 +694,10 @@ static int tcf_block_offload_bind(struct tcf_block *block, struct Qdisc *q, >>>> err = -EOPNOTSUPP; >>>> goto err_unlock; >>>> } >>>> + err = tc_indr_block_call(block, dev, ei, FLOW_BLOCK_BIND, extack); >>>> + if (err) >>>> + block->nooffloaddevcnt++; >>>> err = 0; >>>> - block->nooffloaddevcnt++; >>>> - tc_indr_block_call(block, dev, ei, FLOW_BLOCK_BIND, extack); >>>> err_unlock: >>>> up_write(&block->cb_lock); >>>> return err; >>>> @@ -706,8 +710,6 @@ static void tcf_block_offload_unbind(struct tcf_block *block, struct Qdisc *q, >>>> int err; >>>> >>>> down_write(&block->cb_lock); >>>> - tc_indr_block_call(block, dev, ei, FLOW_BLOCK_UNBIND, NULL); >>>> - >>>> if (!dev->netdev_ops->ndo_setup_tc) >>>> goto no_offload_dev_dec; >>>> err = tcf_block_offload_cmd(block, dev, ei, FLOW_BLOCK_UNBIND, NULL); >>>> @@ -717,7 +719,9 @@ static void tcf_block_offload_unbind(struct tcf_block *block, struct Qdisc *q, >>>> return; >>>> >>>> no_offload_dev_dec: >>>> - WARN_ON(block->nooffloaddevcnt-- == 0); >>>> + err = tc_indr_block_call(block, dev, ei, FLOW_BLOCK_UNBIND, NULL); >>>> + if (err) >>>> + WARN_ON(block->nooffloaddevcnt-- == 0); >>>> up_write(&block->cb_lock); >>>> } >>>> >>>> -- >>>> 1.8.3.1 >>>> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net v3] net/sched: cls_api: Fix nooffloaddevcnt counter when indr block call success 2019-09-23 14:18 ` wenxu @ 2019-09-24 10:33 ` Or Gerlitz 0 siblings, 0 replies; 7+ messages in thread From: Or Gerlitz @ 2019-09-24 10:33 UTC (permalink / raw) To: wenxu Cc: John Hurley, Pieter Jansen van Vuuren, Oz Shlomo, Jakub Kicinski, David Miller, Linux Netdev List, Roi Dayan, Paul Blakey, Jiri Pirko On Mon, Sep 23, 2019 at 5:21 PM wenxu <wenxu@ucloud.cn> wrote: > 在 2019/9/23 17:42, John Hurley 写道: > > On Mon, Sep 23, 2019 at 5:20 AM wenxu <wenxu@ucloud.cn> wrote: > >> Hi John & Jakub > >> > >> There are some limitations for indirect tc callback work with skip_sw ? > >> > > Hi Wenxu, > > This is not really a limitation. > > As Or points out, indirect block offload is not supposed to work with skip_sw. > > Indirect offload allows us to hook onto existing kernel devices (for > > TC events we may which to offload) that are out of the control of the > > offload driver and, therefore, should always accept software path > > rules. > > For example, the vxlan driver does not implement a setup_tc ndo so it > > does not expect to run rules in hw - it should always handle > > associated rules in the software datapath as a minimum. > > I think accepting skip_sw rules for devices with no in-built concept > > of hardware offload would be wrong. > > Do you have a use case that requires skip_sw rules for such devices? > When we use ovs to control the tc offload. The ovs kernel already provide the software > path rules so maybe user don't want other soft path. this (programming the rule to both tc and ovs kernel DPs) is a choice made by the ovs user-space code and could change later. Actually, for complex use case such as connection tracking, there might be cases when multiple tables are used, when the 1st packet/s would jump before hw to sw TC tables, so using skip_sw will likely not to work and we'll get bug reports. The production TC configuration we use/recommend with for overlay networks e-switching is "both" (== "none", i.e none of skip_sw or skip_hw). > And with skip_sw it can be easily distinguish offloaded and non-offloaded rules. per tc rule, the kernel reports on offloaded vs not offloaded packet/bytes, so the info is there. In the system level, I don't think it's good that on point A in time we blocked skip_sw rules for tunnel devices and at point B we enabled it for no real/good reason, I vote for blocking it again, since as Pieter explained disallowing this is not consistent with the kernel SW model on the receiving side. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-09-24 10:33 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-09-19 8:37 [PATCH net v3] net/sched: cls_api: Fix nooffloaddevcnt counter when indr block call success wenxu 2019-09-19 8:40 ` wenxu 2019-09-19 12:50 ` Or Gerlitz 2019-09-23 4:19 ` wenxu 2019-09-23 9:42 ` John Hurley 2019-09-23 14:18 ` wenxu 2019-09-24 10:33 ` Or Gerlitz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox