[ovs-dev] [PATCH V9 07/31] other-config: Add tc-policy switch to control tc flower flag

Roi Dayan roid at mellanox.com
Mon Jun 5 05:17:10 UTC 2017



On 30/05/2017 11:10, Simon Horman wrote:
> On Sun, May 28, 2017 at 02:59:49PM +0300, Roi Dayan wrote:
>> From: Paul Blakey <paulb at mellanox.com>
>>
>> Add a new configuration tc-policy option that controls tc
>> flower flag. Possible options are none, skip_sw, skip_hw.
>> The default is none which is to insert the rule both to sw and hw.
>> This option is only relevant if hw-offload is enabled.
>>
>> Signed-off-by: Paul Blakey <paulb at mellanox.com>
>> Reviewed-by: Roi Dayan <roid at mellanox.com>
>> Reviewed-by: Simon Horman <simon.horman at netronome.com>
>
> Thanks, the comment below not withstanding this looks good to me
> and I would be happy to apply it if someone provided a review.
>
>> ---
>>  lib/netdev.c         |  6 ++++++
>>  lib/tc.c             | 43 ++++++++++++++++++++++++++++++++++++++++++-
>>  lib/tc.h             |  3 +++
>>  vswitchd/vswitch.xml | 17 +++++++++++++++++
>>  4 files changed, 68 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/netdev.c b/lib/netdev.c
>> index 2807a9d..19d809b 100644
>> --- a/lib/netdev.c
>> +++ b/lib/netdev.c
>> @@ -54,6 +54,9 @@
>>  #include "openvswitch/vlog.h"
>>  #include "flow.h"
>>  #include "util.h"
>> +#ifdef __linux__
>> +#include "tc.h"
>> +#endif
>>
>>  VLOG_DEFINE_THIS_MODULE(netdev);
>>
>> @@ -2117,6 +2120,9 @@ netdev_set_flow_api_enabled(const struct smap *ovs_other_config)
>>
>>              VLOG_INFO("netdev: Flow API Enabled");
>>
>> +            tc_set_policy(smap_get_def(ovs_other_config, "tc-policy",
>> +                                       TC_POLICY_DEFAULT));
>> +
>>              ovsthread_once_done(&once);
>>          }
>>      }
>
> In v7 Flavio Leitner made the following comment regarding this.
>
>   "Well, this enforces that the user sets tc-policy before enabling HW
>    offloading, otherwise it won't get set, right?"
>
> My comment, along the same lines, is that it looks like it would be better
> if tc_set_policy() was called independent of netdev_set_flow_api_enabled()
> to allow them to be configured independently.
>
> I realise that at this point the patch-set has some limitations regarding
> restarting ovs when offloading of flows is enabled and disabled. And
> probably the code above plays well with that restriction in place. But I
> think it would be good to look into ways to lift that restriction and thus
> it would seems worthwhile revisiting where tc_set_policy() is called.
>

Hi,

Changing the policy during run time without flushing the rules can
cause unexpected behavior, like old rules are offloaded to hw but
newly added rules will be software. so the status wont match the policy.

I mentioned this before, I already have a patch to make hw-offload and
tc-policy changeable at runtime and will flush the rules.
I hit strange issue using it which I reproduced without the patch where
dump-flows doesn't show rules anymore.
I'm holding that patch from this series as I want to check if the
issue still reproduce in latest kernel+latest OVS and report it
before this patch will be applied. didn't have time to do it yet.

Thanks,
Roi


More information about the dev mailing list