| Message ID | 289c99b5e10631ab6c28c9505667706484888f56.1612968146.git.grive@u256.net |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | dpif-netdev: Parallel offload processing | expand |
Hi Gaetan, On 2/10/21 4:33 PM, Gaetan Rivet wrote: > Expose functions reporting user configuration of offloading threads, as > well as utility functions for multithreading. > > This will only expose the configuration knob to the user, while no > datapath will implement the multiple thread request. > > This will allow implementations to use this API for offload thread > management in relevant layers before enabling the actual dataplane > implementation. > > The offload thread ID is lazily allocated and can as such be in a > different order than the offload thread start sequence. > > The RCU thread will sometime access hardware-offload objects from > a provider for reclamation purposes. In such case, it will get > a default offload thread ID of 0. Care must be taken that using > this thread ID is safe concurrently with the offload threads. > > Signed-off-by: Gaetan Rivet <grive@u256.net> > Reviewed-by: Eli Britstein <elibr@nvidia.com> > --- > lib/netdev-offload-provider.h | 1 + > lib/netdev-offload.c | 88 ++++++++++++++++++++++++++++++++++- > lib/netdev-offload.h | 19 ++++++++ > vswitchd/vswitch.xml | 16 +++++++ > 4 files changed, 122 insertions(+), 2 deletions(-) > > diff --git a/lib/netdev-offload-provider.h b/lib/netdev-offload-provider.h > index 4815a1bd2..f8b66961d 100644 > --- a/lib/netdev-offload-provider.h > +++ b/lib/netdev-offload-provider.h > @@ -84,6 +84,7 @@ struct netdev_flow_api { > struct dpif_flow_stats *); > > /* Get the number of flows offloaded to netdev. > + * 'n_flows' is an array of counters, one per offload thread. > * Return 0 if successful, otherwise returns a positive errno value. */ > int (*flow_get_n_flows)(struct netdev *, uint64_t *n_flows); > > diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c > index 3092f8d55..c30377b91 100644 > --- a/lib/netdev-offload.c > +++ b/lib/netdev-offload.c > @@ -60,6 +60,12 @@ VLOG_DEFINE_THIS_MODULE(netdev_offload); > > static bool netdev_flow_api_enabled = false; > > +#define DEFAULT_OFFLOAD_THREAD_NB 1 > +#define MAX_OFFLOAD_THREAD_NB 10 Out of curiosity, is there a rationale in setting the maximum number of threads to 10? Maxime
On Mon, Mar 8, 2021, at 16:04, Maxime Coquelin wrote: > Hi Gaetan, > > On 2/10/21 4:33 PM, Gaetan Rivet wrote: > > Expose functions reporting user configuration of offloading threads, as > > well as utility functions for multithreading. > > > > This will only expose the configuration knob to the user, while no > > datapath will implement the multiple thread request. > > > > This will allow implementations to use this API for offload thread > > management in relevant layers before enabling the actual dataplane > > implementation. > > > > The offload thread ID is lazily allocated and can as such be in a > > different order than the offload thread start sequence. > > > > The RCU thread will sometime access hardware-offload objects from > > a provider for reclamation purposes. In such case, it will get > > a default offload thread ID of 0. Care must be taken that using > > this thread ID is safe concurrently with the offload threads. > > > > Signed-off-by: Gaetan Rivet <grive@u256.net> > > Reviewed-by: Eli Britstein <elibr@nvidia.com> > > --- > > lib/netdev-offload-provider.h | 1 + > > lib/netdev-offload.c | 88 ++++++++++++++++++++++++++++++++++- > > lib/netdev-offload.h | 19 ++++++++ > > vswitchd/vswitch.xml | 16 +++++++ > > 4 files changed, 122 insertions(+), 2 deletions(-) > > > > diff --git a/lib/netdev-offload-provider.h b/lib/netdev-offload-provider.h > > index 4815a1bd2..f8b66961d 100644 > > --- a/lib/netdev-offload-provider.h > > +++ b/lib/netdev-offload-provider.h > > @@ -84,6 +84,7 @@ struct netdev_flow_api { > > struct dpif_flow_stats *); > > > > /* Get the number of flows offloaded to netdev. > > + * 'n_flows' is an array of counters, one per offload thread. > > * Return 0 if successful, otherwise returns a positive errno value. */ > > int (*flow_get_n_flows)(struct netdev *, uint64_t *n_flows); > > > > diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c > > index 3092f8d55..c30377b91 100644 > > --- a/lib/netdev-offload.c > > +++ b/lib/netdev-offload.c > > @@ -60,6 +60,12 @@ VLOG_DEFINE_THIS_MODULE(netdev_offload); > > > > static bool netdev_flow_api_enabled = false; > > > > +#define DEFAULT_OFFLOAD_THREAD_NB 1 > > +#define MAX_OFFLOAD_THREAD_NB 10 > > Out of curiosity, is there a rationale in setting the maximum number of > threads to 10? > > Maxime Hi Maxime, The limit is mostly arbitrary. I didn't want to allow setting outlandish values, some memory allocations depends on it. In our driver, scaling is effective up to 4 threads at most, and one core use-case of OVS for us, Bluefield, has 8 cores. 10 threads seemed a reasonable limit. I think it's good to have a margin, to test with higher-than-effective numbers in development, while in production we expect on our end to run 4 threads in general. Maybe the limit could be removed, similar to n-handle-threads and n-revalidator-threads. On that point: those two will adapt to each other and depend on the number of CPU cores. Maybe we might also want to allow some budget for offload thread(s), and possibly ct_clean as well.
Hi, On 3/8/21 6:21 PM, Gaëtan Rivet wrote: > On Mon, Mar 8, 2021, at 16:04, Maxime Coquelin wrote: >> Hi Gaetan, >> >> On 2/10/21 4:33 PM, Gaetan Rivet wrote: >>> Expose functions reporting user configuration of offloading threads, as >>> well as utility functions for multithreading. >>> >>> This will only expose the configuration knob to the user, while no >>> datapath will implement the multiple thread request. >>> >>> This will allow implementations to use this API for offload thread >>> management in relevant layers before enabling the actual dataplane >>> implementation. >>> >>> The offload thread ID is lazily allocated and can as such be in a >>> different order than the offload thread start sequence. >>> >>> The RCU thread will sometime access hardware-offload objects from >>> a provider for reclamation purposes. In such case, it will get >>> a default offload thread ID of 0. Care must be taken that using >>> this thread ID is safe concurrently with the offload threads. >>> >>> Signed-off-by: Gaetan Rivet <grive@u256.net> >>> Reviewed-by: Eli Britstein <elibr@nvidia.com> >>> --- >>> lib/netdev-offload-provider.h | 1 + >>> lib/netdev-offload.c | 88 ++++++++++++++++++++++++++++++++++- >>> lib/netdev-offload.h | 19 ++++++++ >>> vswitchd/vswitch.xml | 16 +++++++ >>> 4 files changed, 122 insertions(+), 2 deletions(-) >>> >>> diff --git a/lib/netdev-offload-provider.h b/lib/netdev-offload-provider.h >>> index 4815a1bd2..f8b66961d 100644 >>> --- a/lib/netdev-offload-provider.h >>> +++ b/lib/netdev-offload-provider.h >>> @@ -84,6 +84,7 @@ struct netdev_flow_api { >>> struct dpif_flow_stats *); >>> >>> /* Get the number of flows offloaded to netdev. >>> + * 'n_flows' is an array of counters, one per offload thread. >>> * Return 0 if successful, otherwise returns a positive errno value. */ >>> int (*flow_get_n_flows)(struct netdev *, uint64_t *n_flows); >>> >>> diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c >>> index 3092f8d55..c30377b91 100644 >>> --- a/lib/netdev-offload.c >>> +++ b/lib/netdev-offload.c >>> @@ -60,6 +60,12 @@ VLOG_DEFINE_THIS_MODULE(netdev_offload); >>> >>> static bool netdev_flow_api_enabled = false; >>> >>> +#define DEFAULT_OFFLOAD_THREAD_NB 1 >>> +#define MAX_OFFLOAD_THREAD_NB 10 >> >> Out of curiosity, is there a rationale in setting the maximum number of >> threads to 10? >> >> Maxime > > Hi Maxime, > > The limit is mostly arbitrary. I didn't want to allow setting outlandish values, some memory allocations depends on it. > In our driver, scaling is effective up to 4 threads at most, and one core use-case of OVS for us, Bluefield, has 8 cores. Ok. > 10 threads seemed a reasonable limit. I think it's good to have a margin, to test with higher-than-effective numbers in development, while in production we expect on our end to run 4 threads in general. Ok, thanks for the clarification. It might be good to add this information to the commit message, so that the day someone wants to revisit the value, he can understand how current one was selected. > Maybe the limit could be removed, similar to n-handle-threads and n-revalidator-threads. > On that point: those two will adapt to each other and depend on the number of CPU cores. Maybe we might also want to allow some budget for offload thread(s), and possibly ct_clean as well. > Yes, that might be a good alternative. As if you had lots of CPU cores and multiple devices, it might make sense to have more threads? Thanks, Maxime
diff --git a/lib/netdev-offload-provider.h b/lib/netdev-offload-provider.h index 4815a1bd2..f8b66961d 100644 --- a/lib/netdev-offload-provider.h +++ b/lib/netdev-offload-provider.h @@ -84,6 +84,7 @@ struct netdev_flow_api { struct dpif_flow_stats *); /* Get the number of flows offloaded to netdev. + * 'n_flows' is an array of counters, one per offload thread. * Return 0 if successful, otherwise returns a positive errno value. */ int (*flow_get_n_flows)(struct netdev *, uint64_t *n_flows); diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c index 3092f8d55..c30377b91 100644 --- a/lib/netdev-offload.c +++ b/lib/netdev-offload.c @@ -60,6 +60,12 @@ VLOG_DEFINE_THIS_MODULE(netdev_offload); static bool netdev_flow_api_enabled = false; +#define DEFAULT_OFFLOAD_THREAD_NB 1 +#define MAX_OFFLOAD_THREAD_NB 10 + +static unsigned int offload_thread_nb = DEFAULT_OFFLOAD_THREAD_NB; +DEFINE_EXTERN_PER_THREAD_DATA(netdev_offload_thread_id, OVSTHREAD_ID_UNSET); + /* Protects 'netdev_flow_apis'. */ static struct ovs_mutex netdev_flow_api_provider_mutex = OVS_MUTEX_INITIALIZER; @@ -436,6 +442,64 @@ netdev_is_flow_api_enabled(void) return netdev_flow_api_enabled; } +unsigned int +netdev_offload_thread_nb(void) +{ + return offload_thread_nb; +} + +unsigned int +netdev_offload_ufid_to_thread_id(const ovs_u128 ufid) +{ + uint32_t ufid_hash; + + if (netdev_offload_thread_nb() == 1) { + return 0; + } + + ufid_hash = hash_words64_inline( + (const uint64_t [2]){ ufid.u64.lo, + ufid.u64.hi }, 2, 1); + return ufid_hash % netdev_offload_thread_nb(); +} + +unsigned int +netdev_offload_thread_init(void) +{ + static atomic_count next_id = ATOMIC_COUNT_INIT(0); + bool thread_is_hw_offload; + bool thread_is_rcu; + + thread_is_hw_offload = !strncmp(get_subprogram_name(), + "hw_offload", strlen("hw_offload")); + thread_is_rcu = !strncmp(get_subprogram_name(), "urcu", strlen("urcu")); + + /* Panic if any other thread besides offload and RCU tries + * to initialize their thread ID. */ + ovs_assert(thread_is_hw_offload || thread_is_rcu); + + if (*netdev_offload_thread_id_get() == OVSTHREAD_ID_UNSET) { + unsigned int id; + + if (thread_is_rcu) { + /* RCU will compete with other threads for shared object access. + * Reclamation functions using a thread ID must be thread-safe. + * For that end, and because RCU must consider all potential shared + * objects anyway, its thread-id can be whichever, so return 0. + */ + id = 0; + } else { + /* Only the actual offload threads have their own ID. */ + id = atomic_count_inc(&next_id); + } + /* Panic if any offload thread is getting a spurious ID. */ + ovs_assert(id < netdev_offload_thread_nb()); + return *netdev_offload_thread_id_get() = id; + } else { + return *netdev_offload_thread_id_get(); + } +} + void netdev_ports_flow_flush(const char *dpif_type) { @@ -627,7 +691,16 @@ netdev_ports_get_n_flows(const char *dpif_type, odp_port_t port_no, ovs_rwlock_rdlock(&netdev_hmap_rwlock); data = netdev_ports_lookup(port_no, dpif_type); if (data) { - ret = netdev_flow_get_n_flows(data->netdev, n_flows); + uint64_t thread_n_flows[MAX_OFFLOAD_THREAD_NB] = {0}; + unsigned int tid; + + ret = netdev_flow_get_n_flows(data->netdev, thread_n_flows); + *n_flows = 0; + if (!ret) { + for (tid = 0; tid < netdev_offload_thread_nb(); tid++) { + *n_flows += thread_n_flows[tid]; + } + } } ovs_rwlock_unlock(&netdev_hmap_rwlock); return ret; @@ -680,7 +753,18 @@ netdev_set_flow_api_enabled(const struct smap *ovs_other_config) if (ovsthread_once_start(&once)) { netdev_flow_api_enabled = true; - VLOG_INFO("netdev: Flow API Enabled"); + offload_thread_nb = smap_get_ullong(ovs_other_config, + "n-offload-threads", + DEFAULT_OFFLOAD_THREAD_NB); + if (offload_thread_nb > MAX_OFFLOAD_THREAD_NB) { + VLOG_WARN("netdev: Invalid number of threads requested: %u", + offload_thread_nb); + offload_thread_nb = DEFAULT_OFFLOAD_THREAD_NB; + } + + VLOG_INFO("netdev: Flow API Enabled, using %u thread%s", + offload_thread_nb, + offload_thread_nb > 1 ? "s" : ""); #ifdef __linux__ tc_set_policy(smap_get_def(ovs_other_config, "tc-policy", diff --git a/lib/netdev-offload.h b/lib/netdev-offload.h index d820e23ed..6e72456e1 100644 --- a/lib/netdev-offload.h +++ b/lib/netdev-offload.h @@ -21,6 +21,7 @@ #include "openvswitch/netdev.h" #include "openvswitch/types.h" #include "ovs-rcu.h" +#include "ovs-thread.h" #include "packets.h" #include "flow.h" @@ -80,6 +81,24 @@ struct offload_info { * to delete the original flow. */ }; +DECLARE_EXTERN_PER_THREAD_DATA(unsigned int, netdev_offload_thread_id); + +unsigned int netdev_offload_thread_nb(void); +unsigned int netdev_offload_thread_init(void); +unsigned int netdev_offload_ufid_to_thread_id(const ovs_u128 ufid); + +static inline unsigned int +netdev_offload_thread_id(void) +{ + unsigned int id = *netdev_offload_thread_id_get(); + + if (OVS_UNLIKELY(id == OVSTHREAD_ID_UNSET)) { + id = netdev_offload_thread_init(); + } + + return id; +} + int netdev_flow_flush(struct netdev *); int netdev_flow_dump_create(struct netdev *, struct netdev_flow_dump **dump, bool terse); diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml index a2ad84ede..fc66a6d3e 100644 --- a/vswitchd/vswitch.xml +++ b/vswitchd/vswitch.xml @@ -247,6 +247,22 @@ </p> </column> + <column name="other_config" key="n-offload-threads" + type='{"type": "integer", "minInteger": 1, "maxInteger": 10}'> + <p> + Set this value to the number of threads created to manage hardware + offloads. + </p> + <p> + The default value is <code>1</code>. Changing this value requires + restarting the daemon. + </p> + <p> + This is only relevant if + <ref column="other_config" key="hw-offload"/> is enabled. + </p> + </column> + <column name="other_config" key="tc-policy" type='{"type": "string", "enum": ["set", ["none", "skip_sw", "skip_hw"]]}'>