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.
Done.
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.
Done.
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.
Done.
I introduce a new member in struct offload_info and a new api to pass
this information from dpif-offload-netlink to netdev-offload-tc layer.
<SNIP>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
