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