| Message ID | 20251130154922.785311-1-elibr@nvidia.com |
|---|---|
| Headers | show |
| Series | Generic libraries | expand |
Hi Netdev-doca requires a way to initialize doca-flow library, like dpdk library init. This series generalizes this mechanism as a pre-step towards it. Could you please review it? Thanks, Eli >-----Original Message----- >From: dev <ovs-dev-bounces@openvswitch.org> On Behalf Of Eli Britstein >Sent: Sunday, 30 November 2025 17:49 >To: dev@openvswitch.org >Cc: Eli Britstein <elibr@nvidia.com>; Ilya Maximets <i.maximets@ovn.org>; >David Marchand <david.marchand@redhat.com>; Maor Dickman ><maord@nvidia.com> >Subject: [ovs-dev] [PATCH v1 0/4] Generic libraries > >Currently dpdk is a special case and has dedicated calls from the bridge module >and entries in OVS schema. > >The legacy mode, (dpdk-init and other configurations in other_config) is kept for >backward compatability. > >A new generalized method is introduced, for example (configuration options are >optional): >ovs-vsctl add-library dpdk dpdk-lcore-mask=0x5 > >Eli Britstein (4): > vswitch.ovsschema: Add libraries to Open_vSwitch. > ovs-vsctl: Add add/del-library commands. > lib: Introduce library module. > dpdk: Implement library_class_dpdk. > > lib/automake.mk | 3 + > lib/dpdk.c | 26 ++++++++ > lib/dpdk.h | 2 + > lib/library-provider.h | 25 ++++++++ > lib/library.c | 128 +++++++++++++++++++++++++++++++++++++ > lib/library.h | 18 ++++++ > utilities/ovs-vsctl.c | 89 ++++++++++++++++++++++++++ > vswitchd/bridge.c | 20 ++++++ > vswitchd/ovs-vswitchd.c | 2 + > vswitchd/vswitch.ovsschema | 27 +++++++- > vswitchd/vswitch.xml | 19 ++++++ > 11 files changed, 356 insertions(+), 3 deletions(-) create mode 100644 >lib/library-provider.h create mode 100644 lib/library.c create mode 100644 >lib/library.h
Eli Britstein via dev <ovs-dev@openvswitch.org> writes: > Currently dpdk is a special case and has dedicated calls from the bridge > module and entries in OVS schema. > > The legacy mode, (dpdk-init and other configurations in other_config) is > kept for backward compatability. > > A new generalized method is introduced, for example (configuration > options are optional): > ovs-vsctl add-library dpdk dpdk-lcore-mask=0x5 Hi Eli, I took a quick look over this series. I think the naming here is a bit too generic. For example, when we're setting up DPDK library configuration, we're not 'adding' DPDK library - it is already compiled against the DPDK library. It may be confusing for a user who doesn't know how the distribution compiled their version of OVS and think they can simply type 'add dpdk' and it will be linked. However, there's not a dlopen/rtld type of linking going on so the user could have a mismatched expectation. I do think having a more cohesive configuration and status framework for these kinds of linked libraries could make sense. Perhaps it would be better to just call them what they are: configuration and status. ie: 'ovs-vsctl add-library ...' would become 'ovs-vsctl <library>-set-config ...' 'ovs-vsctl <library>-status' Then things like dpdk would register in a way that we could get commands generated for them for dpdk specific configs. Additionally, I think we need more than just config() and init() calls in the framework. For example, we may want to pre-define the configuration options allowed - that's a double edged sword, of course (since we then need to have some kind of maintenance on config options which is difficult). We don't have an uninit call in the framework, which might make sense. We also have other strange things here. For example, did I understand right in 4/4 that after this patch DPDK now can take either other_config:dpdk-... OR the new library config? Which takes precedence? what happens when both are encountered? Practically, this is a new way of doing things for existing users so that patch needs much more documentation and thoughts about config. I think we will need some understanding of how this is intended to be used going forward as well. For example, DPDK library is really providing two things: new port types, and a different packet buffer implementation. Are all linked in libraries going to provide that? Or should we have different kinds of specifiers for the types of libraries we will provide these kinds of configurations for? At least, we should discuss what this kind of user interface and the software interface contracts should be. > Eli Britstein (4): > vswitch.ovsschema: Add libraries to Open_vSwitch. > ovs-vsctl: Add add/del-library commands. > lib: Introduce library module. > dpdk: Implement library_class_dpdk. > > lib/automake.mk | 3 + > lib/dpdk.c | 26 ++++++++ > lib/dpdk.h | 2 + > lib/library-provider.h | 25 ++++++++ > lib/library.c | 128 +++++++++++++++++++++++++++++++++++++ > lib/library.h | 18 ++++++ > utilities/ovs-vsctl.c | 89 ++++++++++++++++++++++++++ > vswitchd/bridge.c | 20 ++++++ > vswitchd/ovs-vswitchd.c | 2 + > vswitchd/vswitch.ovsschema | 27 +++++++- > vswitchd/vswitch.xml | 19 ++++++ > 11 files changed, 356 insertions(+), 3 deletions(-) > create mode 100644 lib/library-provider.h > create mode 100644 lib/library.c > create mode 100644 lib/library.h
On 19/01/2026 19:31, Aaron Conole wrote: > External email: Use caution opening links or attachments > > > Eli Britstein via dev <ovs-dev@openvswitch.org> writes: > >> Currently dpdk is a special case and has dedicated calls from the bridge >> module and entries in OVS schema. >> >> The legacy mode, (dpdk-init and other configurations in other_config) is >> kept for backward compatability. >> >> A new generalized method is introduced, for example (configuration >> options are optional): >> ovs-vsctl add-library dpdk dpdk-lcore-mask=0x5 > Hi Eli, > > I took a quick look over this series. Hi Aaron, thanks for the review. To give more context, the "library" module was suggested by Ilya as a comment for the previous series. Please see this: https://mail.openvswitch.org/pipermail/ovs-dev/2025-October/427157.html > > I think the naming here is a bit too generic. For example, when we're > setting up DPDK library configuration, we're not 'adding' DPDK library - > it is already compiled against the DPDK library. It may be confusing > for a user who doesn't know how the distribution compiled their version > of OVS and think they can simply type 'add dpdk' and it will be linked. > However, there's not a dlopen/rtld type of linking going on so the user > could have a mismatched expectation. > > I do think having a more cohesive configuration and status framework for > these kinds of linked libraries could make sense. Perhaps it would be > better to just call them what they are: configuration and status. > > ie: 'ovs-vsctl add-library ...' would become > 'ovs-vsctl <library>-set-config ...' > > 'ovs-vsctl <library>-status' Do you mean each library we will have to change ovs-vsctl.c? Doesn't seem right to me. > > Then things like dpdk would register in a way that we could get commands > generated for them for dpdk specific configs. > > Additionally, I think we need more than just config() and init() calls > in the framework. For example, we may want to pre-define the > configuration options allowed - that's a double edged sword, of course > (since we then need to have some kind of maintenance on config options > which is difficult). We don't have an uninit call in the framework, > which might make sense. I only added what was needed for migration dpdk to this generic framework, without changing its functionality. Currently, there is no uninit (e.g. rte_eal_cleanup) call, so I haven't added it. In general I agree it's better, but I don't want to do this in the scope of this series. > > We also have other strange things here. For example, did I understand > right in 4/4 that after this patch DPDK now can take either > other_config:dpdk-... OR the new library config? Which takes > precedence? what happens when both are encountered? Practically, this > is a new way of doing things for existing users so that patch needs much > more documentation and thoughts about config. other_config:dpdk-init=true is a legacy method, that as suggested at the same context of the previous series must be preserved for backward compatibility. For DPDK, the new method of add-library is an alternative way. For new libraries (e.g. doca) it will be the only way. For precedence, there is no real meaning to this. The user can only "enable" it, but cannot "disable" it once enabled. Either way is OK to enable. For documentation, of course it can be enhanced. Would you please suggest a draft for it? > > I think we will need some understanding of how this is intended to be > used going forward as well. For example, DPDK library is really > providing two things: new port types, and a different packet buffer > implementation. Are all linked in libraries going to provide that? Or > should we have different kinds of specifiers for the types of libraries > we will provide these kinds of configurations for? The only "other" library besides dpdk that I can know of now is doca. That would also provide new port types. In the current implementation there is no new packet buffer implementation, but reuse of the DPDK's one. In the future we might implement it, but hard to say now. I didn't understand the last part of "different kind of specifiers". In any case, I don't think either of those are directly related to this series. WDYT? > > At least, we should discuss what this kind of user interface and the > software interface contracts should be. > >> Eli Britstein (4): >> vswitch.ovsschema: Add libraries to Open_vSwitch. >> ovs-vsctl: Add add/del-library commands. >> lib: Introduce library module. >> dpdk: Implement library_class_dpdk. >> >> lib/automake.mk | 3 + >> lib/dpdk.c | 26 ++++++++ >> lib/dpdk.h | 2 + >> lib/library-provider.h | 25 ++++++++ >> lib/library.c | 128 +++++++++++++++++++++++++++++++++++++ >> lib/library.h | 18 ++++++ >> utilities/ovs-vsctl.c | 89 ++++++++++++++++++++++++++ >> vswitchd/bridge.c | 20 ++++++ >> vswitchd/ovs-vswitchd.c | 2 + >> vswitchd/vswitch.ovsschema | 27 +++++++- >> vswitchd/vswitch.xml | 19 ++++++ >> 11 files changed, 356 insertions(+), 3 deletions(-) >> create mode 100644 lib/library-provider.h >> create mode 100644 lib/library.c >> create mode 100644 lib/library.h