Re: [patch iproute2/net-next v2] tc: m_action: introduce support for hw stats type

[Jiri Pirko <jiri@resnulli.us>]

Wed, Mar 11, 2020 at 12:44:42AM CET, dsahern@gmail.com wrote:
>On 3/9/20 9:54 AM, Jiri Pirko wrote:
>> @@ -24,6 +25,27 @@ enum {
>>  					 * actions stats.
>>  					 */
>>  
>> +/* tca HW stats type
>> + * When user does not pass the attribute, he does not care.
>> + * It is the same as if he would pass the attribute with
>> + * all supported bits set.
>> + * In case no bits are set, user is not interested in getting any HW statistics.
>> + */
>> +#define TCA_ACT_HW_STATS_TYPE_IMMEDIATE (1 << 0) /* Means that in dump, user
>> +						  * gets the current HW stats
>> +						  * state from the device
>> +						  * queried at the dump time.
>> +						  */
>> +#define TCA_ACT_HW_STATS_TYPE_DELAYED (1 << 1) /* Means that in dump, user gets
>
>really long names for attributes.
>
>
>> +static void print_hw_stats(const struct rtattr *arg)
>> +{
>> +	struct nla_bitfield32 *hw_stats_type_bf = RTA_DATA(arg);
>> +	__u8 hw_stats_type;
>> +	int i;
>> +
>> +	hw_stats_type = hw_stats_type_bf->value & hw_stats_type_bf->selector;
>> +	print_string(PRINT_FP, NULL, "\t", NULL);
>> +	open_json_array(PRINT_ANY, "hw_stats");
>> +
>> +	for (i = 0; i < ARRAY_SIZE(hw_stats_type_items); i++) {
>> +		const struct hw_stats_type_item *item;
>> +
>> +		item = &hw_stats_type_items[i];
>> +		if ((!hw_stats_type && !item->type) ||
>> +		    hw_stats_type & item->type)
>> +			print_string(PRINT_ANY, NULL, " %s", item->str);
>
>the stats type can be both delayed and immediate?

Yes, if offloaded to 2 drivers, each supporting different stats.


>
>> +	}
>> +	close_json_array(PRINT_JSON, NULL);
>> +}
>> +
>> +static int parse_hw_stats(const char *str, struct nlmsghdr *n)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(hw_stats_type_items); i++) {
>> +		const struct hw_stats_type_item *item;
>> +
>> +		item = &hw_stats_type_items[i];
>> +		if (matches(str, item->str) == 0) {
>> +			struct nla_bitfield32 hw_stats_type_bf =
>> +					{ item->type,
>> +					  item->type };
>> +			addattr_l(n, MAX_MSG, TCA_ACT_HW_STATS_TYPE,
>> +				  &hw_stats_type_bf, sizeof(hw_stats_type_bf));
>
>that is not human friendly. how about something like:
>
>                        struct nla_bitfield32 hw_stats_type_bf = {
>                                .value    = item->type,
>                                .selector = item->type
>                        };

Okay.



>
>with a line between the declaration and code.
>
>and "disabled" corresponds to 0 which does not align with a
>TCA_ACT_HW_STATS_TYPE_  so why send it

It is a bitfield. So 0 means no bit is set, therefore disabled.