From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2021-10-31 13:30:51
On 2021-10-30 22:27, Baowen Zheng wrote:
Thanks for your review, after some considerarion, I think I understand what you are meaning.
[..]
quoted
quoted
quoted
I know Jamal suggested to have skip_sw for actions, but it complicates
the code and I'm still not entirely understand why it is necessary.
If the hardware can independently accept an action offload then
skip_sw per action makes total sense. BTW, my understanding is
Example configuration that seems bizarre to me is when offloaded shared
action has skip_sw flag set but filter doesn't. Then behavior of
classifier that points to such action diverges between hardware and
software (different lists of actions are applied). We always try to make
offloaded TC data path behave exactly the same as software and, even
though here it would be explicit and deliberate, I don't see any
practical use-case for this.
We add the skip_sw to keep compatible with the filter flags and give the user an
option to specify if the action should run in software. I understand what you mean,
maybe our example is not proper, we need to prevent the filter to run in software if the
actions it applies is skip_sw, so we need to add more validation to check about this.
Also I think your suggestion makes full sense if there is no use case to specify the action
should not run in sw and indeed it will make our implement more simple if we omit the
skip_sw option.
Jamal, WDYT?
Let me use an example to illustrate my concern:
#add a policer offload it
tc actions add action police skip_sw rate ... index 20
#now add filter1 which is offloaded
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_sw ip_proto tcp action police index 20
#add filter2 likewise offloaded
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_sw ip_proto udp action police index 20
All good so far...
#Now add a filter3 which is s/w only
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_hw ip_proto icmp action police index 20
filter3 should not be allowed.
If we had added the policer without skip_sw and without
skip_hw then i think filter3 should have been legal
(we just need to account for stats in_hw vs in_sw).
Not sure if that makes sense (and addresses Vlad's earlier
comment).
cheers,
jamal
Thanks for your review, after some considerarion, I think I understand what
you are meaning.
quoted
[..]
quoted
quoted
quoted
quoted
I know Jamal suggested to have skip_sw for actions, but it
complicates the code and I'm still not entirely understand why it is
necessary.
quoted
quoted
quoted
If the hardware can independently accept an action offload then
skip_sw per action makes total sense. BTW, my understanding is
Example configuration that seems bizarre to me is when offloaded
shared action has skip_sw flag set but filter doesn't. Then behavior
of classifier that points to such action diverges between hardware
and software (different lists of actions are applied). We always try
to make offloaded TC data path behave exactly the same as software
and, even though here it would be explicit and deliberate, I don't
see any practical use-case for this.
We add the skip_sw to keep compatible with the filter flags and give
the user an option to specify if the action should run in software. I
understand what you mean, maybe our example is not proper, we need to
prevent the filter to run in software if the actions it applies is skip_sw, so we
need to add more validation to check about this.
quoted
Also I think your suggestion makes full sense if there is no use case
to specify the action should not run in sw and indeed it will make our
implement more simple if we omit the skip_sw option.
Jamal, WDYT?
Let me use an example to illustrate my concern:
#add a policer offload it
tc actions add action police skip_sw rate ... index 20 #now add filter1 which is
offloaded tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_sw ip_proto tcp action police index 20 #add filter2 likewise offloaded
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_sw ip_proto udp action police index 20
All good so far...
#Now add a filter3 which is s/w only
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_hw ip_proto icmp action police index 20
filter3 should not be allowed.
If we had added the policer without skip_sw and without skip_hw then i think
filter3 should have been legal (we just need to account for stats in_hw vs
in_sw).
Not sure if that makes sense (and addresses Vlad's earlier comment).
I think the cases you mentioned make sense to us. But what Vlad concerns is the use
case as:
#add a policer offload it
tc actions add action police skip_sw rate ... index 20
#now add filter4 which can't be offloaded
tc filter add dev $DEV1 proto ip parent ffff: flower \
ip_proto tcp action police index 20
it is possible the filter4 can't be offloaded, then filter4 will run in software,
should this be legal?
Originally I think this is legal, but as comments of Vlad, this should not be legal, since the action
will not be executed in software. I think what Vlad concerns is do we really need skip_sw flag for
an action? If a packet matches the filter in software, the action should not be skip_sw.
If we choose to omit the skip_sw flag and just keep skip_hw, it will simplify our work.
Of course, we can also keep skip_sw by adding more check to avoid the above case.
Vlad, I am not sure if I understand your idea correctly.
On Mon 01 Nov 2021 at 05:29, Baowen Zheng [off-list ref] wrote:
On 2021-10-31 9:31 PM, Jamal Hadi Salim wrote:
quoted
On 2021-10-30 22:27, Baowen Zheng wrote:
quoted
Thanks for your review, after some considerarion, I think I understand what
you are meaning.
quoted
[..]
quoted
quoted
quoted
quoted
I know Jamal suggested to have skip_sw for actions, but it
complicates the code and I'm still not entirely understand why it is
necessary.
quoted
quoted
quoted
If the hardware can independently accept an action offload then
skip_sw per action makes total sense. BTW, my understanding is
Example configuration that seems bizarre to me is when offloaded
shared action has skip_sw flag set but filter doesn't. Then behavior
of classifier that points to such action diverges between hardware
and software (different lists of actions are applied). We always try
to make offloaded TC data path behave exactly the same as software
and, even though here it would be explicit and deliberate, I don't
see any practical use-case for this.
We add the skip_sw to keep compatible with the filter flags and give
the user an option to specify if the action should run in software. I
understand what you mean, maybe our example is not proper, we need to
prevent the filter to run in software if the actions it applies is skip_sw, so we
need to add more validation to check about this.
quoted
Also I think your suggestion makes full sense if there is no use case
to specify the action should not run in sw and indeed it will make our
implement more simple if we omit the skip_sw option.
Jamal, WDYT?
Let me use an example to illustrate my concern:
#add a policer offload it
tc actions add action police skip_sw rate ... index 20 #now add filter1 which is
offloaded tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_sw ip_proto tcp action police index 20 #add filter2 likewise offloaded
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_sw ip_proto udp action police index 20
All good so far...
#Now add a filter3 which is s/w only
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_hw ip_proto icmp action police index 20
filter3 should not be allowed.
If we had added the policer without skip_sw and without skip_hw then i think
filter3 should have been legal (we just need to account for stats in_hw vs
in_sw).
Not sure if that makes sense (and addresses Vlad's earlier comment).
I think the cases you mentioned make sense to us. But what Vlad concerns is the use
case as:
#add a policer offload it
tc actions add action police skip_sw rate ... index 20
#now add filter4 which can't be offloaded
tc filter add dev $DEV1 proto ip parent ffff: flower \
ip_proto tcp action police index 20
it is possible the filter4 can't be offloaded, then filter4 will run in software,
should this be legal?
Originally I think this is legal, but as comments of Vlad, this should not be legal, since the action
will not be executed in software. I think what Vlad concerns is do we really need skip_sw flag for
an action? If a packet matches the filter in software, the action should not be skip_sw.
If we choose to omit the skip_sw flag and just keep skip_hw, it will simplify our work.
Of course, we can also keep skip_sw by adding more check to avoid the above case.
Vlad, I am not sure if I understand your idea correctly.
My suggestion was to forgo the skip_sw flag for shared action offload
and, consecutively, remove the validation code, not to add even more
checks. I still don't see a practical case where skip_sw shared action
is useful. But I don't have any strong feelings about this flag, so if
Jamal thinks it is necessary, then fine by me.
From: Simon Horman <hidden> Date: 2021-11-02 12:40:10
On Mon, Nov 01, 2021 at 09:38:34AM +0200, Vlad Buslov wrote:
On Mon 01 Nov 2021 at 05:29, Baowen Zheng [off-list ref] wrote:
quoted
On 2021-10-31 9:31 PM, Jamal Hadi Salim wrote:
quoted
On 2021-10-30 22:27, Baowen Zheng wrote:
quoted
Thanks for your review, after some considerarion, I think I understand what
..
quoted
quoted
Let me use an example to illustrate my concern:
#add a policer offload it
tc actions add action police skip_sw rate ... index 20 #now add filter1 which is
offloaded tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_sw ip_proto tcp action police index 20 #add filter2 likewise offloaded
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_sw ip_proto udp action police index 20
All good so far...
#Now add a filter3 which is s/w only
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_hw ip_proto icmp action police index 20
filter3 should not be allowed.
If we had added the policer without skip_sw and without skip_hw then i think
filter3 should have been legal (we just need to account for stats in_hw vs
in_sw).
Not sure if that makes sense (and addresses Vlad's earlier comment).
I think the cases you mentioned make sense to us. But what Vlad concerns is the use
case as:
#add a policer offload it
tc actions add action police skip_sw rate ... index 20
#now add filter4 which can't be offloaded
tc filter add dev $DEV1 proto ip parent ffff: flower \
ip_proto tcp action police index 20
it is possible the filter4 can't be offloaded, then filter4 will run in software,
should this be legal?
Originally I think this is legal, but as comments of Vlad, this should not be legal, since the action
will not be executed in software. I think what Vlad concerns is do we really need skip_sw flag for
an action? If a packet matches the filter in software, the action should not be skip_sw.
If we choose to omit the skip_sw flag and just keep skip_hw, it will simplify our work.
Of course, we can also keep skip_sw by adding more check to avoid the above case.
Vlad, I am not sure if I understand your idea correctly.
My suggestion was to forgo the skip_sw flag for shared action offload
and, consecutively, remove the validation code, not to add even more
checks. I still don't see a practical case where skip_sw shared action
is useful. But I don't have any strong feelings about this flag, so if
Jamal thinks it is necessary, then fine by me.
FWIIW, my feelings are the same as Vlad's.
I think these flags add complexity that would be nice to avoid.
But if Jamal thinks its necessary, then including the flags implementation
is fine by me.
On Mon, Nov 01, 2021 at 09:38:34AM +0200, Vlad Buslov wrote:
quoted
On Mon 01 Nov 2021 at 05:29, Baowen Zheng
[off-list ref] wrote:
quoted
quoted
On 2021-10-31 9:31 PM, Jamal Hadi Salim wrote:
quoted
On 2021-10-30 22:27, Baowen Zheng wrote:
quoted
Thanks for your review, after some considerarion, I think I
understand what
..
quoted
quoted
quoted
Let me use an example to illustrate my concern:
#add a policer offload it
tc actions add action police skip_sw rate ... index 20 #now add
filter1 which is offloaded tc filter add dev $DEV1 proto ip parent ffff:
flower \
quoted
quoted
quoted
skip_sw ip_proto tcp action police index 20 #add filter2
likewise offloaded tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_sw ip_proto udp action police index 20
All good so far...
#Now add a filter3 which is s/w only tc filter add dev $DEV1 proto
ip parent ffff: flower \
skip_hw ip_proto icmp action police index 20
filter3 should not be allowed.
If we had added the policer without skip_sw and without skip_hw then
i think
filter3 should have been legal (we just need to account for stats
in_hw vs in_sw).
Not sure if that makes sense (and addresses Vlad's earlier comment).
I think the cases you mentioned make sense to us. But what Vlad
concerns is the use case as:
#add a policer offload it
tc actions add action police skip_sw rate ... index 20 #now add
filter4 which can't be offloaded tc filter add dev $DEV1 proto ip
parent ffff: flower \ ip_proto tcp action police index 20 it is
possible the filter4 can't be offloaded, then filter4 will run in
software, should this be legal?
Originally I think this is legal, but as comments of Vlad, this
should not be legal, since the action will not be executed in
software. I think what Vlad concerns is do we really need skip_sw flag for
an action? If a packet matches the filter in software, the action should not be
skip_sw.
quoted
quoted
If we choose to omit the skip_sw flag and just keep skip_hw, it will simplify
our work.
quoted
quoted
Of course, we can also keep skip_sw by adding more check to avoid the
above case.
quoted
quoted
Vlad, I am not sure if I understand your idea correctly.
My suggestion was to forgo the skip_sw flag for shared action offload
and, consecutively, remove the validation code, not to add even more
checks. I still don't see a practical case where skip_sw shared action
is useful. But I don't have any strong feelings about this flag, so if
Jamal thinks it is necessary, then fine by me.
FWIIW, my feelings are the same as Vlad's.
I think these flags add complexity that would be nice to avoid.
But if Jamal thinks its necessary, then including the flags implementation is
fine by me.
Thanks Simon. Jamal, do you think it is necessary to keep the skip_sw flag for user to specify
the action should not run in software?
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2021-11-03 10:13:58
On 2021-11-03 03:57, Baowen Zheng wrote:
On November 2, 2021 8:40 PM, Simon Horman wrote:
quoted
On Mon, Nov 01, 2021 at 09:38:34AM +0200, Vlad Buslov wrote:
quoted
On Mon 01 Nov 2021 at 05:29, Baowen Zheng
[..]
quoted
quoted
My suggestion was to forgo the skip_sw flag for shared action offload
and, consecutively, remove the validation code, not to add even more
checks. I still don't see a practical case where skip_sw shared action
is useful. But I don't have any strong feelings about this flag, so if
Jamal thinks it is necessary, then fine by me.
FWIIW, my feelings are the same as Vlad's.
I think these flags add complexity that would be nice to avoid.
But if Jamal thinks its necessary, then including the flags implementation is
fine by me.
Thanks Simon. Jamal, do you think it is necessary to keep the skip_sw flag for user to specify
the action should not run in software?
Just catching up with discussion...
IMO, we need the flag. Oz indicated with requirement to be able to
identify the action with an index. So if a specific action is added
for skip_sw (as standalone or alongside a filter) then it cant be
used for skip_hw. To illustrate using extended example:
#filter 1, skip_sw
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_sw ip_proto tcp action police blah index 10
#filter 2, skip_hw
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_hw ip_proto udp action police index 10
Filter2 should be illegal.
And when i dump the actions as so:
tc actions ls action police
For debugability, I should see index 10 clearly marked with
the flag as skip_sw
The other example i gave earlier which showed the sharing
of actions:
#add a policer action and offload it
tc actions add action police skip_sw rate ... index 20
#now add filter1 which is offloaded using offloaded policer
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_sw ip_proto tcp action police index 20
#add filter2 likewise offloaded
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_sw ip_proto udp action police index 20
All good and filter 1 and 2 are sharing policer instance with
index 20.
#Now add a filter3 which is s/w only
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_hw ip_proto icmp action police index 20
filter3 should not be allowed.
cheers,
jamal
On November 3, 2021 6:14 PM, Jamal Hadi Salim wrote:
On 2021-11-03 03:57, Baowen Zheng wrote:
quoted
On November 2, 2021 8:40 PM, Simon Horman wrote:
quoted
On Mon, Nov 01, 2021 at 09:38:34AM +0200, Vlad Buslov wrote:
quoted
On Mon 01 Nov 2021 at 05:29, Baowen Zheng
[..]
quoted
quoted
quoted
My suggestion was to forgo the skip_sw flag for shared action
offload and, consecutively, remove the validation code, not to add
even more checks. I still don't see a practical case where skip_sw
shared action is useful. But I don't have any strong feelings about
this flag, so if Jamal thinks it is necessary, then fine by me.
FWIIW, my feelings are the same as Vlad's.
I think these flags add complexity that would be nice to avoid.
But if Jamal thinks its necessary, then including the flags
implementation is fine by me.
Thanks Simon. Jamal, do you think it is necessary to keep the skip_sw
flag for user to specify the action should not run in software?
Just catching up with discussion...
IMO, we need the flag. Oz indicated with requirement to be able to identify
the action with an index. So if a specific action is added for skip_sw (as
standalone or alongside a filter) then it cant be used for skip_hw. To illustrate
using extended example:
#filter 1, skip_sw
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_sw ip_proto tcp action police blah index 10
#filter 2, skip_hw
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_hw ip_proto udp action police index 10
Filter2 should be illegal.
And when i dump the actions as so:
tc actions ls action police
For debugability, I should see index 10 clearly marked with the flag as skip_sw
The other example i gave earlier which showed the sharing of actions:
#add a policer action and offload it
tc actions add action police skip_sw rate ... index 20 #now add filter1 which is
offloaded using offloaded policer tc filter add dev $DEV1 proto ip parent ffff:
flower \
skip_sw ip_proto tcp action police index 20 #add filter2 likewise offloaded
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_sw ip_proto udp action police index 20
All good and filter 1 and 2 are sharing policer instance with index 20.
#Now add a filter3 which is s/w only
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_hw ip_proto icmp action police index 20
filter3 should not be allowed.
I think the use cases you mentioned above are clear for us. For the case:
#add a policer action and offload it
tc actions add action police skip_sw rate ... index 20
#Now add a filter4 which has no flag
tc filter add dev $DEV1 proto ip parent ffff: flower \
ip_proto icmp action police index 20
Is filter4 legal? basically, it should be legal, but since filter4 may be offloaded failed so
it will run in software, you know the action police should not run in software with skip_sw,
so I think filter4 should be illegal and we should not allow this case.
That is if the action is skip_sw, then the filter refers to this action should also skip_sw.
WDYT?
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2021-11-03 12:33:57
On 2021-11-03 07:30, Baowen Zheng wrote:
On November 3, 2021 6:14 PM, Jamal Hadi Salim wrote:
quoted
On 2021-11-03 03:57, Baowen Zheng wrote:
quoted
On November 2, 2021 8:40 PM, Simon Horman wrote:
quoted
On Mon, Nov 01, 2021 at 09:38:34AM +0200, Vlad Buslov wrote:
quoted
On Mon 01 Nov 2021 at 05:29, Baowen Zheng
[..]
quoted
quoted
quoted
My suggestion was to forgo the skip_sw flag for shared action
offload and, consecutively, remove the validation code, not to add
even more checks. I still don't see a practical case where skip_sw
shared action is useful. But I don't have any strong feelings about
this flag, so if Jamal thinks it is necessary, then fine by me.
FWIIW, my feelings are the same as Vlad's.
I think these flags add complexity that would be nice to avoid.
But if Jamal thinks its necessary, then including the flags
implementation is fine by me.
Thanks Simon. Jamal, do you think it is necessary to keep the skip_sw
flag for user to specify the action should not run in software?
Just catching up with discussion...
IMO, we need the flag. Oz indicated with requirement to be able to identify
the action with an index. So if a specific action is added for skip_sw (as
standalone or alongside a filter) then it cant be used for skip_hw. To illustrate
using extended example:
#filter 1, skip_sw
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_sw ip_proto tcp action police blah index 10
#filter 2, skip_hw
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_hw ip_proto udp action police index 10
Filter2 should be illegal.
And when i dump the actions as so:
tc actions ls action police
For debugability, I should see index 10 clearly marked with the flag as skip_sw
The other example i gave earlier which showed the sharing of actions:
#add a policer action and offload it
tc actions add action police skip_sw rate ... index 20 #now add filter1 which is
offloaded using offloaded policer tc filter add dev $DEV1 proto ip parent ffff:
flower \
skip_sw ip_proto tcp action police index 20 #add filter2 likewise offloaded
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_sw ip_proto udp action police index 20
All good and filter 1 and 2 are sharing policer instance with index 20.
#Now add a filter3 which is s/w only
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_hw ip_proto icmp action police index 20
filter3 should not be allowed.
I think the use cases you mentioned above are clear for us. For the case:
#add a policer action and offload it
tc actions add action police skip_sw rate ... index 20
#Now add a filter4 which has no flag
tc filter add dev $DEV1 proto ip parent ffff: flower \
ip_proto icmp action police index 20
Is filter4 legal?
Yes it is _based on current semantics_.
The reason is when adding a filter and specifying neither
skip_sw nor skip_hw it defaults to allowing both.
i.e is the same as skip_sw|skip_hw. You will need to have
counters for both s/w and h/w (which i think is taken care of today).
basically, it should be legal, but since filter4 may be offloaded failed so
it will run in software, you know the action police should not run in software with skip_sw,
so I think filter4 should be illegal and we should not allow this case.
That is if the action is skip_sw, then the filter refers to this action should also skip_sw.
WDYT?
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2021-11-03 13:33:57
On 2021-11-03 08:33, Jamal Hadi Salim wrote:
On 2021-11-03 07:30, Baowen Zheng wrote:
quoted
On November 3, 2021 6:14 PM, Jamal Hadi Salim wrote:
quoted
On 2021-11-03 03:57, Baowen Zheng wrote:
quoted
On November 2, 2021 8:40 PM, Simon Horman wrote:
quoted
On Mon, Nov 01, 2021 at 09:38:34AM +0200, Vlad Buslov wrote:
quoted
On Mon 01 Nov 2021 at 05:29, Baowen Zheng
[..]
quoted
quoted
quoted
My suggestion was to forgo the skip_sw flag for shared action
offload and, consecutively, remove the validation code, not to add
even more checks. I still don't see a practical case where skip_sw
shared action is useful. But I don't have any strong feelings about
this flag, so if Jamal thinks it is necessary, then fine by me.
FWIIW, my feelings are the same as Vlad's.
I think these flags add complexity that would be nice to avoid.
But if Jamal thinks its necessary, then including the flags
implementation is fine by me.
Thanks Simon. Jamal, do you think it is necessary to keep the skip_sw
flag for user to specify the action should not run in software?
Just catching up with discussion...
IMO, we need the flag. Oz indicated with requirement to be able to identify
the action with an index. So if a specific action is added for skip_sw (as
standalone or alongside a filter) then it cant be used for skip_hw. To illustrate
using extended example:
#filter 1, skip_sw
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_sw ip_proto tcp action police blah index 10
#filter 2, skip_hw
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_hw ip_proto udp action police index 10
Filter2 should be illegal.
And when i dump the actions as so:
tc actions ls action police
For debugability, I should see index 10 clearly marked with the flag as skip_sw
The other example i gave earlier which showed the sharing of actions:
#add a policer action and offload it
tc actions add action police skip_sw rate ... index 20 #now add filter1 which is
offloaded using offloaded policer tc filter add dev $DEV1 proto ip parent ffff:
flower \
skip_sw ip_proto tcp action police index 20 #add filter2 likewise offloaded
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_sw ip_proto udp action police index 20
All good and filter 1 and 2 are sharing policer instance with index 20.
#Now add a filter3 which is s/w only
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_hw ip_proto icmp action police index 20
filter3 should not be allowed.
I think the use cases you mentioned above are clear for us. For the case:
#add a policer action and offload it
tc actions add action police skip_sw rate ... index 20
#Now add a filter4 which has no flag
tc filter add dev $DEV1 proto ip parent ffff: flower \
ip_proto icmp action police index 20
Is filter4 legal?
Yes it is _based on current semantics_.
The reason is when adding a filter and specifying neither
skip_sw nor skip_hw it defaults to allowing both.
i.e is the same as skip_sw|skip_hw. You will need to have
counters for both s/w and h/w (which i think is taken care of today).
Apologies, i will like to take this one back. Couldnt stop thinking
about it while sipping coffee;->
To be safe that should be illegal. The flags have to match _exactly_
for both action and filter to make any sense. i.e in the above case
they are not.
cheers,
jamal
On November 3, 2021 8:34 PM, Jamal Hadi Salim wrote:
On 2021-11-03 07:30, Baowen Zheng wrote:
quoted
On November 3, 2021 6:14 PM, Jamal Hadi Salim wrote:
quoted
On 2021-11-03 03:57, Baowen Zheng wrote:
quoted
On November 2, 2021 8:40 PM, Simon Horman wrote:
quoted
On Mon, Nov 01, 2021 at 09:38:34AM +0200, Vlad Buslov wrote:
quoted
On Mon 01 Nov 2021 at 05:29, Baowen Zheng
[..]
quoted
quoted
quoted
My suggestion was to forgo the skip_sw flag for shared action
offload and, consecutively, remove the validation code, not to add
even more checks. I still don't see a practical case where skip_sw
shared action is useful. But I don't have any strong feelings
about this flag, so if Jamal thinks it is necessary, then fine by me.
FWIIW, my feelings are the same as Vlad's.
I think these flags add complexity that would be nice to avoid.
But if Jamal thinks its necessary, then including the flags
implementation is fine by me.
Thanks Simon. Jamal, do you think it is necessary to keep the
skip_sw flag for user to specify the action should not run in software?
Just catching up with discussion...
IMO, we need the flag. Oz indicated with requirement to be able to
identify the action with an index. So if a specific action is added
for skip_sw (as standalone or alongside a filter) then it cant be
used for skip_hw. To illustrate using extended example:
#filter 1, skip_sw
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_sw ip_proto tcp action police blah index 10
#filter 2, skip_hw
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_hw ip_proto udp action police index 10
Filter2 should be illegal.
And when i dump the actions as so:
tc actions ls action police
For debugability, I should see index 10 clearly marked with the flag
as skip_sw
The other example i gave earlier which showed the sharing of actions:
#add a policer action and offload it
tc actions add action police skip_sw rate ... index 20 #now add
filter1 which is offloaded using offloaded policer tc filter add dev $DEV1
proto ip parent ffff:
quoted
quoted
flower \
skip_sw ip_proto tcp action police index 20 #add filter2
likewise offloaded tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_sw ip_proto udp action police index 20
All good and filter 1 and 2 are sharing policer instance with index 20.
#Now add a filter3 which is s/w only
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_hw ip_proto icmp action police index 20
filter3 should not be allowed.
I think the use cases you mentioned above are clear for us. For the case:
#add a policer action and offload it
tc actions add action police skip_sw rate ... index 20 #Now add a
filter4 which has no flag tc filter add dev $DEV1 proto ip parent
ffff: flower \
ip_proto icmp action police index 20
Is filter4 legal?
Yes it is _based on current semantics_.
The reason is when adding a filter and specifying neither skip_sw nor skip_hw
it defaults to allowing both.
i.e is the same as skip_sw|skip_hw. You will need to have counters for both
s/w and h/w (which i think is taken care of today).
Thanks, but what we concern is not the counters but the behavior of this filter.
Since the filter runs in software and action is skip_sw, so the action will not execute in software.
So when the packet matches the filter, it will execute all the actions except the skip_sw action.
I think it is not what we expect, we expect the packet execute all the actions the filter refers to.
So I think in this case, filter4 should not be allowed.
WDYT?
quoted
basically, it should be legal, but since filter4 may be offloaded
failed so it will run in software, you know the action police should
not run in software with skip_sw, so I think filter4 should be illegal and we
should not allow this case.
quoted
That is if the action is skip_sw, then the filter refers to this action should also
From: Simon Horman <hidden> Date: 2021-11-03 13:38:29
On Wed, Nov 03, 2021 at 09:33:52AM -0400, Jamal Hadi Salim wrote:
On 2021-11-03 08:33, Jamal Hadi Salim wrote:
quoted
On 2021-11-03 07:30, Baowen Zheng wrote:
quoted
On November 3, 2021 6:14 PM, Jamal Hadi Salim wrote:
quoted
On 2021-11-03 03:57, Baowen Zheng wrote:
quoted
On November 2, 2021 8:40 PM, Simon Horman wrote:
quoted
On Mon, Nov 01, 2021 at 09:38:34AM +0200, Vlad Buslov wrote:
quoted
On Mon 01 Nov 2021 at 05:29, Baowen Zheng
[..]
quoted
quoted
quoted
My suggestion was to forgo the skip_sw flag for shared action
offload and, consecutively, remove the validation code, not to add
even more checks. I still don't see a practical case where skip_sw
shared action is useful. But I don't have any strong feelings about
this flag, so if Jamal thinks it is necessary, then fine by me.
FWIIW, my feelings are the same as Vlad's.
I think these flags add complexity that would be nice to avoid.
But if Jamal thinks its necessary, then including the flags
implementation is fine by me.
Thanks Simon. Jamal, do you think it is necessary to keep the skip_sw
flag for user to specify the action should not run in software?
Just catching up with discussion...
IMO, we need the flag. Oz indicated with requirement to be able
to identify
the action with an index. So if a specific action is added for
skip_sw (as
standalone or alongside a filter) then it cant be used for
skip_hw. To illustrate
using extended example:
#filter 1, skip_sw
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_sw ip_proto tcp action police blah index 10
#filter 2, skip_hw
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_hw ip_proto udp action police index 10
Filter2 should be illegal.
And when i dump the actions as so:
tc actions ls action police
For debugability, I should see index 10 clearly marked with the
flag as skip_sw
The other example i gave earlier which showed the sharing of actions:
#add a policer action and offload it
tc actions add action police skip_sw rate ... index 20 #now add
filter1 which is
offloaded using offloaded policer tc filter add dev $DEV1 proto
ip parent ffff:
flower \
skip_sw ip_proto tcp action police index 20 #add filter2
likewise offloaded
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_sw ip_proto udp action police index 20
All good and filter 1 and 2 are sharing policer instance with index 20.
#Now add a filter3 which is s/w only
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_hw ip_proto icmp action police index 20
filter3 should not be allowed.
I think the use cases you mentioned above are clear for us. For the case:
#add a policer action and offload it
tc actions add action police skip_sw rate ... index 20
#Now add a filter4 which has no flag
tc filter add dev $DEV1 proto ip parent ffff: flower \
ip_proto icmp action police index 20
Is filter4 legal?
Yes it is _based on current semantics_.
The reason is when adding a filter and specifying neither
skip_sw nor skip_hw it defaults to allowing both.
i.e is the same as skip_sw|skip_hw. You will need to have
counters for both s/w and h/w (which i think is taken care of today).
Apologies, i will like to take this one back. Couldnt stop thinking
about it while sipping coffee;->
To be safe that should be illegal. The flags have to match _exactly_
for both action and filter to make any sense. i.e in the above case
they are not.
I could be wrong, but I would have thought that in this case the flow
is legal but is only added to hw (because the action doesn't exist in sw).
But if you prefer to make it illegal I guess that is ok too.
Thanks for your reply.
On November 3, 2021 9:34 PM, Jamal Hadi Salim wrote:
On 2021-11-03 08:33, Jamal Hadi Salim wrote:
quoted
On 2021-11-03 07:30, Baowen Zheng wrote:
quoted
On November 3, 2021 6:14 PM, Jamal Hadi Salim wrote:
quoted
On 2021-11-03 03:57, Baowen Zheng wrote:
quoted
On November 2, 2021 8:40 PM, Simon Horman wrote:
quoted
On Mon, Nov 01, 2021 at 09:38:34AM +0200, Vlad Buslov wrote:
quoted
On Mon 01 Nov 2021 at 05:29, Baowen Zheng
[..]
quoted
quoted
quoted
My suggestion was to forgo the skip_sw flag for shared action
offload and, consecutively, remove the validation code, not to
add even more checks. I still don't see a practical case where
skip_sw shared action is useful. But I don't have any strong
feelings about this flag, so if Jamal thinks it is necessary, then fine by
me.
quoted
quoted
quoted
quoted
quoted
FWIIW, my feelings are the same as Vlad's.
I think these flags add complexity that would be nice to avoid.
But if Jamal thinks its necessary, then including the flags
implementation is fine by me.
Thanks Simon. Jamal, do you think it is necessary to keep the
skip_sw flag for user to specify the action should not run in software?
Just catching up with discussion...
IMO, we need the flag. Oz indicated with requirement to be able to
identify the action with an index. So if a specific action is added
for skip_sw (as standalone or alongside a filter) then it cant be
used for skip_hw.
To illustrate
using extended example:
#filter 1, skip_sw
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_sw ip_proto tcp action police blah index 10
#filter 2, skip_hw
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_hw ip_proto udp action police index 10
Filter2 should be illegal.
And when i dump the actions as so:
tc actions ls action police
For debugability, I should see index 10 clearly marked with the flag
as skip_sw
The other example i gave earlier which showed the sharing of actions:
#add a policer action and offload it tc actions add action police
skip_sw rate ... index 20 #now add
filter1 which is
offloaded using offloaded policer tc filter add dev $DEV1 proto ip
parent ffff:
flower \
skip_sw ip_proto tcp action police index 20 #add filter2
likewise offloaded tc filter add dev $DEV1 proto ip parent ffff:
flower \
skip_sw ip_proto udp action police index 20
All good and filter 1 and 2 are sharing policer instance with index 20.
#Now add a filter3 which is s/w only tc filter add dev $DEV1 proto
ip parent ffff: flower \
skip_hw ip_proto icmp action police index 20
filter3 should not be allowed.
I think the use cases you mentioned above are clear for us. For the case:
#add a policer action and offload it
tc actions add action police skip_sw rate ... index 20 #Now add a
filter4 which has no flag tc filter add dev $DEV1 proto ip parent
ffff: flower \
ip_proto icmp action police index 20
Is filter4 legal?
Yes it is _based on current semantics_.
The reason is when adding a filter and specifying neither skip_sw nor
skip_hw it defaults to allowing both.
i.e is the same as skip_sw|skip_hw. You will need to have counters for
both s/w and h/w (which i think is taken care of today).
Apologies, i will like to take this one back. Couldnt stop thinking about it while
sipping coffee;-> To be safe that should be illegal. The flags have to match
_exactly_ for both action and filter to make any sense. i.e in the above case
they are not.
Thanks. I think we have get agreement that filter4 is illegal.
Sorry for more clarification about another case that Vlad mentioned:
#add a policer action with skip_hw
tc actions add action police skip_hw rate ... index 20
#Now add a filter5 which has no flag
tc filter add dev $DEV1 proto ip parent ffff: flower \
ip_proto icmp action police index 20
I think the filter5 could be legal, since it will not run in hardware.
Driver will check failed when try to offload this filter. So the filter5 will only run in software.
WDYT?
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2021-11-03 14:05:04
On 2021-11-03 09:38, Simon Horman wrote:
On Wed, Nov 03, 2021 at 09:33:52AM -0400, Jamal Hadi Salim wrote:
quoted
On 2021-11-03 08:33, Jamal Hadi Salim wrote:
quoted
On 2021-11-03 07:30, Baowen Zheng wrote:
quoted
On November 3, 2021 6:14 PM, Jamal Hadi Salim wrote:
quoted
On 2021-11-03 03:57, Baowen Zheng wrote:
quoted
On November 2, 2021 8:40 PM, Simon Horman wrote:
quoted
On Mon, Nov 01, 2021 at 09:38:34AM +0200, Vlad Buslov wrote:
quoted
On Mon 01 Nov 2021 at 05:29, Baowen Zheng
[..]
quoted
quoted
quoted
My suggestion was to forgo the skip_sw flag for shared action
offload and, consecutively, remove the validation code, not to add
even more checks. I still don't see a practical case where skip_sw
shared action is useful. But I don't have any strong feelings about
this flag, so if Jamal thinks it is necessary, then fine by me.
FWIIW, my feelings are the same as Vlad's.
I think these flags add complexity that would be nice to avoid.
But if Jamal thinks its necessary, then including the flags
implementation is fine by me.
Thanks Simon. Jamal, do you think it is necessary to keep the skip_sw
flag for user to specify the action should not run in software?
Just catching up with discussion...
IMO, we need the flag. Oz indicated with requirement to be able
to identify
the action with an index. So if a specific action is added for
skip_sw (as
standalone or alongside a filter) then it cant be used for
skip_hw. To illustrate
using extended example:
#filter 1, skip_sw
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_sw ip_proto tcp action police blah index 10
#filter 2, skip_hw
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_hw ip_proto udp action police index 10
Filter2 should be illegal.
And when i dump the actions as so:
tc actions ls action police
For debugability, I should see index 10 clearly marked with the
flag as skip_sw
The other example i gave earlier which showed the sharing of actions:
#add a policer action and offload it
tc actions add action police skip_sw rate ... index 20 #now add
filter1 which is
offloaded using offloaded policer tc filter add dev $DEV1 proto
ip parent ffff:
flower \
skip_sw ip_proto tcp action police index 20 #add filter2
likewise offloaded
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_sw ip_proto udp action police index 20
All good and filter 1 and 2 are sharing policer instance with index 20.
#Now add a filter3 which is s/w only
tc filter add dev $DEV1 proto ip parent ffff: flower \
skip_hw ip_proto icmp action police index 20
filter3 should not be allowed.
I think the use cases you mentioned above are clear for us. For the case:
#add a policer action and offload it
tc actions add action police skip_sw rate ... index 20
#Now add a filter4 which has no flag
tc filter add dev $DEV1 proto ip parent ffff: flower \
ip_proto icmp action police index 20
Is filter4 legal?
Yes it is _based on current semantics_.
The reason is when adding a filter and specifying neither
skip_sw nor skip_hw it defaults to allowing both.
i.e is the same as skip_sw|skip_hw. You will need to have
counters for both s/w and h/w (which i think is taken care of today).
Apologies, i will like to take this one back. Couldnt stop thinking
about it while sipping coffee;->
To be safe that should be illegal. The flags have to match _exactly_
for both action and filter to make any sense. i.e in the above case
they are not.
I could be wrong, but I would have thought that in this case the flow
is legal but is only added to hw (because the action doesn't exist in sw).
I was worried what would show up in a dump of the filter.
Would it show only the h/w counter? And if yes, is the s/w
version mutated with no policer (since the policer is only
in h/w)?
But if you prefer to make it illegal I guess that is ok too.
It just seemed easier from manageability pov to make it illegal, no?
i.e if flags dont match exactly it is illegal is a simple check.
cheers,
jamal
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2021-11-03 14:16:06
On 2021-11-03 10:03, Baowen Zheng wrote:
Thanks for your reply.
On November 3, 2021 9:34 PM, Jamal Hadi Salim wrote:
quoted
On 2021-11-03 08:33, Jamal Hadi Salim wrote:
quoted
On 2021-11-03 07:30, Baowen Zheng wrote:
quoted
On November 3, 2021 6:14 PM, Jamal Hadi Salim wrote:
[..]
Sorry for more clarification about another case that Vlad mentioned:
#add a policer action with skip_hw
tc actions add action police skip_hw rate ... index 20
#Now add a filter5 which has no flag
tc filter add dev $DEV1 proto ip parent ffff: flower \
ip_proto icmp action police index 20
I think the filter5 could be legal, since it will not run in hardware.
Driver will check failed when try to offload this filter. So the filter5 will only run in software.
WDYT?
I think this one also has ambiguity. If the filter doesnt specify skip_sw or skip_hw it will run both in s/w and h/w. I am worried if
that looks suprising to someone debugging after because in h/w
there is filter 5 but no policer but in s/w twin we have filter 5
and policer index 20.
It could be design intent, but in my opinion we have priorities
to resolve such ambiguities in policies.
If we use the rule which says the flags have to match exactly then we
can simplify resolving any ambiguity - which will make it illegal, no?
cheers,
jamal
On November 3, 2021 10:16 PM, Jamal Hadi Salim wrote:
On 2021-11-03 10:03, Baowen Zheng wrote:
quoted
Thanks for your reply.
On November 3, 2021 9:34 PM, Jamal Hadi Salim wrote:
quoted
On 2021-11-03 08:33, Jamal Hadi Salim wrote:
quoted
On 2021-11-03 07:30, Baowen Zheng wrote:
quoted
On November 3, 2021 6:14 PM, Jamal Hadi Salim wrote:
[..]
quoted
Sorry for more clarification about another case that Vlad mentioned:
#add a policer action with skip_hw
tc actions add action police skip_hw rate ... index 20 #Now add a
filter5 which has no flag tc filter add dev $DEV1 proto ip parent
ffff: flower \
ip_proto icmp action police index 20 I think the filter5 could
be legal, since it will not run in hardware.
Driver will check failed when try to offload this filter. So the filter5 will only
run in software.
quoted
WDYT?
I think this one also has ambiguity. If the filter doesnt specify skip_sw or
skip_hw it will run both in s/w and h/w. I am worried if that looks suprising to
someone debugging after because in h/w there is filter 5 but no policer but in
s/w twin we have filter 5 and policer index 20.
In this case, the filter will not in h/w because when the driver tries to offload the filter,
It will found the action is not in h/w and return failed, then the filter will not in h/w, so the filter will only
In software.
It could be design intent, but in my opinion we have priorities to resolve such
ambiguities in policies.
If we use the rule which says the flags have to match exactly then we can
simplify resolving any ambiguity - which will make it illegal, no?
When you mentioned " match exactly ", do you mean the flags of the filter and the actions should be
exactly same?
Please consider the case that filter has flag and the action does not have any flag. I think we should allow this case.
Because it is legal before our patch, we do not expect to break this use case, yes?
So maybe the "match exactly" just limits action flags, when action has flags(skip_sw or skip_hw), the filter must have
exactly the same flags.
WDYT?
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2021-11-03 15:35:17
On 2021-11-03 10:48, Baowen Zheng wrote:
On November 3, 2021 10:16 PM, Jamal Hadi Salim wrote:
quoted
On 2021-11-03 10:03, Baowen Zheng wrote:
quoted
Thanks for your reply.
On November 3, 2021 9:34 PM, Jamal Hadi Salim wrote:
quoted
On 2021-11-03 08:33, Jamal Hadi Salim wrote:
quoted
On 2021-11-03 07:30, Baowen Zheng wrote:
quoted
On November 3, 2021 6:14 PM, Jamal Hadi Salim wrote:
[..]
quoted
Sorry for more clarification about another case that Vlad mentioned:
#add a policer action with skip_hw
tc actions add action police skip_hw rate ... index 20 #Now add a
filter5 which has no flag tc filter add dev $DEV1 proto ip parent
ffff: flower \
ip_proto icmp action police index 20 I think the filter5 could
be legal, since it will not run in hardware.
Driver will check failed when try to offload this filter. So the filter5 will only
run in software.
quoted
WDYT?
I think this one also has ambiguity. If the filter doesnt specify skip_sw or
skip_hw it will run both in s/w and h/w. I am worried if that looks suprising to
someone debugging after because in h/w there is filter 5 but no policer but in
s/w twin we have filter 5 and policer index 20.
In this case, the filter will not in h/w because when the driver tries to offload the filter,
It will found the action is not in h/w and return failed, then the filter will not in h/w, so the filter will only
In software.
So you have partial failure? That doesnt sound good. What do you return
to the user - "success" or "somehow success"?
I worry it is still ambigous. Did the user really intend to do that?
If they did maybe they should have just added it to s/w instead of h/w
and s/w and then get saved by the driver?
quoted
It could be design intent, but in my opinion we have priorities to resolve such
ambiguities in policies.
If we use the rule which says the flags have to match exactly then we can
simplify resolving any ambiguity - which will make it illegal, no?
When you mentioned " match exactly ", do you mean the flags of the filter and the actions should be
exactly same?
Please consider the case that filter has flag and the action does not have any flag.
See above.
I think we should allow this case.
Because it is legal before our patch, we do not expect to break this use case, yes?
So maybe the "match exactly" just limits action flags, when action has flags(skip_sw or skip_hw), the filter must have
exactly the same flags.
Maybe i am missing something but nothing should break.
I think what you mean is when the action is specified with
the filter. The flags should be the same in that case.
Example, filter 1:
tc filter add dev $DEV1 proto ip paren ffff: flower \
ip_proto icmp action police blah
where flag is 0 implies this filter goes both in h/w and s/w.
If i dump the policer or the filter i will see some index provided by
the kernel and i should be able to see both s/w and h/w
counters.
Same thing if i did:
Example filter 2:
tc filter add dev $DEV1 proto ip paren ffff: flower \
skip_sw ip_proto udp action police blah
both filter + action will have where flag of skip_sw when i dump
implies this filter goes only in h/w and any displayed index
is allocated by the kernel.
Our challenge is when someone specifies a specific action by index
and tries to use it ambigously.
cheers,
jamal