On 4/27/22 04:41, Jianbo Liu wrote:
> On Wed, 2022-04-27 at 01:58 +0200, Ilya Maximets wrote:
>> On 4/26/22 09:14, Simon Horman wrote:
>>> On Tue, Apr 19, 2022 at 01:38:33AM +0000, Jianbo Liu via dev wrote:
>>>> On Thu, 2022-04-07 at 09:15 +0000, Jianbo Liu wrote:
>>>>> This series is to add support for tc offloading of ovs
>>>>> metering, and
>>>>> enhance OVS to use new kernel feature which offload tc police
>>>>> action to
>>>>> hardware.
>>>>> To do the offloading, new APIs for meter are added in netdev-
>>>>> offload,
>>>>> and OVS meters are mapped to tc police actions with one-to-one
>>>>> relationship for dpif-netlink.
>>>>>
>>>>> Notes:
>>>>> v3
>>>>> - Add netdev-offload APIs for meter.
>>>>> - Move the implementation to lower netdev-offload-tc layer.
>>>>>
>>>>> v2
>>>>> - Move tc police parse call from last patch to 2nd patch.
>>>>> 2nd patch is adding the parse lib func and add the call
>>>>> to it in
>>>>> that patch.
>>>>> Last patch is the put of tc police act.
>>>>> In 2nd patch also add empty switch case for tc police act
>>>>> put as
>>>>> the impl.
>>>>> is done in the last patch when support for put is being
>>>>> added.
>>>
>>> ...
>>>
>>>> Friendly ping for review.
>>>
>>> Hi Jianbo,
>>>
>>> sorry for the delayed response.
>>>
>>> My colleagues at Corigine have exercised this patchset and have
>>> verified
>>> it for our test-cases.
>>>
>>> Also, I have run the upstream test workflows over them, and they
>>> look good.
>>>
>>> 1. netdev-offload: Add meter offload provider
>>>
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fhorms2%2Fovs%2Factions%2Fruns%2F2220399423&data=05%7C01%7Cjianbol%40nvidia.com%7Cbbf3220c395d4d76290e08da27e0abc6%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637866143161220737%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=WiPBTWZX1XsWskvmW9RPgH6VU3znLRcLq894G8cYGUw%3D&reserved=0
>>> 2. tc: Add support parsing tc police action
>>>
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fhorms2%2Fovs%2Factions%2Fruns%2F2220409233&data=05%7C01%7Cjianbol%40nvidia.com%7Cbbf3220c395d4d76290e08da27e0abc6%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637866143161220737%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=jeMI5%2F9CN%2B2lXFEL%2Bu3L1YkENYoNB9iudLukRZHv1cU%3D&reserved=0
>>> 3. netdev-linux: Refactor put police action netlink message
>>>
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fhorms2%2Fovs%2Factions%2Fruns%2F2220516986&data=05%7C01%7Cjianbol%40nvidia.com%7Cbbf3220c395d4d76290e08da27e0abc6%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637866143161220737%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=vXnK87IOXkdQt%2BDYG90ksy0CWZHfH7lo%2FmdHCXTu6Bc%3D&reserved=0
>>> 4. netdev-linux: Add functions to manipulate tc police action
>>>
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fhorms2%2Fovs%2Factions%2Fruns%2F2220566774&data=05%7C01%7Cjianbol%40nvidia.com%7Cbbf3220c395d4d76290e08da27e0abc6%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637866143161220737%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=iqFPTqegSUQX35jkLzjCDavPWrzTTX3Bg%2FgZEdDCHho%3D&reserved=0
>>> 5. netdev-offload-tc: Implement and register meter offload API for
>>> tc
>>>
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fhorms2%2Fovs%2Factions%2Fruns%2F2220568491&data=05%7C01%7Cjianbol%40nvidia.com%7Cbbf3220c395d4d76290e08da27e0abc6%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637866143161220737%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=sZ5izlE%2FYu9wfy%2B1q7wwzyn5JDE2MeH8OzTKilE7Fqg%3D&reserved=0
>>> 6. netdev-offload-tc: Cleanup police actions with reserved indexes
>>> on startup
>>>
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fhorms2%2Fovs%2Factions%2Fruns%2F2220569761&data=05%7C01%7Cjianbol%40nvidia.com%7Cbbf3220c395d4d76290e08da27e0abc6%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637866143161220737%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=ODy0cbf5MvUFZ%2F61%2F49wMNTzkXhOFIZinhcVQmG78I0%3D&reserved=0
>>> 7. netdev-offload-tc: Offloading rules with police actions
>>>
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fhorms2%2Fovs%2Factions%2Fruns%2F2220571039&data=05%7C01%7Cjianbol%40nvidia.com%7Cbbf3220c395d4d76290e08da27e0abc6%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637866143161220737%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=pvaCfZZD3j0rBL7jw8olMJxRz4%2FNBbE81jkZqgOyCzk%3D&reserved=0
>>> 8. dpif-netlink: Offloading meter to tc police action
>>>
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fhorms2%2Fovs%2Factions%2Fruns%2F2220572904&data=05%7C01%7Cjianbol%40nvidia.com%7Cbbf3220c395d4d76290e08da27e0abc6%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637866143161220737%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=k4r3DT2R%2FzcbrByDcj8LrIEcjPkgxh%2BtXLNPc8XT%2FLA%3D&reserved=0
>>>
>>> I have also reviewed the patches, and with my OVS maintainer hat
>>> on, I'm
>>> happy to apply them, and plan to do so in the coming days unless
>>> there are
>>> objections.
>>
>> Thanks, Simon, for taking care of this patch set! And sorry for me
>> not being
>> very responsive.
>>
>> I had a quick look over the patch set just now and I think that in
>> this
>> version most of my main concerns were addressed and it looks good in
>> general.
>>
>> One thing I didn't understand is why we need to have 'struct
>> meter_offload_api'
>> as a separate entity? I mean, it never used if a normal offload API
>> is
>> not initialized and it's tied with the particular offload API. Can
>> these
>> callbacks be just part of the normal 'struct netdev_flow_api' ?
>> Should not be hard to merge meter_offload_api and netdev_flow_api
>> into one.
>> That should also reduce the amount of code. Or am I missing
>> something?
>
> Conceptually, meter is used globally, and it does not belong to any
> perticular netdev/flow. All the callbacks in 'struct netdev_flow_api'
> have a parameter related to netdev, but meter callbacks don't need.
> The meter_offload_set/get/del can be part of 'struct netdev_flow_api',
> but meter_offload_init/destory must be placed to elsewhere, to avoid
> being called more than once.
> So, why not add new API for meter? But I'd like to change if you
> insist. What's your decision?
I agree that meters do not belong to any particular netdev, that's
true. However, netdev_flow_api is also a global entity. All netdevs
are using the same instance of the netdev_fow_api. And most of the
structures in netdev-offload-tc are global (hash maps, configurations).
'netdev' argument can just be viewed as a guide for these callbacks,
like meter_id for the meter offload API, not something necessary.
(I guess, we could rename netdev_flow_api to just offload_api to make
this more clear, but let's not think about that too much for now to
not overcomplicate things.)
meter_offload_init/destory APIs might not be even needed.
meter_tc_init_policer can be called directly from the netdev_tc_init_flow_api,
if made re-enter-able (if (meter_police_ids) { return; }), or can be
called from under the existing ovsthread_once_start(&once).
meter_offload_destory in current implementation only frees the id pool.
This might be not necessary for the following reasons:
- If datapath is destroyed, but the process is still running, it's
likely that we're waiting for the creation of a new datapath that
will re-use the id-pool.
- If we're going to shut down the OVS, it's nice but not necessary
to free all the memory before doing so. Since the id-pool is a global
variable, that will not be treated as a memory leak by sanitizers.
What do you think?
Forgot to mention in a previous reply that the patch 8/8 of the patch
set should add a NEWS entry for the new feature (the NEWS file is a
bit mangled at the moment, but Alin is going to fix it soon).
Some test in the tests/system-offloads-traffic.at would be also nice
to have, if possible.
>
> Thanks!
> Jianbo
>
>>
>> Another small nit is that, I guess, lib/tc.c is a better home for
>> most of the
>> tc_police functions from lib/netdev-linux.c. But I agree that they
>> are
>> already mixed up and it's kind of a separate task to split them. So,
>> it's
>> not really a concern for this patch set.
>>
>>>
>>> One area that we, at Corigine, would like to follow-up on is using
>>> a
>>> revalidation process rather than evicting rules on start-up.
>>
>> If TC police deletion failures can happen, that can become a problem
>> for
>> long-running setups indeed. Not sure what the best solution should
>> be, but
>> I agree that this case has to be handled in some way.
>>
>> Best regards, Ilya Maximets.
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
