Hi Ilya,
On 11/30/2021 3:58 PM, Eelco Chaudron wrote:
On 29 Nov 2021, at 8:38, Chris Mi wrote:
On 11/25/2021 4:52 PM, Eelco Chaudron wrote:
On 15 Nov 2021, at 3:53, 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 <[email protected]>
Reviewed-by: Eli Britstein <[email protected]>
---
lib/automake.mk | 2 ++
lib/dpif-offload-provider.h | 52 +++++++++++++++++++++++++++++++++++++
lib/dpif-offload.c | 43 ++++++++++++++++++++++++++++++
lib/dpif-provider.h | 4 ++-
4 files changed, 100 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-offload-provider.h b/lib/dpif-offload-provider.h
new file mode 100644
index 000000000..3b94740df
--- /dev/null
+++ b/lib/dpif-offload-provider.h
@@ -0,0 +1,52 @@
+/*
+ * 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_class {
+ /* Type of dpif offload in this class, e.g. "system", "netdev", etc.
+ *
+ * One of the providers should supply a "system" type, since this is
+ * the type assumed if no type is specified when opening a dpif. */
+ const char *type;
Why is the offload class type directly related to the dpif class type?
Should it not be independent? I think the type should be the offload type so in
this case TC. If in the future a new offload type is introduced in the kernel,
for example, eBPF. We could add a new type eBPF and decide which one the user
will use. Guess this also effect patch 5, so will add some more comment there
also.
But if a certain dpif type supports both "TC" and "eBPF", will the dpif have
two dpif_offload_classes?
Guess the datapath can choose which offload to use, in theory, it could have
both, and on a port level, it can be decided which one is used for that port.
If eBPF is supported in the future, I think we just need to add new API like
this:
struct dpif_offload_class {
const char *type,
int (*init)(void);
void (*destroy)(void);
void (*sflow_recv_wait)(void);
void (*sflow_recv)(void);
void (*process_ebpf)(void);
...
};
It’s true you need this, but currently, you have a one-to-one mapping in the
code (see comments). So your offload classes should not be named netdev,
netlink, etc.
Even if two dpif types need to support a same feature, like sFlow offload, I
don't think the function
could be shared for all dpif types. If this is true, maybe it is simple and
good to have a 1:1 mapping.
Since the change is not small, so I think it is good to make clear of the
design before changing the code.
If we do a 1:1 mapping, we could as well remove the whole offload class, as it
could simply be integrated into the dpif one. I've copied Ilya, as it was his
idea to add the offload class, and probably has a more clear vision on how he
sees this as part of the framework.
Could you please elaborate your idea of the offload class? Although
Eelco replied my questions,
I'm still not clear about it. In my understanding, struct
dpif_offload_class is similar to struct dpif_class.
If other features need to leverage on it, they just add new callback
inside of struct dpif_offload_class.
If we introduce different classes for different features, I'm afraid
only the init and destroy callbacks
can be shared.
Thanks,
Chris
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
