On 27 Jun 2022, at 12:00, Jianbo Liu wrote:
> 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. Good point, I should think better before sending a quick reply ;) <SNIP> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
