* [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;
as well as URLs for NNTP newsgroup(s).