| Message ID | 20230309130252.356289-3-simon.horman@corigine.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for DPDK meter HW offload | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
References: <20230309130252.356289-3-simon.horman@corigine.com> Bleep bloop. Greetings Simon Horman, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Jin Liu <jin.liu@corigine.com>, Simon Horman <simon.horman@corigine.com> Lines checked: 224, Warnings: 1, Errors: 0 Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
On 3/9/23 14:02, Simon Horman wrote: > From: Peng Zhang <peng.zhang@corigine.com> > > Add API to offload DPDK meter to HW, and the corresponding functions to call > the DPDK meter callbacks from all the registered flow API providers. > The interfaces are like those related to DPDK meter in dpif_class, in order > to pass necessary info to HW. > > Signed-off-by: Peng Zhang <peng.zhang@corigine.com> > Signed-off-by: Jin Liu <jin.liu@corigine.com> > Signed-off-by: Simon Horman <simon.horman@corigine.com> > --- > Documentation/howto/dpdk.rst | 5 +- > lib/netdev-offload-provider.h | 30 +++++++++++ > lib/netdev-offload.c | 99 +++++++++++++++++++++++++++++++++++ > lib/netdev-offload.h | 9 ++++ > 4 files changed, 141 insertions(+), 2 deletions(-) > > diff --git a/Documentation/howto/dpdk.rst b/Documentation/howto/dpdk.rst > index 04609b20bd21..02fc568770ee 100644 > --- a/Documentation/howto/dpdk.rst > +++ b/Documentation/howto/dpdk.rst > @@ -401,10 +401,11 @@ Supported actions for hardware offload are: > - Modification of IPv6 (set_field:<ADDR>->ipv6_src/ipv6_dst/mod_nw_ttl). > - Clone/output (tnl_push and output) for encapsulating over a tunnel. > - Tunnel pop, for packets received on physical ports. > +- Meter. > > .. note:: > - Tunnel offloads are experimental APIs in DPDK. In order to enable it, > - compile with -DALLOW_EXPERIMENTAL_API. > + Tunnel offloads and Meter offloads are experimental APIs in DPDK. To enable > + these features, compile with -DALLOW_EXPERIMENTAL_API. > > Multiprocess > ------------ > diff --git a/lib/netdev-offload-provider.h b/lib/netdev-offload-provider.h > index 9108856d18d1..7ecbb8d026f1 100644 > --- a/lib/netdev-offload-provider.h > +++ b/lib/netdev-offload-provider.h > @@ -102,6 +102,16 @@ struct netdev_flow_api { > int (*meter_set)(ofproto_meter_id meter_id, > struct ofputil_meter_config *config); > > + /* Offloads or modifies the offloaded meter on the netdev with the given > + * 'meter_id' and the configuration in 'config'. On failure, a non-zero > + * error code is returned. > + * > + * The meter id specified through 'config->meter_id' is converted as an > + * internal meter id. */ > + int (*dpdk_meter_set)(struct netdev *, > + ofproto_meter_id meter_id, > + struct ofputil_meter_config *); Hi. This looks strange. We do already have the meter API here. There is no need to create a separate set of APIs for each offload provider. This breaks the abstraction. If there is something missing in existing meter_* callbacks, modify them, so they can be suitable for both implementations. If you need to iterate over all the ports, we have a special netdev_ports_traverse() function that can be used from the netdev-offload-dpdk. Also, In my reply for v1 I mentioned that this patch set should be sent for dpdk-latest branch, not master. i.e., it should have 'dpdk-latest' in the subject prefix. CC: Ian. dpdk-latest branch may need a rebase though. Ian, may I ask you to rebase it on the latest master? Or I can do that myself, if necessary. Best regards, Ilya Maximets.
On Wed, Mar 22, 2023 at 09:35:29PM +0100, Ilya Maximets wrote: > On 3/9/23 14:02, Simon Horman wrote: > > From: Peng Zhang <peng.zhang@corigine.com> ... > > diff --git a/lib/netdev-offload-provider.h b/lib/netdev-offload-provider.h > > index 9108856d18d1..7ecbb8d026f1 100644 > > --- a/lib/netdev-offload-provider.h > > +++ b/lib/netdev-offload-provider.h > > @@ -102,6 +102,16 @@ struct netdev_flow_api { > > int (*meter_set)(ofproto_meter_id meter_id, > > struct ofputil_meter_config *config); > > > > + /* Offloads or modifies the offloaded meter on the netdev with the given > > + * 'meter_id' and the configuration in 'config'. On failure, a non-zero > > + * error code is returned. > > + * > > + * The meter id specified through 'config->meter_id' is converted as an > > + * internal meter id. */ > > + int (*dpdk_meter_set)(struct netdev *, > > + ofproto_meter_id meter_id, > > + struct ofputil_meter_config *); > > Hi. > > This looks strange. We do already have the meter API here. > There is no need to create a separate set of APIs for each > offload provider. This breaks the abstraction. > > If there is something missing in existing meter_* callbacks, > modify them, so they can be suitable for both implementations. > If you need to iterate over all the ports, we have a special > netdev_ports_traverse() function that can be used from the > netdev-offload-dpdk. Thanks, we are working on revising this accordingly. > Also, In my reply for v1 I mentioned that this patch set > should be sent for dpdk-latest branch, not master. i.e., > it should have 'dpdk-latest' in the subject prefix. Sorry about that. This series is based on dpdk-latest. But I omitted dpdk-latest from the subject prefix. > CC: Ian. > > dpdk-latest branch may need a rebase though. Ian, may I ask > you to rebase it on the latest master? Or I can do that > myself, if necessary. > > Best regards, Ilya Maximets.
diff --git a/Documentation/howto/dpdk.rst b/Documentation/howto/dpdk.rst index 04609b20bd21..02fc568770ee 100644 --- a/Documentation/howto/dpdk.rst +++ b/Documentation/howto/dpdk.rst @@ -401,10 +401,11 @@ Supported actions for hardware offload are: - Modification of IPv6 (set_field:<ADDR>->ipv6_src/ipv6_dst/mod_nw_ttl). - Clone/output (tnl_push and output) for encapsulating over a tunnel. - Tunnel pop, for packets received on physical ports. +- Meter. .. note:: - Tunnel offloads are experimental APIs in DPDK. In order to enable it, - compile with -DALLOW_EXPERIMENTAL_API. + Tunnel offloads and Meter offloads are experimental APIs in DPDK. To enable + these features, compile with -DALLOW_EXPERIMENTAL_API. Multiprocess ------------ diff --git a/lib/netdev-offload-provider.h b/lib/netdev-offload-provider.h index 9108856d18d1..7ecbb8d026f1 100644 --- a/lib/netdev-offload-provider.h +++ b/lib/netdev-offload-provider.h @@ -102,6 +102,16 @@ struct netdev_flow_api { int (*meter_set)(ofproto_meter_id meter_id, struct ofputil_meter_config *config); + /* Offloads or modifies the offloaded meter on the netdev with the given + * 'meter_id' and the configuration in 'config'. On failure, a non-zero + * error code is returned. + * + * The meter id specified through 'config->meter_id' is converted as an + * internal meter id. */ + int (*dpdk_meter_set)(struct netdev *, + ofproto_meter_id meter_id, + struct ofputil_meter_config *); + /* Queries HW for meter stats with the given 'meter_id'. Store the stats * of dropped packets to band 0. On failure, a non-zero error code is * returned. @@ -113,6 +123,18 @@ struct netdev_flow_api { int (*meter_get)(ofproto_meter_id meter_id, struct ofputil_meter_stats *stats); + /* Queries netdev for meter stats with the given 'meter_id'. Store the + * stats of dropped packets to band 0. On failure, a non-zero error code + * is returned. + * + * Note that the 'stats' structure is already initialized, and only the + * available statistics should be incremented, not replaced. Those fields + * are packet_in_count, byte_in_count and band[]->byte_count and + * band[]->packet_count. */ + int (*dpdk_meter_get)(struct netdev *, + ofproto_meter_id meter_id, + struct ofputil_meter_stats *); + /* Removes meter 'meter_id' from HW. Store the stats of dropped packets to * band 0. On failure, a non-zero error code is returned. * @@ -121,6 +143,14 @@ struct netdev_flow_api { int (*meter_del)(ofproto_meter_id meter_id, struct ofputil_meter_stats *stats); + /* Removes meter 'meter_id' from netdev. Store the stats of dropped packets + * to band 0. On failure, a non-zero error code is returned. + * + * If del success, 'stats' will be set zero. */ + int (*dpdk_meter_del)(struct netdev *, + ofproto_meter_id meter_id, + struct ofputil_meter_stats *stats); + /* Initializies the netdev flow api. * Return 0 if successful, otherwise returns a positive errno value. */ int (*init_flow_api)(struct netdev *); diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c index 4592262bd34e..44cab0c4ee79 100644 --- a/lib/netdev-offload.c +++ b/lib/netdev-offload.c @@ -895,3 +895,102 @@ netdev_set_flow_api_enabled(const struct smap *ovs_other_config) } } } + +void +dpdk_meter_offload_set(const char *dpif_type, + ofproto_meter_id meter_id, + struct ofputil_meter_config *config) +{ + struct netdev_registered_flow_api *rfa; + struct port_to_netdev_data *data; + struct netdev *dev; + int ret; + + ovs_rwlock_rdlock(&netdev_hmap_rwlock); + HMAP_FOR_EACH (data, portno_node, &port_to_netdev) { + dev = data->netdev; + if (netdev_get_dpif_type(dev) == dpif_type) { + /* Offload APIs could fail, for example, because the offload is not + * supported. This is fine, as the offload API should take care of + * this. */ + CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) { + if (rfa->flow_api->dpdk_meter_set) { + ret = rfa->flow_api->dpdk_meter_set(dev, meter_id, config); + if (ret) { + VLOG_DBG_RL(&rl, "Failed setting meter %u for flow api" + " %s, error %d", meter_id.uint32, + rfa->flow_api->type, ret); + } + } + } + } + } + ovs_rwlock_unlock(&netdev_hmap_rwlock); +} + +void +dpdk_meter_offload_get(const char *dpif_type, + ofproto_meter_id meter_id, + struct ofputil_meter_stats *stats) +{ + struct netdev_registered_flow_api *rfa; + struct ofputil_meter_stats offload_stats; + struct port_to_netdev_data *data; + struct netdev *dev; + int ret; + + ovs_rwlock_rdlock(&netdev_hmap_rwlock); + HMAP_FOR_EACH (data, portno_node, &port_to_netdev) { + memset(&offload_stats, 0, sizeof(struct ofputil_meter_stats)); + dev = data->netdev; + if (netdev_get_dpif_type(dev) == dpif_type) { + CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) { + if (rfa->flow_api->dpdk_meter_get) { + ret = rfa->flow_api->dpdk_meter_get(dev, meter_id, stats); + if (ret) { + VLOG_DBG_RL(&rl, "Failed getting meter %u for flow " + "api %s, error %d", meter_id.uint32, + rfa->flow_api->type, ret); + } + } + } + + if (!offload_stats.byte_in_count && + !offload_stats.packet_in_count) { + continue; + } + stats->byte_in_count += offload_stats.byte_in_count; + stats->packet_in_count += offload_stats.packet_in_count; + } + } + ovs_rwlock_unlock(&netdev_hmap_rwlock); +} + +void +dpdk_meter_offload_del(const char *dpif_type, + ofproto_meter_id meter_id, + struct ofputil_meter_stats *stats) +{ + struct netdev_registered_flow_api *rfa; + struct port_to_netdev_data *data; + struct netdev *dev; + int ret; + + ovs_rwlock_rdlock(&netdev_hmap_rwlock); + HMAP_FOR_EACH (data, portno_node, &port_to_netdev) { + dev = data->netdev; + if (netdev_get_dpif_type(dev) == dpif_type) { + CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) { + if (rfa->flow_api->dpdk_meter_del) { + ret = rfa->flow_api->dpdk_meter_del(dev, meter_id, stats); + if (ret) { + VLOG_DBG_RL(&rl, "Failed deleting meter %u for flow " + "api %s, error %d", meter_id.uint32, + rfa->flow_api->type, ret); + } + } + } + } + } + ovs_rwlock_unlock(&netdev_hmap_rwlock); +} diff --git a/lib/netdev-offload.h b/lib/netdev-offload.h index edc843cd99a3..e53f70a277b3 100644 --- a/lib/netdev-offload.h +++ b/lib/netdev-offload.h @@ -161,6 +161,15 @@ int netdev_ports_get_n_flows(const char *dpif_type, void meter_offload_set(ofproto_meter_id, struct ofputil_meter_config *); int meter_offload_get(ofproto_meter_id, struct ofputil_meter_stats *); int meter_offload_del(ofproto_meter_id, struct ofputil_meter_stats *); +void dpdk_meter_offload_set(const char *dpif_type, + ofproto_meter_id meter_id, + struct ofputil_meter_config *config); +void dpdk_meter_offload_del(const char *dpif_type, + ofproto_meter_id meter_id_, + struct ofputil_meter_stats *stats); +void dpdk_meter_offload_get(const char *dpif_type, + ofproto_meter_id meter_id_, + struct ofputil_meter_stats *stats); #ifdef __cplusplus }