Re: [PATCH net-next v2 0/4] Implement classifier-action terse dump mode

[Vlad Buslov <vladbu@mellanox.com>]

On Tue 19 May 2020 at 21:58, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Tue, May 19, 2020 at 2:04 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>> I considered that approach initially but decided against it for
>> following reasons:
>>
>> - Generic data is covered by current terse dump implementation.
>>   Everything else will be act or cls specific which would result long
>>   list of flag values like: TCA_DUMP_FLOWER_KEY_ETH_DST,
>>   TCA_DUMP_FLOWER_KEY_ETH_DST, TCA_DUMP_FLOWER_KEY_VLAN_ID, ...,
>>   TCA_DUMP_TUNNEL_KEY_ENC_KEY_ID, TCA_DUMP_TUNNEL_KEY_ENC_TOS. All of
>>   these would require a lot of dedicated logic in act and cls dump
>>   callbacks. Also, it would be quite a challenge to test all possible
>>   combinations.
>
> Well, if you consider netlink dump as a database query, what Edward
> proposed is merely "select COLUMN1 COLUMN2 from cls_db" rather
> than "select * from cls_db".
>
> No one said it is easy to implement, it is just more elegant than you
> select a hardcoded set of columns for the user.

As I explained to Edward, having denser netlink packets with more
filters per packet is only part of optimization. Another part is not
executing some code at all. Consider fl_dump_key() which is 200 lines
function with bunch of conditionals like that:

static int fl_dump_key(struct sk_buff *skb, struct net *net,
		       struct fl_flow_key *key, struct fl_flow_key *mask)
{
	if (mask->meta.ingress_ifindex) {
		struct net_device *dev;

		dev = __dev_get_by_index(net, key->meta.ingress_ifindex);
		if (dev && nla_put_string(skb, TCA_FLOWER_INDEV, dev->name))
			goto nla_put_failure;
	}

	if (fl_dump_key_val(skb, key->eth.dst, TCA_FLOWER_KEY_ETH_DST,
			    mask->eth.dst, TCA_FLOWER_KEY_ETH_DST_MASK,
			    sizeof(key->eth.dst)) ||
	    fl_dump_key_val(skb, key->eth.src, TCA_FLOWER_KEY_ETH_SRC,
			    mask->eth.src, TCA_FLOWER_KEY_ETH_SRC_MASK,
			    sizeof(key->eth.src)) ||
	    fl_dump_key_val(skb, &key->basic.n_proto, TCA_FLOWER_KEY_ETH_TYPE,
			    &mask->basic.n_proto, TCA_FLOWER_UNSPEC,
			    sizeof(key->basic.n_proto)))
		goto nla_put_failure;

	if (fl_dump_key_mpls(skb, &key->mpls, &mask->mpls))
		goto nla_put_failure;

	if (fl_dump_key_vlan(skb, TCA_FLOWER_KEY_VLAN_ID,
			     TCA_FLOWER_KEY_VLAN_PRIO, &key->vlan, &mask->vlan))
		goto nla_put_failure;
    ...


Now imagine all of these are extended with additional if (flags &
TCA_DUMP_XXX). All gains from not outputting some other minor stuff into
netlink packet will be negated by it.


>
> Think about it, what if another user wants a less terse dump but still
> not a full dump? Would you implement ops->terse_dump2()? Or
> what if people still think your terse dump is not as terse as she wants?
> ops->mini_dump()? How many ops's we would end having?

User can discard whatever he doesn't need in user land code. The goal of
this change is performance optimization, not designing a generic
kernel-space data filtering mechanism.

>
>
>>
>> - It is hard to come up with proper validation for such implementation.
>>   In case of terse dump I just return an error if classifier doesn't
>>   implement the callback (and since current implementation only outputs
>>   generic action info, it doesn't even require support from
>>   action-specific dump callbacks). But, for example, how do we validate
>>   a case where user sets some flower and tunnel_key act dump flags from
>>   previous paragraph, but Qdisc contains some other classifier? Or
>>   flower classifier points to other types of actions? Or when flower
>>   classifier has and tunnel_key actions but also mirred? Should the
>
> Each action should be able to dump selectively too. If you think it
> as a database, it is just a different table with different schemas.

How is designing custom SQL-like query language (according to your
example at the beginning of the mail) for filter dump is going to
improve performance? If there is a way to do it in fast a generic manner
with BPF, then I'm very interested to hear the details. But adding
hundred more hardcoded conditionals is just not a solution considering
main motivations for this change is performance.