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

Eelco Chaudron echaudro at redhat.com
Fri Oct 8 08:34:41 UTC 2021



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.

<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