[ovs-dev] [PATCH v15 3/7] dpif-offload-provider: Introduce dpif-offload-provider layer

Chris Mi cmi at nvidia.com
Tue Oct 12 07:19:47 UTC 2021


On 10/8/2021 4:34 PM, Eelco Chaudron wrote:
>
> On 8 Oct 2021, at 10:06, Chris Mi wrote:
>
>> On 10/1/2021 5:52 PM, Eelco Chaudron wrote:
>>> See some small comments inline.
>>>
>>> On 15 Sep 2021, at 14:43, Chris Mi wrote:
> <SNIP>
>
>>>>>>>> --- /dev/null
>>>> +++ b/lib/dpif-offload-provider.h
>>>> @@ -0,0 +1,47 @@
>>>> +/*
>>>> + * Copyright (c) 2021, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
>>>> + *
>>>> + * Licensed under the Apache License, Version 2.0 (the "License");
>>>> + * you may not use this file except in compliance with the License.
>>>> + * You may obtain a copy of the License at:
>>>> + *
>>>> + *     http://www.apache.org/licenses/LICENSE-2.0
>>>> + *
>>>> + * Unless required by applicable law or agreed to in writing, software
>>>> + * distributed under the License is distributed on an "AS IS" BASIS,
>>>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
>>>> + * See the License for the specific language governing permissions and
>>>> + * limitations under the License.
>>>> + */
>>>> +
>>>> +#ifndef DPIF_OFFLOAD_PROVIDER_H
>>>> +#define DPIF_OFFLOAD_PROVIDER_H
>>>> +
>>>> +struct dpif;
>>>> +struct dpif_offload_sflow;
>>>> +
>>>> +/* Datapath interface offload structure, to be defined by each implementation
>>>> + * of a datapath interface.
>>>> + */
>>>> +struct dpif_offload_api {
>>>> +    /* Called when the dpif provider is registered and right after dpif
>>>> +     * provider init function. */
>>>> +    void (*init)(void);
>>> | EC> Do we want to think ahead and allow init to fail? Other init callbacks return int.
>>> | CM> We don't want to fail the whole offload if dpif offload fails to init. So we don't check the return value.
>>>
>>> Why? In theory, if dpif offload fails, all offload would fill as we could miss vital functions? I think we should add it and check for it (even in the current partial implementation can’t fail).
>> If kernel disables CONFIG_PSAMPLE, dpif offload init will fail. If return error for it, that means we can't use any offload.
>> I think that's a regression. Customer will not be happy with it if they upgrade OVS.
> Guess I was not clear enough in my previous comment.
>
> I wanted to just have the API change, so if in the future it needs to fail it could.
> So just define the init function as “int (*init)(void);”, but you just return ok always for now.
Done.
>
> On the other hand, in your case, the dpif_offload_netlink_init() function’s nl_lookup_genl_family() failure should probably change to a VLOG_WARN, not an INFO.
Done.
>
> And third, if dpif_offload_netlink_init() is failing due to the fact it can not initialize PSAMPLE, offload of it should also not be done in netdev_tc_flow_put() and should return an EOPNOTSUPP.
Done.

I introduce a new member in struct offload_info and a new api to pass 
this information from dpif-offload-netlink to netdev-offload-tc layer.
>
> <SNIP>


More information about the dev mailing list