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;
}
>
> > > > + 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
