| Message ID | 20220503030836.28783-2-jianbol@nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for ovs metering with tc offload | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 3 May 2022, at 5:08, Jianbo Liu via dev wrote: > Add API to offload meter to HW, and the corresponding functions to call > the meter callbacks from all the registered flow API providers. > The interfaces are like those related to meter in dpif_class, in order > to pass necessary info to HW. See my comments inline below. > Signed-off-by: Jianbo Liu <jianbol@nvidia.com> > --- > lib/netdev-offload-provider.h | 15 ++++++++++++ > lib/netdev-offload.c | 45 +++++++++++++++++++++++++++++++++++ > lib/netdev-offload.h | 8 +++++++ > 3 files changed, 68 insertions(+) > > diff --git a/lib/netdev-offload-provider.h b/lib/netdev-offload-provider.h > index 8ff2de983..2441534c3 100644 > --- a/lib/netdev-offload-provider.h > +++ b/lib/netdev-offload-provider.h > @@ -94,6 +94,21 @@ struct netdev_flow_api { > * takes ownership of a packet if errno != EOPNOTSUPP. */ > int (*hw_miss_packet_recover)(struct netdev *, struct dp_packet *); > > + /* Offloads the meter or modifies it if exists in HW > + * with the given 'meter_id' and the configuration in 'config'. */ > + int (*meter_set)(ofproto_meter_id meter_id, > + struct ofputil_meter_config *config); > + > + /* Queries HW for meter stats with the given 'meter_id'. */ > + int (*meter_get)(ofproto_meter_id meter_id, > + struct ofputil_meter_stats *stats, > + uint16_t max_bands); > + The comments on meter_del and meter_get need more details on how statistics are handled. See below, and the comments in dpif-provider.h. > + /* Removes meter 'meter_id' from HW. */ > + int (*meter_del)(ofproto_meter_id meter_id, > + struct ofputil_meter_stats *stats, > + uint16_t max_bands); > + > /* 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 fb108c0d5..41f6e1723 100644 > --- a/lib/netdev-offload.c > +++ b/lib/netdev-offload.c > @@ -195,6 +195,51 @@ netdev_assign_flow_api(struct netdev *netdev) > return -1; > } > > +int > +meter_offload_set(ofproto_meter_id meter_id, > + struct ofputil_meter_config *config) > +{ > + struct netdev_registered_flow_api *rfa; > + > + CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) { > + if (rfa->flow_api->meter_set) { > + rfa->flow_api->meter_set(meter_id, config); > + } > + } > + > + return 0; We are not reporting any failures here, this does not seem right to me? Same for the other three functions below. > +} > + > +int > +meter_offload_get(ofproto_meter_id meter_id, > + struct ofputil_meter_stats *stats, uint16_t max_bands) > +{ > + struct netdev_registered_flow_api *rfa; > + > + CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) { > + if (rfa->flow_api->meter_get) { > + rfa->flow_api->meter_get(meter_id, stats, max_bands); The following comments also apply to meter_offload_del() below, and comments on the first comment ;) So here in theory multiple offload APIs can handle the same meter ID, so here we should clear all statistics in the structure and the individual callers should ADD their counter, not replace them. Or we should pass in a tmp ofputil_meter_stats structure and do this. I think this goes for the stats->packet_in_count, stats->byte_in_count and band->byte_count and band->packet_count. > + } > + } > + > + return 0; > +} > + > +int > +meter_offload_del(ofproto_meter_id meter_id, > + struct ofputil_meter_stats *stats, uint16_t max_bands) > +{ > + struct netdev_registered_flow_api *rfa; > + > + CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) { > + if (rfa->flow_api->meter_del) { > + rfa->flow_api->meter_del(meter_id, stats, max_bands); > + } > + } > + > + return 0; > +} > + > int > netdev_flow_flush(struct netdev *netdev) > { > diff --git a/lib/netdev-offload.h b/lib/netdev-offload.h > index 8237a85dd..a6d4ba3cb 100644 > --- a/lib/netdev-offload.h > +++ b/lib/netdev-offload.h > @@ -22,6 +22,7 @@ > #include "openvswitch/types.h" > #include "ovs-rcu.h" > #include "ovs-thread.h" > +#include "openvswitch/ofp-meter.h" > #include "packets.h" > #include "flow.h" > > @@ -158,6 +159,13 @@ int netdev_ports_flow_get(const char *dpif_type, struct match *match, > int netdev_ports_get_n_flows(const char *dpif_type, > odp_port_t port_no, uint64_t *n_flows); > > +int meter_offload_set(ofproto_meter_id, > + struct ofputil_meter_config *); > +int meter_offload_get(ofproto_meter_id, > + struct ofputil_meter_stats *, uint16_t); > +int meter_offload_del(ofproto_meter_id, > + struct ofputil_meter_stats *, uint16_t); > + > #ifdef __cplusplus > } > #endif > -- > 2.26.2 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Fri, 2022-05-13 at 13:46 +0200, Eelco Chaudron wrote: > > > On 3 May 2022, at 5:08, Jianbo Liu via dev wrote: > > > Add API to offload meter to HW, and the corresponding functions to > > call > > the meter callbacks from all the registered flow API providers. > > The interfaces are like those related to meter in dpif_class, in > > order > > to pass necessary info to HW. > > See my comments inline below. > > > Signed-off-by: Jianbo Liu <jianbol@nvidia.com> > > --- > > lib/netdev-offload-provider.h | 15 ++++++++++++ > > lib/netdev-offload.c | 45 > > +++++++++++++++++++++++++++++++++++ > > lib/netdev-offload.h | 8 +++++++ > > 3 files changed, 68 insertions(+) > > > > diff --git a/lib/netdev-offload-provider.h b/lib/netdev-offload- > > provider.h > > index 8ff2de983..2441534c3 100644 > > --- a/lib/netdev-offload-provider.h > > +++ b/lib/netdev-offload-provider.h > > @@ -94,6 +94,21 @@ struct netdev_flow_api { > > * takes ownership of a packet if errno != EOPNOTSUPP. */ > > int (*hw_miss_packet_recover)(struct netdev *, struct > > dp_packet *); > > > > + /* Offloads the meter or modifies it if exists in HW > > + * with the given 'meter_id' and the configuration in > > 'config'. */ > > + int (*meter_set)(ofproto_meter_id meter_id, > > + struct ofputil_meter_config *config); > > + > > + /* Queries HW for meter stats with the given 'meter_id'. */ > > + int (*meter_get)(ofproto_meter_id meter_id, > > + struct ofputil_meter_stats *stats, > > + uint16_t max_bands); > > + > > The comments on meter_del and meter_get need more details on how > statistics are handled. See below, and the comments in dpif- > provider.h. > OK. > > + /* Removes meter 'meter_id' from HW. */ > > + int (*meter_del)(ofproto_meter_id meter_id, > > + struct ofputil_meter_stats *stats, > > + uint16_t max_bands); > > + > > /* 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 fb108c0d5..41f6e1723 100644 > > --- a/lib/netdev-offload.c > > +++ b/lib/netdev-offload.c > > @@ -195,6 +195,51 @@ netdev_assign_flow_api(struct netdev *netdev) > > return -1; > > } > > > > +int > > +meter_offload_set(ofproto_meter_id meter_id, > > + struct ofputil_meter_config *config) > > +{ > > + struct netdev_registered_flow_api *rfa; > > + > > + CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) { > > + if (rfa->flow_api->meter_set) { > > + rfa->flow_api->meter_set(meter_id, config); > > + } > > + } > > + > > + return 0; > > We are not reporting any failures here, this does not seem right to > me? Same for the other three functions below. > If offload fails, ovs-meter should work as before. So, it's not necessary to check the return value, right? > > +} > > + > > +int > > +meter_offload_get(ofproto_meter_id meter_id, > > + struct ofputil_meter_stats *stats, uint16_t > > max_bands) > > +{ > > + struct netdev_registered_flow_api *rfa; > > + > > + CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) { > > + if (rfa->flow_api->meter_get) { > > + rfa->flow_api->meter_get(meter_id, stats, max_bands); > > The following comments also apply to meter_offload_del() below, and > comments on the first comment ;) > > So here in theory multiple offload APIs can handle the same meter ID, > so here we should clear all statistics in the structure and the > individual callers should ADD their counter, not replace them. > I don't know why the counters are replaced, could you please explain? > Or we should pass in a tmp ofputil_meter_stats structure and do this. > > I think this goes for the stats->packet_in_count, stats- > >byte_in_count and band->byte_count and band->packet_count. You can check in later patch that I added packet_in_count/byte_in_count by "+=", so the value is not replaced, but added. > > > + } > > + } > > + > > + return 0; > > +} > > + > > +int > > +meter_offload_del(ofproto_meter_id meter_id, > > + struct ofputil_meter_stats *stats, uint16_t > > max_bands) > > +{ > > + struct netdev_registered_flow_api *rfa; > > + > > + CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) { > > + if (rfa->flow_api->meter_del) { > > + rfa->flow_api->meter_del(meter_id, stats, max_bands); > > + } > > + } > > + > > + return 0; > > +} > > + > > int > > netdev_flow_flush(struct netdev *netdev) > > { > > diff --git a/lib/netdev-offload.h b/lib/netdev-offload.h > > index 8237a85dd..a6d4ba3cb 100644 > > --- a/lib/netdev-offload.h > > +++ b/lib/netdev-offload.h > > @@ -22,6 +22,7 @@ > > #include "openvswitch/types.h" > > #include "ovs-rcu.h" > > #include "ovs-thread.h" > > +#include "openvswitch/ofp-meter.h" > > #include "packets.h" > > #include "flow.h" > > > > @@ -158,6 +159,13 @@ int netdev_ports_flow_get(const char > > *dpif_type, struct match *match, > > int netdev_ports_get_n_flows(const char *dpif_type, > > odp_port_t port_no, uint64_t > > *n_flows); > > > > +int meter_offload_set(ofproto_meter_id, > > + struct ofputil_meter_config *); > > +int meter_offload_get(ofproto_meter_id, > > + struct ofputil_meter_stats *, uint16_t); > > +int meter_offload_del(ofproto_meter_id, > > + struct ofputil_meter_stats *, uint16_t); > > + > > #ifdef __cplusplus > > } > > #endif > > -- > > 2.26.2 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 16 May 2022, at 12:31, Jianbo Liu wrote: > On Fri, 2022-05-13 at 13:46 +0200, Eelco Chaudron wrote: >> >> >> On 3 May 2022, at 5:08, Jianbo Liu via dev wrote: >> >>> Add API to offload meter to HW, and the corresponding functions to >>> call >>> the meter callbacks from all the registered flow API providers. >>> The interfaces are like those related to meter in dpif_class, in >>> order >>> to pass necessary info to HW. >> >> See my comments inline below. >> >>> Signed-off-by: Jianbo Liu <jianbol@nvidia.com> >>> --- >>> lib/netdev-offload-provider.h | 15 ++++++++++++ >>> lib/netdev-offload.c | 45 >>> +++++++++++++++++++++++++++++++++++ >>> lib/netdev-offload.h | 8 +++++++ >>> 3 files changed, 68 insertions(+) >>> >>> diff --git a/lib/netdev-offload-provider.h b/lib/netdev-offload- >>> provider.h >>> index 8ff2de983..2441534c3 100644 >>> --- a/lib/netdev-offload-provider.h >>> +++ b/lib/netdev-offload-provider.h >>> @@ -94,6 +94,21 @@ struct netdev_flow_api { >>> * takes ownership of a packet if errno != EOPNOTSUPP. */ >>> int (*hw_miss_packet_recover)(struct netdev *, struct >>> dp_packet *); >>> >>> + /* Offloads the meter or modifies it if exists in HW >>> + * with the given 'meter_id' and the configuration in >>> 'config'. */ >>> + int (*meter_set)(ofproto_meter_id meter_id, >>> + struct ofputil_meter_config *config); >>> + >>> + /* Queries HW for meter stats with the given 'meter_id'. */ >>> + int (*meter_get)(ofproto_meter_id meter_id, >>> + struct ofputil_meter_stats *stats, >>> + uint16_t max_bands); >>> + >> >> The comments on meter_del and meter_get need more details on how >> statistics are handled. See below, and the comments in dpif- >> provider.h. >> > > OK. > >>> + /* Removes meter 'meter_id' from HW. */ >>> + int (*meter_del)(ofproto_meter_id meter_id, >>> + struct ofputil_meter_stats *stats, >>> + uint16_t max_bands); >>> + >>> /* 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 fb108c0d5..41f6e1723 100644 >>> --- a/lib/netdev-offload.c >>> +++ b/lib/netdev-offload.c >>> @@ -195,6 +195,51 @@ netdev_assign_flow_api(struct netdev *netdev) >>> return -1; >>> } >>> >>> +int >>> +meter_offload_set(ofproto_meter_id meter_id, >>> + struct ofputil_meter_config *config) >>> +{ >>> + struct netdev_registered_flow_api *rfa; >>> + >>> + CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) { >>> + if (rfa->flow_api->meter_set) { >>> + rfa->flow_api->meter_set(meter_id, config); >>> + } >>> + } >>> + >>> + return 0; >> >> We are not reporting any failures here, this does not seem right to >> me? Same for the other three functions below. >> > > If offload fails, ovs-meter should work as before. So, it's not > necessary to check the return value, right? If this is the design, maybe we should at least add a log message so we know the initialization has failed, so we know why any flow inserts would fail with ENOSUPP? >>> +} >>> + >>> +int >>> +meter_offload_get(ofproto_meter_id meter_id, >>> + struct ofputil_meter_stats *stats, uint16_t >>> max_bands) >>> +{ >>> + struct netdev_registered_flow_api *rfa; >>> + >>> + CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) { >>> + if (rfa->flow_api->meter_get) { >>> + rfa->flow_api->meter_get(meter_id, stats, max_bands); >> >> The following comments also apply to meter_offload_del() below, and >> comments on the first comment ;) >> >> So here in theory multiple offload APIs can handle the same meter ID, >> so here we should clear all statistics in the structure and the > >> individual callers should ADD their counter, not replace them. >> > > I don't know why the counters are replaced, could you please explain? Because the API help not cleanly states that meters should add the value rather than replace, it could be wrongly interpreted. But I guess you are fixing this. >> Or we should pass in a tmp ofputil_meter_stats structure and do this. >> >> I think this goes for the stats->packet_in_count, stats- >>> byte_in_count and band->byte_count and band->packet_count. > > You can check in later patch that I added packet_in_count/byte_in_count > by "+=", so the value is not replaced, but added. ACK, ignore this part. > >> >>> + } >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +int >>> +meter_offload_del(ofproto_meter_id meter_id, >>> + struct ofputil_meter_stats *stats, uint16_t >>> max_bands) >>> +{ >>> + struct netdev_registered_flow_api *rfa; >>> + >>> + CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) { >>> + if (rfa->flow_api->meter_del) { >>> + rfa->flow_api->meter_del(meter_id, stats, max_bands); >>> + } >>> + } >>> + >>> + return 0; >>> +} >>> + >>> int >>> netdev_flow_flush(struct netdev *netdev) >>> { >>> diff --git a/lib/netdev-offload.h b/lib/netdev-offload.h >>> index 8237a85dd..a6d4ba3cb 100644 >>> --- a/lib/netdev-offload.h >>> +++ b/lib/netdev-offload.h >>> @@ -22,6 +22,7 @@ >>> #include "openvswitch/types.h" >>> #include "ovs-rcu.h" >>> #include "ovs-thread.h" >>> +#include "openvswitch/ofp-meter.h" >>> #include "packets.h" >>> #include "flow.h" >>> >>> @@ -158,6 +159,13 @@ int netdev_ports_flow_get(const char >>> *dpif_type, struct match *match, >>> int netdev_ports_get_n_flows(const char *dpif_type, >>> odp_port_t port_no, uint64_t >>> *n_flows); >>> >>> +int meter_offload_set(ofproto_meter_id, >>> + struct ofputil_meter_config *); >>> +int meter_offload_get(ofproto_meter_id, >>> + struct ofputil_meter_stats *, uint16_t); >>> +int meter_offload_del(ofproto_meter_id, >>> + struct ofputil_meter_stats *, uint16_t); >>> + >>> #ifdef __cplusplus >>> } >>> #endif >>> -- >>> 2.26.2 >>> >>> _______________________________________________ >>> dev mailing list >>> dev@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>
On Mon, 2022-05-23 at 11:59 +0200, Eelco Chaudron wrote: > > > On 16 May 2022, at 12:31, Jianbo Liu wrote: > > > On Fri, 2022-05-13 at 13:46 +0200, Eelco Chaudron wrote: > > > > > > > > > On 3 May 2022, at 5:08, Jianbo Liu via dev wrote: > > > > > > > Add API to offload meter to HW, and the corresponding functions > > > > to > > > > call > > > > the meter callbacks from all the registered flow API providers. > > > > The interfaces are like those related to meter in dpif_class, > > > > in > > > > order > > > > to pass necessary info to HW. > > > > > > See my comments inline below. > > > > > > > Signed-off-by: Jianbo Liu <jianbol@nvidia.com> > > > > --- > > > > lib/netdev-offload-provider.h | 15 ++++++++++++ > > > > lib/netdev-offload.c | 45 > > > > +++++++++++++++++++++++++++++++++++ > > > > lib/netdev-offload.h | 8 +++++++ > > > > 3 files changed, 68 insertions(+) > > > > > > > > diff --git a/lib/netdev-offload-provider.h b/lib/netdev- > > > > offload- > > > > provider.h > > > > index 8ff2de983..2441534c3 100644 > > > > --- a/lib/netdev-offload-provider.h > > > > +++ b/lib/netdev-offload-provider.h > > > > @@ -94,6 +94,21 @@ struct netdev_flow_api { > > > > * takes ownership of a packet if errno != EOPNOTSUPP. */ > > > > int (*hw_miss_packet_recover)(struct netdev *, struct > > > > dp_packet *); > > > > > > > > + /* Offloads the meter or modifies it if exists in HW > > > > + * with the given 'meter_id' and the configuration in > > > > 'config'. */ > > > > + int (*meter_set)(ofproto_meter_id meter_id, > > > > + struct ofputil_meter_config *config); > > > > + > > > > + /* Queries HW for meter stats with the given 'meter_id'. > > > > */ > > > > + int (*meter_get)(ofproto_meter_id meter_id, > > > > + struct ofputil_meter_stats *stats, > > > > + uint16_t max_bands); > > > > + > > > > > > The comments on meter_del and meter_get need more details on how > > > statistics are handled. See below, and the comments in dpif- > > > provider.h. > > > > > > > OK. > > > > > > + /* Removes meter 'meter_id' from HW. */ > > > > + int (*meter_del)(ofproto_meter_id meter_id, > > > > + struct ofputil_meter_stats *stats, > > > > + uint16_t max_bands); > > > > + > > > > /* 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 fb108c0d5..41f6e1723 100644 > > > > --- a/lib/netdev-offload.c > > > > +++ b/lib/netdev-offload.c > > > > @@ -195,6 +195,51 @@ netdev_assign_flow_api(struct netdev > > > > *netdev) > > > > return -1; > > > > } > > > > > > > > +int > > > > +meter_offload_set(ofproto_meter_id meter_id, > > > > + struct ofputil_meter_config *config) > > > > +{ > > > > + struct netdev_registered_flow_api *rfa; > > > > + > > > > + CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) { > > > > + if (rfa->flow_api->meter_set) { > > > > + rfa->flow_api->meter_set(meter_id, config); > > > > + } > > > > + } > > > > + > > > > + return 0; > > > > > > We are not reporting any failures here, this does not seem right > > > to > > > me? Same for the other three functions below. > > > > > > > If offload fails, ovs-meter should work as before. So, it's not > > necessary to check the return value, right? > > If this is the design, maybe we should at least add a log message so > we know the initialization has failed, so we know why any flow > inserts would fail with ENOSUPP? Did you mean adding logging when return !ENOSUPP? Another issue is that, do you want to log every time user fails to add or modify a meter parameters? > > > > > +} > > > > + > > > > +int > > > > +meter_offload_get(ofproto_meter_id meter_id, > > > > + struct ofputil_meter_stats *stats, uint16_t > > > > max_bands) > > > > +{ > > > > + struct netdev_registered_flow_api *rfa; > > > > + > > > > + CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) { > > > > + if (rfa->flow_api->meter_get) { > > > > + rfa->flow_api->meter_get(meter_id, stats, > > > > max_bands); > > > > > > The following comments also apply to meter_offload_del() below, > > > and > > > comments on the first comment ;) > > > > > > So here in theory multiple offload APIs can handle the same meter > > > ID, > > > so here we should clear all statistics in the structure and the > > > > > individual callers should ADD their counter, not replace them. > > > > > > > I don't know why the counters are replaced, could you please > > explain? > > Because the API help not cleanly states that meters should add the > value rather than replace, it could be wrongly interpreted. But I > guess you are fixing this. > > > > Or we should pass in a tmp ofputil_meter_stats structure and do > > > this. > > > > > > I think this goes for the stats->packet_in_count, stats- > > > > byte_in_count and band->byte_count and band->packet_count. > > > > You can check in later patch that I added > > packet_in_count/byte_in_count > > by "+=", so the value is not replaced, but added. > > ACK, ignore this part. > > > > > > > > > + } > > > > + } > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +int > > > > +meter_offload_del(ofproto_meter_id meter_id, > > > > + struct ofputil_meter_stats *stats, uint16_t > > > > max_bands) > > > > +{ > > > > + struct netdev_registered_flow_api *rfa; > > > > + > > > > + CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) { > > > > + if (rfa->flow_api->meter_del) { > > > > + rfa->flow_api->meter_del(meter_id, stats, > > > > max_bands); > > > > + } > > > > + } > > > > + > > > > + return 0; > > > > +} > > > > + > > > > int > > > > netdev_flow_flush(struct netdev *netdev) > > > > { > > > > diff --git a/lib/netdev-offload.h b/lib/netdev-offload.h > > > > index 8237a85dd..a6d4ba3cb 100644 > > > > --- a/lib/netdev-offload.h > > > > +++ b/lib/netdev-offload.h > > > > @@ -22,6 +22,7 @@ > > > > #include "openvswitch/types.h" > > > > #include "ovs-rcu.h" > > > > #include "ovs-thread.h" > > > > +#include "openvswitch/ofp-meter.h" > > > > #include "packets.h" > > > > #include "flow.h" > > > > > > > > @@ -158,6 +159,13 @@ int netdev_ports_flow_get(const char > > > > *dpif_type, struct match *match, > > > > int netdev_ports_get_n_flows(const char *dpif_type, > > > > odp_port_t port_no, uint64_t > > > > *n_flows); > > > > > > > > +int meter_offload_set(ofproto_meter_id, > > > > + struct ofputil_meter_config *); > > > > +int meter_offload_get(ofproto_meter_id, > > > > + struct ofputil_meter_stats *, uint16_t); > > > > +int meter_offload_del(ofproto_meter_id, > > > > + struct ofputil_meter_stats *, uint16_t); > > > > + > > > > #ifdef __cplusplus > > > > } > > > > #endif > > > > -- > > > > 2.26.2 > > > > > > > > _______________________________________________ > > > > dev mailing list > > > > dev@openvswitch.org > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 24 May 2022, at 4:05, Jianbo Liu wrote: > On Mon, 2022-05-23 at 11:59 +0200, Eelco Chaudron wrote: >> >> >> On 16 May 2022, at 12:31, Jianbo Liu wrote: >> >>> On Fri, 2022-05-13 at 13:46 +0200, Eelco Chaudron wrote: >>>> >>>> >>>> On 3 May 2022, at 5:08, Jianbo Liu via dev wrote: >>>> >>>>> Add API to offload meter to HW, and the corresponding functions >>>>> to >>>>> call >>>>> the meter callbacks from all the registered flow API providers. >>>>> The interfaces are like those related to meter in dpif_class, >>>>> in >>>>> order >>>>> to pass necessary info to HW. >>>> >>>> See my comments inline below. >>>> >>>>> Signed-off-by: Jianbo Liu <jianbol@nvidia.com> >>>>> --- >>>>> lib/netdev-offload-provider.h | 15 ++++++++++++ >>>>> lib/netdev-offload.c | 45 >>>>> +++++++++++++++++++++++++++++++++++ >>>>> lib/netdev-offload.h | 8 +++++++ >>>>> 3 files changed, 68 insertions(+) >>>>> >>>>> diff --git a/lib/netdev-offload-provider.h b/lib/netdev- >>>>> offload- >>>>> provider.h >>>>> index 8ff2de983..2441534c3 100644 >>>>> --- a/lib/netdev-offload-provider.h >>>>> +++ b/lib/netdev-offload-provider.h >>>>> @@ -94,6 +94,21 @@ struct netdev_flow_api { >>>>> * takes ownership of a packet if errno != EOPNOTSUPP. */ >>>>> int (*hw_miss_packet_recover)(struct netdev *, struct >>>>> dp_packet *); >>>>> >>>>> + /* Offloads the meter or modifies it if exists in HW >>>>> + * with the given 'meter_id' and the configuration in >>>>> 'config'. */ >>>>> + int (*meter_set)(ofproto_meter_id meter_id, >>>>> + struct ofputil_meter_config *config); >>>>> + >>>>> + /* Queries HW for meter stats with the given 'meter_id'. >>>>> */ >>>>> + int (*meter_get)(ofproto_meter_id meter_id, >>>>> + struct ofputil_meter_stats *stats, >>>>> + uint16_t max_bands); >>>>> + >>>> >>>> The comments on meter_del and meter_get need more details on how >>>> statistics are handled. See below, and the comments in dpif- >>>> provider.h. >>>> >>> >>> OK. >>> >>>>> + /* Removes meter 'meter_id' from HW. */ >>>>> + int (*meter_del)(ofproto_meter_id meter_id, >>>>> + struct ofputil_meter_stats *stats, >>>>> + uint16_t max_bands); >>>>> + >>>>> /* 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 fb108c0d5..41f6e1723 100644 >>>>> --- a/lib/netdev-offload.c >>>>> +++ b/lib/netdev-offload.c >>>>> @@ -195,6 +195,51 @@ netdev_assign_flow_api(struct netdev >>>>> *netdev) >>>>> return -1; >>>>> } >>>>> >>>>> +int >>>>> +meter_offload_set(ofproto_meter_id meter_id, >>>>> + struct ofputil_meter_config *config) >>>>> +{ >>>>> + struct netdev_registered_flow_api *rfa; >>>>> + >>>>> + CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) { >>>>> + if (rfa->flow_api->meter_set) { >>>>> + rfa->flow_api->meter_set(meter_id, config); >>>>> + } >>>>> + } >>>>> + >>>>> + return 0; >>>> >>>> We are not reporting any failures here, this does not seem right >>>> to >>>> me? Same for the other three functions below. >>>> >>> >>> If offload fails, ovs-meter should work as before. So, it's not >>> necessary to check the return value, right? >> >> If this is the design, maybe we should at least add a log message so >> we know the initialization has failed, so we know why any flow >> inserts would fail with ENOSUPP? > > Did you mean adding logging when return !ENOSUPP? This part is already there in netdev-offload-tc.c > Another issue is that, do you want to log every time user fails to add > or modify a meter parameters? Yes, that was what I was suggesting so in the above code something like: int meter_offload_set(ofproto_meter_id meter_id, struct ofputil_meter_config *config) { struct netdev_registered_flow_api *rfa; CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) { if (rfa->flow_api->meter_set) { int ret = rfa->flow_api->meter_set(meter_id, config); if (ret) { VLOG_DBG_RL(“Failed setting meter for flow api %s, error %d”, rya->flow_api->type, ret); } } } >>>>> +} >>>>> + >>>>> +int >>>>> +meter_offload_get(ofproto_meter_id meter_id, >>>>> + struct ofputil_meter_stats *stats, uint16_t >>>>> max_bands) >>>>> +{ >>>>> + struct netdev_registered_flow_api *rfa; >>>>> + >>>>> + CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) { >>>>> + if (rfa->flow_api->meter_get) { >>>>> + rfa->flow_api->meter_get(meter_id, stats, >>>>> max_bands); >>>> >>>> The following comments also apply to meter_offload_del() below, >>>> and >>>> comments on the first comment ;) >>>> >>>> So here in theory multiple offload APIs can handle the same meter >>>> ID, >>>> so here we should clear all statistics in the structure and the >>> >>>> individual callers should ADD their counter, not replace them. >>>> >>> >>> I don't know why the counters are replaced, could you please >>> explain? >> >> Because the API help not cleanly states that meters should add the >> value rather than replace, it could be wrongly interpreted. But I >> guess you are fixing this. >> >>>> Or we should pass in a tmp ofputil_meter_stats structure and do >>>> this. >>>> >>>> I think this goes for the stats->packet_in_count, stats- >>>>> byte_in_count and band->byte_count and band->packet_count. >>> >>> You can check in later patch that I added >>> packet_in_count/byte_in_count >>> by "+=", so the value is not replaced, but added. >> >> ACK, ignore this part. >>> >>>> >>>>> + } >>>>> + } >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +int >>>>> +meter_offload_del(ofproto_meter_id meter_id, >>>>> + struct ofputil_meter_stats *stats, uint16_t >>>>> max_bands) >>>>> +{ >>>>> + struct netdev_registered_flow_api *rfa; >>>>> + >>>>> + CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) { >>>>> + if (rfa->flow_api->meter_del) { >>>>> + rfa->flow_api->meter_del(meter_id, stats, >>>>> max_bands); >>>>> + } >>>>> + } >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> int >>>>> netdev_flow_flush(struct netdev *netdev) >>>>> { >>>>> diff --git a/lib/netdev-offload.h b/lib/netdev-offload.h >>>>> index 8237a85dd..a6d4ba3cb 100644 >>>>> --- a/lib/netdev-offload.h >>>>> +++ b/lib/netdev-offload.h >>>>> @@ -22,6 +22,7 @@ >>>>> #include "openvswitch/types.h" >>>>> #include "ovs-rcu.h" >>>>> #include "ovs-thread.h" >>>>> +#include "openvswitch/ofp-meter.h" >>>>> #include "packets.h" >>>>> #include "flow.h" >>>>> >>>>> @@ -158,6 +159,13 @@ int netdev_ports_flow_get(const char >>>>> *dpif_type, struct match *match, >>>>> int netdev_ports_get_n_flows(const char *dpif_type, >>>>> odp_port_t port_no, uint64_t >>>>> *n_flows); >>>>> >>>>> +int meter_offload_set(ofproto_meter_id, >>>>> + struct ofputil_meter_config *); >>>>> +int meter_offload_get(ofproto_meter_id, >>>>> + struct ofputil_meter_stats *, uint16_t); >>>>> +int meter_offload_del(ofproto_meter_id, >>>>> + struct ofputil_meter_stats *, uint16_t); >>>>> + >>>>> #ifdef __cplusplus >>>>> } >>>>> #endif >>>>> -- >>>>> 2.26.2 >>>>> >>>>> _______________________________________________ >>>>> dev mailing list >>>>> dev@openvswitch.org >>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/lib/netdev-offload-provider.h b/lib/netdev-offload-provider.h index 8ff2de983..2441534c3 100644 --- a/lib/netdev-offload-provider.h +++ b/lib/netdev-offload-provider.h @@ -94,6 +94,21 @@ struct netdev_flow_api { * takes ownership of a packet if errno != EOPNOTSUPP. */ int (*hw_miss_packet_recover)(struct netdev *, struct dp_packet *); + /* Offloads the meter or modifies it if exists in HW + * with the given 'meter_id' and the configuration in 'config'. */ + int (*meter_set)(ofproto_meter_id meter_id, + struct ofputil_meter_config *config); + + /* Queries HW for meter stats with the given 'meter_id'. */ + int (*meter_get)(ofproto_meter_id meter_id, + struct ofputil_meter_stats *stats, + uint16_t max_bands); + + /* Removes meter 'meter_id' from HW. */ + int (*meter_del)(ofproto_meter_id meter_id, + struct ofputil_meter_stats *stats, + uint16_t max_bands); + /* 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 fb108c0d5..41f6e1723 100644 --- a/lib/netdev-offload.c +++ b/lib/netdev-offload.c @@ -195,6 +195,51 @@ netdev_assign_flow_api(struct netdev *netdev) return -1; } +int +meter_offload_set(ofproto_meter_id meter_id, + struct ofputil_meter_config *config) +{ + struct netdev_registered_flow_api *rfa; + + CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) { + if (rfa->flow_api->meter_set) { + rfa->flow_api->meter_set(meter_id, config); + } + } + + return 0; +} + +int +meter_offload_get(ofproto_meter_id meter_id, + struct ofputil_meter_stats *stats, uint16_t max_bands) +{ + struct netdev_registered_flow_api *rfa; + + CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) { + if (rfa->flow_api->meter_get) { + rfa->flow_api->meter_get(meter_id, stats, max_bands); + } + } + + return 0; +} + +int +meter_offload_del(ofproto_meter_id meter_id, + struct ofputil_meter_stats *stats, uint16_t max_bands) +{ + struct netdev_registered_flow_api *rfa; + + CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) { + if (rfa->flow_api->meter_del) { + rfa->flow_api->meter_del(meter_id, stats, max_bands); + } + } + + return 0; +} + int netdev_flow_flush(struct netdev *netdev) { diff --git a/lib/netdev-offload.h b/lib/netdev-offload.h index 8237a85dd..a6d4ba3cb 100644 --- a/lib/netdev-offload.h +++ b/lib/netdev-offload.h @@ -22,6 +22,7 @@ #include "openvswitch/types.h" #include "ovs-rcu.h" #include "ovs-thread.h" +#include "openvswitch/ofp-meter.h" #include "packets.h" #include "flow.h" @@ -158,6 +159,13 @@ int netdev_ports_flow_get(const char *dpif_type, struct match *match, int netdev_ports_get_n_flows(const char *dpif_type, odp_port_t port_no, uint64_t *n_flows); +int meter_offload_set(ofproto_meter_id, + struct ofputil_meter_config *); +int meter_offload_get(ofproto_meter_id, + struct ofputil_meter_stats *, uint16_t); +int meter_offload_del(ofproto_meter_id, + struct ofputil_meter_stats *, uint16_t); + #ifdef __cplusplus } #endif
Add API to offload meter to HW, and the corresponding functions to call the meter callbacks from all the registered flow API providers. The interfaces are like those related to meter in dpif_class, in order to pass necessary info to HW. Signed-off-by: Jianbo Liu <jianbol@nvidia.com> --- lib/netdev-offload-provider.h | 15 ++++++++++++ lib/netdev-offload.c | 45 +++++++++++++++++++++++++++++++++++ lib/netdev-offload.h | 8 +++++++ 3 files changed, 68 insertions(+)