| Message ID | 20211115025312.786953-4-cmi@nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add offload support for sFlow | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
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 <cmi@nvidia.com> > Reviewed-by: Eli Britstein <elibr@nvidia.com> > --- > 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. The rest of the patch looks good. > + > + /* Called when the dpif offload provider is registered. */ > + int (*init)(void); > + > + /* 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..f2bf3e634 > --- /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_class *offload_class = dpif->offload_class; > + > + if (offload_class && offload_class->sflow_recv_wait) { > + offload_class->sflow_recv_wait(); > + } > +} > + > +int > +dpif_offload_sflow_recv(const struct dpif *dpif, > + struct dpif_offload_sflow *sflow) > +{ > + const struct dpif_offload_class *offload_class = dpif->offload_class; > + > + if (offload_class && offload_class->sflow_recv) { > + return offload_class->sflow_recv(sflow); > + } > + > + return EOPNOTSUPP; > +} > diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h > index 27e3a7658..37b3d3618 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 > @@ -35,6 +36,7 @@ extern "C" { > * This structure should be treated as opaque by dpif implementations. */ > struct dpif { > const struct dpif_class *dpif_class; > + const struct dpif_offload_class *offload_class; > char *base_name; > char *full_name; > uint8_t netflow_engine_type; > -- > 2.30.2
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 <cmi@nvidia.com> >> Reviewed-by: Eli Britstein <elibr@nvidia.com> >> --- >> 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? 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); ... }; 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. > The rest of the patch looks good. > >> + >> + /* Called when the dpif offload provider is registered. */ >> + int (*init)(void); >> + >> + /* 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..f2bf3e634 >> --- /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_class *offload_class = dpif->offload_class; >> + >> + if (offload_class && offload_class->sflow_recv_wait) { >> + offload_class->sflow_recv_wait(); >> + } >> +} >> + >> +int >> +dpif_offload_sflow_recv(const struct dpif *dpif, >> + struct dpif_offload_sflow *sflow) >> +{ >> + const struct dpif_offload_class *offload_class = dpif->offload_class; >> + >> + if (offload_class && offload_class->sflow_recv) { >> + return offload_class->sflow_recv(sflow); >> + } >> + >> + return EOPNOTSUPP; >> +} >> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h >> index 27e3a7658..37b3d3618 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 >> @@ -35,6 +36,7 @@ extern "C" { >> * This structure should be treated as opaque by dpif implementations. */ >> struct dpif { >> const struct dpif_class *dpif_class; >> + const struct dpif_offload_class *offload_class; >> char *base_name; >> char *full_name; >> uint8_t netflow_engine_type; >> -- >> 2.30.2
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 <cmi@nvidia.com> >>> Reviewed-by: Eli Britstein <elibr@nvidia.com> >>> --- >>> 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. >> The rest of the patch looks good. >> >>> + >>> + /* Called when the dpif offload provider is registered. */ >>> + int (*init)(void); >>> + >>> + /* 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..f2bf3e634 >>> --- /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_class *offload_class = dpif->offload_class; >>> + >>> + if (offload_class && offload_class->sflow_recv_wait) { >>> + offload_class->sflow_recv_wait(); >>> + } >>> +} >>> + >>> +int >>> +dpif_offload_sflow_recv(const struct dpif *dpif, >>> + struct dpif_offload_sflow *sflow) >>> +{ >>> + const struct dpif_offload_class *offload_class = dpif->offload_class; >>> + >>> + if (offload_class && offload_class->sflow_recv) { >>> + return offload_class->sflow_recv(sflow); >>> + } >>> + >>> + return EOPNOTSUPP; >>> +} >>> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h >>> index 27e3a7658..37b3d3618 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 >>> @@ -35,6 +36,7 @@ extern "C" { >>> * This structure should be treated as opaque by dpif implementations. */ >>> struct dpif { >>> const struct dpif_class *dpif_class; >>> + const struct dpif_offload_class *offload_class; >>> char *base_name; >>> char *full_name; >>> uint8_t netflow_engine_type; >>> -- >>> 2.30.2
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 <cmi@nvidia.com> >>>> Reviewed-by: Eli Britstein <elibr@nvidia.com> >>>> --- >>>> 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
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; + + /* Called when the dpif offload provider is registered. */ + int (*init)(void); + + /* 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..f2bf3e634 --- /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_class *offload_class = dpif->offload_class; + + if (offload_class && offload_class->sflow_recv_wait) { + offload_class->sflow_recv_wait(); + } +} + +int +dpif_offload_sflow_recv(const struct dpif *dpif, + struct dpif_offload_sflow *sflow) +{ + const struct dpif_offload_class *offload_class = dpif->offload_class; + + if (offload_class && offload_class->sflow_recv) { + return offload_class->sflow_recv(sflow); + } + + return EOPNOTSUPP; +} diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h index 27e3a7658..37b3d3618 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 @@ -35,6 +36,7 @@ extern "C" { * This structure should be treated as opaque by dpif implementations. */ struct dpif { const struct dpif_class *dpif_class; + const struct dpif_offload_class *offload_class; char *base_name; char *full_name; uint8_t netflow_engine_type;