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

Chris Mi cmi at nvidia.com
Fri Oct 8 09:02:41 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.
>
> 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.
>
> 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.
OK, I see. Will change it.

Thanks,
Chris
>
> <SNIP>
>
>>>> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
>>>> index 7e11b9697..ce20cdeb1 100644
>>>> --- a/lib/dpif-provider.h
>>>> +++ b/lib/dpif-provider.h
>>>> @@ -22,8 +22,9 @@
>>>>     * exposed over OpenFlow as a single switch.  Datapaths and the collections of
>>>>     * ports that they contain may be fixed or dynamic. */
>>>>
>>>> -#include "openflow/openflow.h"
>>>>    #include "dpif.h"
>>>> +#include "dpif-offload-provider.h"
>>>> +#include "openflow/openflow.h"
>>>>    #include "util.h"
>>>>
>>>>    #ifdef  __cplusplus
>>>> @@ -635,6 +636,11 @@ struct dpif_class {
>>>>         * sufficient to store BOND_BUCKETS number of elements. */
>>>>        int (*bond_stats_get)(struct dpif *dpif, uint32_t bond_id,
>>>>                              uint64_t *n_bytes);
>>>> +
>>>> +    /* Some offload actions require functionality that is not netdev based,
>>>> +     * but dpif. Add dpif-offload-provider layer API to support such
>>>> +     * offload actions. */
>>>> +    const struct dpif_offload_api *offload_api;
>>> | EC> Here you add the provider directly into the dpif class. Not sure if this is what Ilya had in mind. As in general, these get integrated into the dpif/netdev, not the class. Ilya can you comment on/review this?
>>> | CM> OK.
>>>
>>>
>>> Guess we are still waiting for Ilya’s feedback on this…
>> Ilya,
>>
>> If you have time, could you please take a look if this new design is ok?
>>
>> Thanks,
>> Chris



More information about the dev mailing list