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 <[email protected]>
> > ---
> > 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
> > [email protected]
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
