On Mon, 2022-06-27 at 11:33 +0200, Eelco Chaudron wrote:
>
>
> On 27 Jun 2022, at 11:32, Jianbo Liu wrote:
>
> > On Mon, 2022-06-27 at 11:03 +0200, Eelco Chaudron wrote:
> > >
> > >
> > > On 21 Jun 2022, at 5:29, Jianbo Liu wrote:
> > >
> > > > On Mon, 2022-06-20 at 12:15 +0200, Eelco Chaudron wrote:
> > > > > On 27 May 2022, at 11:00, Jianbo Liu wrote:
> > > > >
> > > > > > For dpif-netlink, meters are mapped by tc to police actions
> > > > > > with
> > > > > > one-to-one relationship. Implement meter offload API to
> > > > > > set/get/del
> > > > > > the police action, and a hmap is used to save the mappings.
> > > > > > An id-pool is used to manage all the available police
> > > > > > indexes,
> > > > > > which
> > > > > > are 0x10000000-0x1fffffff, reserved only for OVS.
> > > > > >
> > > > > > Signed-off-by: Jianbo Liu <[email protected]>
> > > > > > ---
> > >
> > > <SNIP>
> > >
> > > > > > +
> > > > > > +static int
> > > > > > +meter_tc_set_policer(ofproto_meter_id meter_id,
> > > > > > + struct ofputil_meter_config *config)
> > > > > > +{
> > > > > > + uint32_t police_index;
> > > > > > + uint32_t rate, burst;
> > > > > > + bool add_policer;
> > > > > > + int err;
> > > > > > +
> > > > > > + ovs_assert(config->bands != NULL);
> > > > >
> > > > > I would remove the assert and just return and error if bands
> > > > > ==
> > > > > NULL
> > > > > or n_bands is < 1.
> > > > > So:
> > > > >
> > > > > if (!config->bands || config->n_bands < 1) {
> > > > > return EINVAL;
> > > > > }
> > > > >
> > > >
> > > > I think maybe not necessary, as dpif_netlink_meter_set__() is
> > > > called
> > > > before, and it doesn't check if bands is not NULL and n_bands
> > > > >= 1.
> > >
> > > You are right it’s assuming it’s non-null and walks through the
> > > n_bands.
> > >
> > > So maybe you should just ignore if the bands is not 1, so
> > > something
> > > like:
> > >
> > > if (!config->bands || config->n_bands < 1) {
> > > return 0;
> > > }
> > >
> > > The dpif_netlink_meter_set__() also check is the band type is
> > > OVS_METER_BAND_TYPE_DROP. Not sure if you need to check this also
> > > when setting the stats, as now your code assumes this.
> >
> > How about adding one more check?
> > if (!config->bands || config->n_bands < 1
> > || config->bands[0].type != OFPMBT13_DROP) {
> > return 0;
> > }
>
> That will work, make sure you also update your get stats function
> with a similar check.
>
No need for get stats and del functions, as they will try to find the
police index before calling TC funcs. So if meter_set return directly
(or fail), there is not police action mapped for this meter, and thus
no TC stats/del funcs will be called.
> > >
> > > > > > + rate = config->bands[0].rate;
> > > > > > + if (config->flags & OFPMF13_BURST) {
> > > > > > + burst = config->bands[0].burst_size;
> > > > > > + } else {
> > > > > > + burst = config->bands[0].rate;
> > > > > > + }
> > >
> > > <SNIP>
> > >
> > > > > > +static int
> > > > > > +meter_tc_del_policer(ofproto_meter_id meter_id,
> > > > > > + struct ofputil_meter_stats *stats,
> > > > > > + uint16_t max_bands OVS_UNUSED)
> > > > > > +{
> > > > > > + uint32_t police_index;
> > > > > > + int err = ENOENT;
> > > > > > +
> > > > > > + if (!meter_id_lookup(meter_id.uint32, &police_index))
> > > > > > {
> > > > > > + err = tc_del_policer_action(police_index, stats);
> > > > > > + if (err && err != ENOENT) {
> > > > > > + VLOG_WARN_RL(&error_rl,
> > > > > > + "Failed to del police %u for
> > > > > > meter
> > > > > > %u:
> > > > > > %s",
> > > > > > + police_index, meter_id.uint32,
> > > > > > ovs_strerror(err));
> > > > >
> > > > > Why do we not cleanup the meter id from the pool on TC
> > > > > failure?
> > > > > Do we
> > > > > assume the entry exists and it was not cleaned up correctly?
> > > > >
> > > >
> > > > Just in case, I don't think it's possible becausue the reserved
> > > > police
> > > > index is big enough, and no other application will use them.
> > >
> > > Yes, we should have enough IDs, was just curious why. Did you
> > > encounter this when testing?
> >
> > No, I didn't.
> >
> > >
> > > > > If this is the case, this should be an error log, not a
> > > > > warning,
> > > > > as
> > > > > something is wrong in this environment.
> > > >
> > > > OK, I will add an error log.
> > >
> > > Thanks, this will at least trigger some attention to a potential
> > > problem.
> > >
> > > > >
> > > > > > + } else {
> > > > > > + meter_free_police_index(police_index);
> > > > > > + }
> > > > > > + meter_id_remove(meter_id.uint32);
> > > > > > + }
> > > > > > +
> > > > > > + return err;
> > > > > > +}
> > > > > > +
> > > > > > const struct netdev_flow_api netdev_offload_tc = {
> > > > > > .type = "linux_tc",
> > > > > > .flow_flush = netdev_tc_flow_flush,
> > > > > > @@ -2312,5 +2512,8 @@ const struct netdev_flow_api
> > > > > > netdev_offload_tc = {
> > > > > > .flow_get = netdev_tc_flow_get,
> > > > > > .flow_del = netdev_tc_flow_del,
> > > > > > .flow_get_n_flows = netdev_tc_get_n_flows,
> > > > > > + .meter_set = meter_tc_set_policer,
> > > > > > + .meter_get = meter_tc_get_policer,
> > > > > > + .meter_del = meter_tc_del_policer,
> > > > > > .init_flow_api = netdev_tc_init_flow_api,
> > > > > > };
> > > > > > --
> > > > > > 2.26.2
> > > > >
> > >
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
