Re: [patch net-next v2 6/9] switchdev: add basic support for flow matching and actions

[Florian Fainelli <f.fainelli@gmail.com>]

On 09/19/14 06:49, Jiri Pirko wrote:
> This patch adds basic support for flows. The infrastructure is prepared
> to easily add another flow matching types. So far, only the key one is
> implemented.
>
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> ---

[snip]

>
> +struct swdev_flow_match_key {
> +	struct {
> +		u32	priority;	/* Packet QoS priority. */
> +		u32	in_port_ifindex; /* Input switch port ifindex (or 0). */
> +	} phy;
> +	struct {
> +		u8     src[ETH_ALEN];	/* Ethernet source address. */
> +		u8     dst[ETH_ALEN];	/* Ethernet destination address. */
> +		__be16 tci;		/* 0 if no VLAN, VLAN_TAG_PRESENT set otherwise. */

Humm, how about QinQ here? I would provision two more 16 bits fields so 
we can do all sorts of VLAN matching.

You might want to allow for a 4 to 8 bytes hardware switch tag as well.

> +		__be16 type;		/* Ethernet frame type. */
> +	} eth;
> +	struct {
> +		u8     proto;		/* IP protocol or lower 8 bits of ARP opcode. */
> +		u8     tos;		/* IP ToS. */
> +		u8     ttl;		/* IP TTL/hop limit. */
> +		u8     frag;		/* One of OVS_FRAG_TYPE_*. */

Options might be missing?

[snip]

> +
> +static void print_flow(const struct swdev_flow *flow, struct net_device *dev,
> +		       const char *comment)
> +{
> +	pr_debug("%s flow %s:\n", dev->name, comment);
> +	print_flow_match(&flow->match);
> +	print_flow_actions(flow->action, flow->action_count);
> +}

I am really not sure how much of this valuable besides early (as in, 
right now) debugging, don't we rather want a generic way to dump a given 
flow under a its native netlink format, does that code has to be here in 
the first place?
--
Florian