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

Chris Mi cmi at nvidia.com
Fri Oct 8 08:06:41 UTC 2021


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:
>
>> Some offload actions require functionality that is not netdev
>> based, but dpif. For example, sFlow action requires to create
>> a psample netlink socket to receive the sampled packets from
>> TC or kernel driver.
>>
>> Create dpif-offload-provider layer to support such actions.
>>
>> Signed-off-by: Chris Mi <cmi at nvidia.com>
>> Reviewed-by: Eli Britstein <elibr at nvidia.com>
>> ---
>>   lib/automake.mk             |  2 ++
>>   lib/dpif-netdev.c           |  1 +
>>   lib/dpif-netlink.c          |  2 ++
>>   lib/dpif-offload-provider.h | 47 +++++++++++++++++++++++++++++++++++++
>>   lib/dpif-offload.c          | 43 +++++++++++++++++++++++++++++++++
>>   lib/dpif-provider.h         |  8 ++++++-
>>   lib/dpif.c                  | 10 ++++++++
>>   7 files changed, 112 insertions(+), 1 deletion(-)
>>   create mode 100644 lib/dpif-offload-provider.h
>>   create mode 100644 lib/dpif-offload.c
>>
>> diff --git a/lib/automake.mk b/lib/automake.mk
>> index 46f869a33..9259f57de 100644
>> --- a/lib/automake.mk
>> +++ b/lib/automake.mk
>> @@ -127,6 +127,8 @@ lib_libopenvswitch_la_SOURCES = \
>>   	lib/dpif-netdev-private.h \
>>   	lib/dpif-netdev-perf.c \
>>   	lib/dpif-netdev-perf.h \
>> +	lib/dpif-offload.c \
>> +	lib/dpif-offload-provider.h \
>>   	lib/dpif-provider.h \
>>   	lib/dpif.c \
>>   	lib/dpif.h \
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index 4cd0e9ebd..1645b44b0 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -8801,6 +8801,7 @@ const struct dpif_class dpif_netdev_class = {
>>       dpif_netdev_bond_add,
>>       dpif_netdev_bond_del,
>>       dpif_netdev_bond_stats_get,
>> +    NULL,                       /* dpif_offload_api */
>>   };
>>
>>   static void
>> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
>> index 34fc04237..f546b48e5 100644
>> --- a/lib/dpif-netlink.c
>> +++ b/lib/dpif-netlink.c
>> @@ -34,6 +34,7 @@
>>
>>   #include "bitmap.h"
>>   #include "dpif-netlink-rtnl.h"
>> +#include "dpif-offload-provider.h"
>>   #include "dpif-provider.h"
>>   #include "fat-rwlock.h"
>>   #include "flow.h"
>> @@ -4340,6 +4341,7 @@ const struct dpif_class dpif_netlink_class = {
>>       NULL,                       /* bond_add */
>>       NULL,                       /* bond_del */
>>       NULL,                       /* bond_stats_get */
>> +    NULL,                       /* dpif_offload_api */
>>   };
>>
>>   static int
>> diff --git a/lib/dpif-offload-provider.h b/lib/dpif-offload-provider.h
>> new file mode 100644
>> index 000000000..75238c4cb
>> --- /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.
>
>> +
>> +    /* Free all dpif offload resources. */
>> +    void (*destroy)(void);
>> +
>> +    /* Arranges for the poll loop for an upcall handler to wake up when psample
>> +     * has a message queued to be received. */
>> +    void (*sflow_recv_wait)(void);
>> +
>> +    /* Polls for an upcall from psample for an upcall handler.
>> +     * Return 0 for success. */
>> +    int (*sflow_recv)(struct dpif_offload_sflow *sflow);
>> +};
>> +
>> +void dpif_offload_sflow_recv_wait(const struct dpif *dpif);
>> +int dpif_offload_sflow_recv(const struct dpif *dpif,
>> +                            struct dpif_offload_sflow *sflow);
>> +
>> +#endif /* DPIF_OFFLOAD_PROVIDER_H */
>> diff --git a/lib/dpif-offload.c b/lib/dpif-offload.c
>> new file mode 100644
>> index 000000000..842e05798
>> --- /dev/null
>> +++ b/lib/dpif-offload.c
>> @@ -0,0 +1,43 @@
>> +/*
>> + * 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.
>> + */
>> +
>> +#include <config.h>
>> +#include <errno.h>
>> +
>> +#include "dpif-provider.h"
>> +
>> +void
>> +dpif_offload_sflow_recv_wait(const struct dpif *dpif)
>> +{
>> +    const struct dpif_offload_api *offload_api = dpif->dpif_class->offload_api;
>> +
>> +    if (offload_api && offload_api->sflow_recv_wait) {
>> +        offload_api->sflow_recv_wait();
>> +    }
>> +}
>> +
>> +int
>> +dpif_offload_sflow_recv(const struct dpif *dpif,
>> +                        struct dpif_offload_sflow *sflow)
>> +{
>> +    const struct dpif_offload_api *offload_api = dpif->dpif_class->offload_api;
>> +
>> +    if (offload_api && offload_api->sflow_recv) {
>> +        return offload_api->sflow_recv(sflow);
>> +    }
>> +
>> +    return EOPNOTSUPP;
>> +}
>> 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
>
>>   };
>>
>>   extern const struct dpif_class dpif_netlink_class;
>> diff --git a/lib/dpif.c b/lib/dpif.c
>> index 8c4aed47b..13a3cd0d4 100644
>> --- a/lib/dpif.c
>> +++ b/lib/dpif.c
>> @@ -153,6 +153,10 @@ dp_register_provider__(const struct dpif_class *new_class)
>>           return error;
>>       }
>>
>> +    if (new_class->offload_api && new_class->offload_api->init) {
>> +        new_class->offload_api->init();
>> +    }
>> +
>>       registered_class = xmalloc(sizeof *registered_class);
>>       registered_class->dpif_class = new_class;
>>       registered_class->refcount = 0;
>> @@ -183,6 +187,7 @@ static int
>>   dp_unregister_provider__(const char *type)
>>   {
>>       struct shash_node *node;
>> +    const struct dpif_class *dpif_class;
>>       struct registered_dpif_class *registered_class;
>>
>>       node = shash_find(&dpif_classes, type);
>> @@ -196,6 +201,11 @@ dp_unregister_provider__(const char *type)
>>           return EBUSY;
>>       }
>>
>> +    dpif_class = registered_class->dpif_class;
>> +    if (dpif_class->offload_api && dpif_class->offload_api->destroy) {
>> +        dpif_class->offload_api->destroy();
>> +    }
>> +
>>       shash_delete(&dpif_classes, node);
>>       free(registered_class);
>>
>> -- 
>> 2.27.0



More information about the dev mailing list