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 <jian...@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
> > > > d...@openvswitch.org
> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
