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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
