On Mon, 2022-06-20 at 12:12 +0200, Eelco Chaudron wrote:
>
> On 27 May 2022, at 11:00, Jianbo Liu 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.
> >
> > Signed-off-by: Jianbo Liu <[email protected]>
> > ---
> > lib/netdev-offload-provider.h | 19 +++++++++++++
> > lib/netdev-offload.c | 51
> > +++++++++++++++++++++++++++++++++++
> > lib/netdev-offload.h | 8 ++++++
> > 3 files changed, 78 insertions(+)
> >
> > diff --git a/lib/netdev-offload-provider.h b/lib/netdev-offload-
> > provider.h
> > index 8ff2de983..c3028606b 100644
> > --- a/lib/netdev-offload-provider.h
> > +++ b/lib/netdev-offload-provider.h
> > @@ -94,6 +94,25 @@ struct netdev_flow_api {
> > * takes ownership of a packet if errno != EOPNOTSUPP. */
> > int (*hw_miss_packet_recover)(struct netdev *, struct dp_packet
> > *);
>
> Your code seems to assume that the stats structure is already
> initialized, this includes setting up all the possible bands, the
> stats->n_bands is also set up and should be used in your code (see also
> my comments on patch 6). If this is true, you can remove the max_band
> variable from all your callback.
>
I think yes, because you can see my last patch. I check the return
value of dpif_netlink_meter_set__() before calling meter_offload_set().
static int
dpif_netlink_meter_set(struct dpif *dpif_, ofproto_meter_id meter_id,
struct ofputil_meter_config *config)
{
....
err = dpif_netlink_meter_set__(dpif_, meter_id, config);
if (!err && netdev_is_flow_api_enabled()) {
meter_offload_set(meter_id, config);
}
return err;
}
In dpif_netlink_meter_set__(), we will check the band type.
So, if no err is returned from dpif_netlink_meter_set__(), we can
asssume that there is a drop band in the meter.
And you are right about the max_band. I will remove it.
> If this is not the case, you should change your patch 6, to use the
> max_bands value and increase/update the n_bands if it's currently not
> set to 1.
>
> > + /* Offloads or modifies the offloaded meter in HW with the
> > given 'meter_id'
> > + * and the configuration in 'config'.
> > + *
> > + * The meter id specified through 'config->meter_id' is
> > ignored. */
> > + int (*meter_set)(ofproto_meter_id meter_id,
> > + struct ofputil_meter_config *config);
> > +
> > + /* Queries HW for meter stats with the given 'meter_id'.
> > + * Store the stats of dropped packets to band 0. */
> > + int (*meter_get)(ofproto_meter_id meter_id,
> > + struct ofputil_meter_stats *stats,
> > + uint16_t max_bands);
> > +
> > + /* Removes meter 'meter_id' from HW. Stores meter statistics
> > if successful.
> > + * 'stats' may be passed in as NULL if no stats are needed. */
> > + int (*meter_del)(ofproto_meter_id meter_id,
> > + struct ofputil_meter_stats *stats,
> > + uint16_t max_bands);
> > +
>
> I was hoping for some more details on the API, stats and return
> values.
> What about the following change?
Yes, I will change as you suggested. Thanks.
>
> diff --git a/lib/netdev-offload-provider.h b/lib/netdev-offload-
> provider.h
> index c3028606b..9d78fd953 100644
> --- a/lib/netdev-offload-provider.h
> +++ b/lib/netdev-offload-provider.h
> @@ -95,20 +95,31 @@ struct netdev_flow_api {
> int (*hw_miss_packet_recover)(struct netdev *, struct dp_packet
> *);
>
> /* Offloads or modifies the offloaded meter in HW with the given
> 'meter_id'
> - * and the configuration in 'config'.
> + * and the configuration in 'config'. On failure, a non-zero
> error code is
> + * returned.
> *
> * The meter id specified through 'config->meter_id' is ignored.
> */
> int (*meter_set)(ofproto_meter_id meter_id,
> struct ofputil_meter_config *config);
>
> - /* Queries HW for meter stats with the given 'meter_id'.
> - * Store the stats of dropped packets to band 0. */
> + /* 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.
> + *
> + * 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 (*meter_get)(ofproto_meter_id meter_id,
> struct ofputil_meter_stats *stats,
> uint16_t max_bands);
>
> - /* Removes meter 'meter_id' from HW. Stores meter statistics if
> successful.
> - * 'stats' may be passed in as NULL if no stats are needed. */
> + /* Removes meter 'meter_id' from HW. Store the stats of dropped
> packets to
> + * band 0. On failure, a non-zero error code is returned.
> + *
> + * 'stats' may be passed in as NULL if no stats are needed, See
> the above
> + * function for additional details on the 'stats' usage. */
> 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..e853b4e2a 100644
> > --- a/lib/netdev-offload.c
> > +++ b/lib/netdev-offload.c
> > @@ -58,6 +58,7 @@
> > VLOG_DEFINE_THIS_MODULE(netdev_offload);
> >
> >
> > +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> > static bool netdev_flow_api_enabled = false;
> >
> > #define DEFAULT_OFFLOAD_THREAD_NB 1
> > @@ -195,6 +196,56 @@ 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) {
> > + int ret = rfa->flow_api->meter_set(meter_id, config);
> > + if (ret) {
> > + VLOG_DBG_RL(&rl,
> > + "Failed setting meter for flow api %s,
> > error %d",
> > + rfa->flow_api->type, ret);
>
> Thanks for adding this, assumed you would be adding it to the other
> two functions also.
Sure, will do. Thanks!
> In addition, I added the meter ID to the message. For example:
> diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c
> index e853b4e2a..104870383 100644
> --- a/lib/netdev-offload.c
> +++ b/lib/netdev-offload.c
> @@ -206,9 +206,9 @@ meter_offload_set(ofproto_meter_id meter_id,
> if (rfa->flow_api->meter_set) {
> int ret = rfa->flow_api->meter_set(meter_id, config);
> if (ret) {
> - VLOG_DBG_RL(&rl,
> - "Failed setting meter for flow api %s,
> error %d",
> - rfa->flow_api->type, ret);
> + VLOG_DBG_RL(&rl, "Failed setting meter %u for flow
> api %s, "
> + "error %d", meter_id.uint32, rfa-
> >flow_api->type,
> + ret);
> }
> }
> }
> @@ -224,7 +224,12 @@ meter_offload_get(ofproto_meter_id meter_id,
>
> 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);
> + int ret = rfa->flow_api->meter_get(meter_id, stats,
> max_bands);
> + if (ret) {
> + VLOG_DBG_RL(&rl, "Failed getting meter %u for flow
> api %s, "
> + "error %d", meter_id.uint32, rfa-
> >flow_api->type,
> + ret);
> + }
> }
> }
>
> @@ -239,7 +244,12 @@ meter_offload_del(ofproto_meter_id meter_id,
>
> 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);
> + int ret = rfa->flow_api->meter_del(meter_id, stats,
> max_bands);
> + if (ret) {
> + VLOG_DBG_RL(&rl, "Failed deleting meter %u for flow
> api %s, "
> + "error %d", meter_id.uint32, rfa-
> >flow_api->type,
> + ret);
> + }
> }
> }
>
>
> > + }
> > + }
> > + }
> > +
> > + 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
> > --
> > 2.26.2
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
