| Message ID | 20210517140434.59555-4-cian.ferriter@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | DPIF Framework + Optimizations | expand |
> This commit adds a function pointer to the pmd thread data structure, > giving the pmd thread flexibility in its dpif-input function choice. > This allows choosing of the implementation based on ISA capabilities > of the runtime CPU, leading to optimizations and higher performance. > > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> Thanks for the patch Harry/Cian, a few minor comments below. > --- > lib/dpif-netdev-private-thread.h | 12 ++++++++++++ > lib/dpif-netdev.c | 7 ++++++- > 2 files changed, 18 insertions(+), 1 deletion(-) > > diff --git a/lib/dpif-netdev-private-thread.h b/lib/dpif-netdev-private-thread.h > index 5e5308b96..01a28a681 100644 > --- a/lib/dpif-netdev-private-thread.h > +++ b/lib/dpif-netdev-private-thread.h > @@ -47,6 +47,13 @@ struct dp_netdev_pmd_thread_ctx { > uint32_t emc_insert_min; > }; > > +/* Forward declaration for typedef. */ > +struct dp_netdev_pmd_thread; > + > +typedef void (*dp_netdev_input_func)(struct dp_netdev_pmd_thread *pmd, > + struct dp_packet_batch *packets, > + odp_port_t port_no); > + > /* PMD: Poll modes drivers. PMD accesses devices via polling to eliminate > * the performance overhead of interrupt processing. Therefore netdev can > * not implement rx-wait for these devices. dpif-netdev needs to poll > @@ -101,6 +108,11 @@ struct dp_netdev_pmd_thread { > /* Current context of the PMD thread. */ > struct dp_netdev_pmd_thread_ctx ctx; > > + /* Function pointer to call for dp_netdev_input() functionality. */ > + dp_netdev_input_func netdev_input_func; Probably white space to be added here to improve readability. > + /* Pointer for per-DPIF implementation scratch space. */ > + void *netdev_input_func_userdata; > + > struct seq *reload_seq; > uint64_t last_reload_seq; > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index 88f37c505..bec984643 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -4234,8 +4234,9 @@ dp_netdev_process_rxq_port(struct > dp_netdev_pmd_thread *pmd, > } > } > } > + > /* Process packet batch. */ > - dp_netdev_input(pmd, &batch, port_no); > + pmd->netdev_input_func(pmd, &batch, port_no); > > /* Assign processing cycles to rx queue. */ > cycles = cycle_timer_stop(&pmd->perf_stats, &timer); > @@ -6033,6 +6034,10 @@ dp_netdev_configure_pmd(struct > dp_netdev_pmd_thread *pmd, struct dp_netdev *dp, > hmap_init(&pmd->tnl_port_cache); > hmap_init(&pmd->send_port_cache); > cmap_init(&pmd->tx_bonds); > + > + /* Initialize the DPIF function pointer to the default scalar version. */ > + pmd->netdev_input_func = dp_netdev_input; > + I like the approach above, this follows what we has previously been implemented for DPCLS. It's good the existing scalar is the default as well so there is no change for existing users out of the box. BR Ian > /* init the 'flow_cache' since there is no > * actual thread created for NON_PMD_CORE_ID. */ > if (core_id == NON_PMD_CORE_ID) { > -- > 2.31.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Hi Ian, Thanks for the review. My responses are inline. > -----Original Message----- > From: Stokes, Ian <ian.stokes@intel.com> > Sent: Tuesday 1 June 2021 19:58 > To: Ferriter, Cian <cian.ferriter@intel.com>; ovs-dev@openvswitch.org; Van Haaren, Harry <harry.van.haaren@intel.com> > Cc: i.maximets@ovn.org > Subject: RE: [ovs-dev] [v12 03/16] dpif-netdev: Add function pointer for netdev input. > > > This commit adds a function pointer to the pmd thread data structure, > > giving the pmd thread flexibility in its dpif-input function choice. > > This allows choosing of the implementation based on ISA capabilities > > of the runtime CPU, leading to optimizations and higher performance. > > > > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> > > Thanks for the patch Harry/Cian, a few minor comments below. > > > --- > > lib/dpif-netdev-private-thread.h | 12 ++++++++++++ > > lib/dpif-netdev.c | 7 ++++++- > > 2 files changed, 18 insertions(+), 1 deletion(-) > > > > diff --git a/lib/dpif-netdev-private-thread.h b/lib/dpif-netdev-private-thread.h > > index 5e5308b96..01a28a681 100644 > > --- a/lib/dpif-netdev-private-thread.h > > +++ b/lib/dpif-netdev-private-thread.h > > @@ -47,6 +47,13 @@ struct dp_netdev_pmd_thread_ctx { > > uint32_t emc_insert_min; > > }; > > > > +/* Forward declaration for typedef. */ > > +struct dp_netdev_pmd_thread; > > + > > +typedef void (*dp_netdev_input_func)(struct dp_netdev_pmd_thread *pmd, > > + struct dp_packet_batch *packets, > > + odp_port_t port_no); > > + > > /* PMD: Poll modes drivers. PMD accesses devices via polling to eliminate > > * the performance overhead of interrupt processing. Therefore netdev can > > * not implement rx-wait for these devices. dpif-netdev needs to poll > > @@ -101,6 +108,11 @@ struct dp_netdev_pmd_thread { > > /* Current context of the PMD thread. */ > > struct dp_netdev_pmd_thread_ctx ctx; > > > > + /* Function pointer to call for dp_netdev_input() functionality. */ > > + dp_netdev_input_func netdev_input_func; > > Probably white space to be added here to improve readability. > Sure, I'll add this in the next version of the patch set. > > + /* Pointer for per-DPIF implementation scratch space. */ > > + void *netdev_input_func_userdata; > > + > > struct seq *reload_seq; > > uint64_t last_reload_seq; > > > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > > index 88f37c505..bec984643 100644 > > --- a/lib/dpif-netdev.c > > +++ b/lib/dpif-netdev.c > > @@ -4234,8 +4234,9 @@ dp_netdev_process_rxq_port(struct > > dp_netdev_pmd_thread *pmd, > > } > > } > > } > > + > > /* Process packet batch. */ > > - dp_netdev_input(pmd, &batch, port_no); > > + pmd->netdev_input_func(pmd, &batch, port_no); > > > > /* Assign processing cycles to rx queue. */ > > cycles = cycle_timer_stop(&pmd->perf_stats, &timer); > > @@ -6033,6 +6034,10 @@ dp_netdev_configure_pmd(struct > > dp_netdev_pmd_thread *pmd, struct dp_netdev *dp, > > hmap_init(&pmd->tnl_port_cache); > > hmap_init(&pmd->send_port_cache); > > cmap_init(&pmd->tx_bonds); > > + > > + /* Initialize the DPIF function pointer to the default scalar version. */ > > + pmd->netdev_input_func = dp_netdev_input; > > + > > I like the approach above, this follows what we has previously been implemented for DPCLS. > > It's good the existing scalar is the default as well so there is no change for existing users out of the box. > > BR > Ian Yes, the above aims to be extensible for multiple difference implementations of the DPIF. Thanks, Cian > > /* init the 'flow_cache' since there is no > > * actual thread created for NON_PMD_CORE_ID. */ > > if (core_id == NON_PMD_CORE_ID) { > > -- > > 2.31.1 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> Hi Ian, > > Thanks for the review. My responses are inline. > > > -----Original Message----- > > From: Stokes, Ian <ian.stokes@intel.com> > > Sent: Tuesday 1 June 2021 19:58 > > To: Ferriter, Cian <cian.ferriter@intel.com>; ovs-dev@openvswitch.org; Van > Haaren, Harry <harry.van.haaren@intel.com> > > Cc: i.maximets@ovn.org > > Subject: RE: [ovs-dev] [v12 03/16] dpif-netdev: Add function pointer for netdev > input. > > > > > This commit adds a function pointer to the pmd thread data structure, > > > giving the pmd thread flexibility in its dpif-input function choice. > > > This allows choosing of the implementation based on ISA capabilities > > > of the runtime CPU, leading to optimizations and higher performance. > > > > > > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> > > > > Thanks for the patch Harry/Cian, a few minor comments below. > > > > > --- > > > lib/dpif-netdev-private-thread.h | 12 ++++++++++++ > > > lib/dpif-netdev.c | 7 ++++++- > > > 2 files changed, 18 insertions(+), 1 deletion(-) > > > > > > diff --git a/lib/dpif-netdev-private-thread.h b/lib/dpif-netdev-private- > thread.h > > > index 5e5308b96..01a28a681 100644 > > > --- a/lib/dpif-netdev-private-thread.h > > > +++ b/lib/dpif-netdev-private-thread.h > > > @@ -47,6 +47,13 @@ struct dp_netdev_pmd_thread_ctx { > > > uint32_t emc_insert_min; > > > }; > > > > > > +/* Forward declaration for typedef. */ > > > +struct dp_netdev_pmd_thread; > > > + > > > +typedef void (*dp_netdev_input_func)(struct dp_netdev_pmd_thread > *pmd, > > > + struct dp_packet_batch *packets, > > > + odp_port_t port_no); > > > + > > > /* PMD: Poll modes drivers. PMD accesses devices via polling to eliminate > > > * the performance overhead of interrupt processing. Therefore netdev can > > > * not implement rx-wait for these devices. dpif-netdev needs to poll > > > @@ -101,6 +108,11 @@ struct dp_netdev_pmd_thread { > > > /* Current context of the PMD thread. */ > > > struct dp_netdev_pmd_thread_ctx ctx; > > > > > > + /* Function pointer to call for dp_netdev_input() functionality. */ > > > + dp_netdev_input_func netdev_input_func; > > > > Probably white space to be added here to improve readability. > > > > Sure, I'll add this in the next version of the patch set. > > > > + /* Pointer for per-DPIF implementation scratch space. */ > > > + void *netdev_input_func_userdata; > > > + > > > struct seq *reload_seq; > > > uint64_t last_reload_seq; > > > > > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > > > index 88f37c505..bec984643 100644 > > > --- a/lib/dpif-netdev.c > > > +++ b/lib/dpif-netdev.c > > > @@ -4234,8 +4234,9 @@ dp_netdev_process_rxq_port(struct > > > dp_netdev_pmd_thread *pmd, > > > } > > > } > > > } > > > + > > > /* Process packet batch. */ > > > - dp_netdev_input(pmd, &batch, port_no); > > > + pmd->netdev_input_func(pmd, &batch, port_no); > > > > > > /* Assign processing cycles to rx queue. */ > > > cycles = cycle_timer_stop(&pmd->perf_stats, &timer); > > > @@ -6033,6 +6034,10 @@ dp_netdev_configure_pmd(struct > > > dp_netdev_pmd_thread *pmd, struct dp_netdev *dp, > > > hmap_init(&pmd->tnl_port_cache); > > > hmap_init(&pmd->send_port_cache); > > > cmap_init(&pmd->tx_bonds); > > > + > > > + /* Initialize the DPIF function pointer to the default scalar version. */ > > > + pmd->netdev_input_func = dp_netdev_input; > > > + > > > > I like the approach above, this follows what we has previously been > implemented for DPCLS. > > > > It's good the existing scalar is the default as well so there is no change for > existing users out of the box. > > > > BR > > Ian > > Yes, the above aims to be extensible for multiple difference implementations of > the DPIF. +1, that's key in this work for sure. Thanks Ian > > Thanks, > Cian > > > > /* init the 'flow_cache' since there is no > > > * actual thread created for NON_PMD_CORE_ID. */ > > > if (core_id == NON_PMD_CORE_ID) { > > > -- > > > 2.31.1 > > > > > > _______________________________________________ > > > dev mailing list > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/lib/dpif-netdev-private-thread.h b/lib/dpif-netdev-private-thread.h index 5e5308b96..01a28a681 100644 --- a/lib/dpif-netdev-private-thread.h +++ b/lib/dpif-netdev-private-thread.h @@ -47,6 +47,13 @@ struct dp_netdev_pmd_thread_ctx { uint32_t emc_insert_min; }; +/* Forward declaration for typedef. */ +struct dp_netdev_pmd_thread; + +typedef void (*dp_netdev_input_func)(struct dp_netdev_pmd_thread *pmd, + struct dp_packet_batch *packets, + odp_port_t port_no); + /* PMD: Poll modes drivers. PMD accesses devices via polling to eliminate * the performance overhead of interrupt processing. Therefore netdev can * not implement rx-wait for these devices. dpif-netdev needs to poll @@ -101,6 +108,11 @@ struct dp_netdev_pmd_thread { /* Current context of the PMD thread. */ struct dp_netdev_pmd_thread_ctx ctx; + /* Function pointer to call for dp_netdev_input() functionality. */ + dp_netdev_input_func netdev_input_func; + /* Pointer for per-DPIF implementation scratch space. */ + void *netdev_input_func_userdata; + struct seq *reload_seq; uint64_t last_reload_seq; diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 88f37c505..bec984643 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -4234,8 +4234,9 @@ dp_netdev_process_rxq_port(struct dp_netdev_pmd_thread *pmd, } } } + /* Process packet batch. */ - dp_netdev_input(pmd, &batch, port_no); + pmd->netdev_input_func(pmd, &batch, port_no); /* Assign processing cycles to rx queue. */ cycles = cycle_timer_stop(&pmd->perf_stats, &timer); @@ -6033,6 +6034,10 @@ dp_netdev_configure_pmd(struct dp_netdev_pmd_thread *pmd, struct dp_netdev *dp, hmap_init(&pmd->tnl_port_cache); hmap_init(&pmd->send_port_cache); cmap_init(&pmd->tx_bonds); + + /* Initialize the DPIF function pointer to the default scalar version. */ + pmd->netdev_input_func = dp_netdev_input; + /* init the 'flow_cache' since there is no * actual thread created for NON_PMD_CORE_ID. */ if (core_id == NON_PMD_CORE_ID) {