Re: [dpdk-dev] [PATCH] ethdev: add function to print a flow

[Ori Kam <orika@mellanox.com>]

General comment for the discussion.

I think it is important that all fields of all actions and items will be printed.
This means that any modification to the rte_flow should also reflect
in this function.

Best,
Ori



> -----Original Message-----
> From: Asaf Penso
> Sent: Tuesday, December 11, 2018 9:25 AM
> To: Stephen Hemminger <stephen@networkplumber.org>
> Cc: Adrien Mazarguil <adrien.mazarguil@6wind.com>; dev@dpdk.org; Shahaf
> Shuler <shahafs@mellanox.com>; Ori Kam <orika@mellanox.com>; Thomas
> Monjalon <thomas@monjalon.net>
> Subject: RE: [dpdk-dev] [PATCH] ethdev: add function to print a flow
> 
> 
> 
> Regards,
> Asaf Penso
> 
> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Thursday, December 6, 2018 11:44 PM
> > To: Asaf Penso <asafp@mellanox.com>
> > Cc: Adrien Mazarguil <adrien.mazarguil@6wind.com>; dev@dpdk.org;
> > Shahaf Shuler <shahafs@mellanox.com>; Ori Kam <orika@mellanox.com>;
> > Thomas Monjalon <thomas@monjalon.net>
> > Subject: Re: [dpdk-dev] [PATCH] ethdev: add function to print a flow
> >
> > On Wed, 28 Nov 2018 15:26:06 +0000
> > Asaf Penso <asafp@mellanox.com> wrote:
> >
> > > Flow contains the following information: port id, attributes, patterns
> > > and actions.
> > > The function rte_flow_print prints all above information.
> > >
> > > It can be used for debugging purposes to validate the behavior of
> > > different dpdk applications.
> > >
> > > Example: running testpmd with the following flow create:
> > > flow create 1 transfer ingress
> > > pattern eth src is 52:54:00:15:b1:b1 dst is 24:8A:07:8D:AE:C6 / end
> > > actions of_push_vlan ethertype 0x8100 / of_set_vlan_vid vlan_vid 0x888
> > > / of_set_vlan_pcp vlan_pcp 7 / port_id id 0 / end Will result in this
> > > output:
> > > Print flow info
> > > port_id  =1
> > > group    =0
> > > priority =0
> > > ingress  =1
> > > egress   =0
> > > transfer =1
> > > group    =0
> > > reserved =0
> > > pattern type  =9
> > >  name=RTE_FLOW_ITEM_TYPE_ETH
> > >   spec type=0x0, src=52:54:00:15:b1:b1, dst=24:8a:07:8d:ae:c6
> > >   mask type=0x0, src=ff:ff:ff:ff:ff:ff, dst=ff:ff:ff:ff:ff:ff actions
> > > type  =23  name=RTE_FLOW_ACTION_TYPE_OF_PUSH_VLAN
> > >   ethertype=0x81
> > > actions type  =24
> > >  name=RTE_FLOW_ACTION_TYPE_OF_SET_VLAN_VID
> > >   vlan_vid=0x8808
> > > actions type  =25
> > >  name=RTE_FLOW_ACTION_TYPE_OF_SET_VLAN_PCP
> > >   vlan_pcp=7
> > > actions type  =13
> > >  name=RTE_FLOW_ACTION_TYPE_PORT_ID
> > >   id=0
> > >   reserved=0
> > >
> > > Signed-off-by: Asaf Penso <asafp@mellanox.com>
> > > ---
> > >  lib/librte_ethdev/rte_ethdev_version.map |    1 +
> > >  lib/librte_ethdev/rte_flow.c             |  226
> > ++++++++++++++++++++++++++++++
> > >  lib/librte_ethdev/rte_flow.h             |   31 ++++
> > >  3 files changed, 258 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/lib/librte_ethdev/rte_ethdev_version.map
> > > b/lib/librte_ethdev/rte_ethdev_version.map
> > > index 92ac3de..7676983 100644
> > > --- a/lib/librte_ethdev/rte_ethdev_version.map
> > > +++ b/lib/librte_ethdev/rte_ethdev_version.map
> > > @@ -249,6 +249,7 @@ EXPERIMENTAL {
> > >  	rte_eth_switch_domain_free;
> > >  	rte_flow_conv;
> > >  	rte_flow_expand_rss;
> > > +	rte_flow_print;
> > >  	rte_mtr_capabilities_get;
> > >  	rte_mtr_create;
> > >  	rte_mtr_destroy;
> > > diff --git a/lib/librte_ethdev/rte_flow.c
> > > b/lib/librte_ethdev/rte_flow.c index 3277be1..742d892 100644
> > > --- a/lib/librte_ethdev/rte_flow.c
> > > +++ b/lib/librte_ethdev/rte_flow.c
> > > @@ -16,6 +16,8 @@
> > >  #include "rte_flow_driver.h"
> > >  #include "rte_flow.h"
> > >
> > > +int rte_flow_logtype;
> > > +
> > >  /**
> > >   * Flow elements description tables.
> > >   */
> > > @@ -202,6 +204,222 @@ struct rte_flow_desc_data {
> > >  				  NULL, rte_strerror(ENOSYS));
> > >  }
> > >
> > > +/* Example:
> > > + *
> > > + * struct eth_addr mac;
> > > + *   [...]
> > > + * printf("The Ethernet address is "RTE_ETH_ADDR_FMT"\n",
> > > + *	RTE_ETH_ADDR_ARGS(mac));
> > > + *
> > > + */
> > > +#define RTE_ETH_ADDR_FMT \
> > > +
> > 	"%02"PRIx8":%02"PRIx8":%02"PRIx8":%02"PRIx8":%02"PRIx8":%02"P
> > RIx8
> > > +#define RTE_ETH_ADDR_ARGS(addr)
> > RTE_ETH_ADDR_BYTES_ARGS((addr).ea)
> > > +#define RTE_ETH_ADDR_BYTES_ARGS(bytes) \
> > > +	(bytes)[0], (bytes)[1], (bytes)[2], (bytes)[3], (bytes)[4],
> > > +(bytes)[5]
> > > +
> > > +/* Print the flow fields. */
> > > +void __rte_experimental
> > > +rte_flow_print(uint16_t port_id,
> > > +		const struct rte_flow_attr *attr,
> > > +		const struct rte_flow_item pattern[],
> > > +		const struct rte_flow_action actions[]) {
> > > +	RTE_FLOW_LOG(INFO, "Print flow info\n");
> > > +	RTE_FLOW_LOG(INFO, "port_id  =%u\n", port_id);
> > > +	RTE_FLOW_LOG(INFO, "group    =%u\n", attr->group);
> > > +	RTE_FLOW_LOG(INFO, "priority =%u\n", attr->priority);
> > > +	RTE_FLOW_LOG(INFO, "ingress  =%u\n", attr->ingress);
> > > +	RTE_FLOW_LOG(INFO, "egress   =%u\n", attr->egress);
> > > +	RTE_FLOW_LOG(INFO, "transfer =%u\n", attr->transfer);
> > > +	RTE_FLOW_LOG(INFO, "group    =%u\n", attr->group);
> > > +	RTE_FLOW_LOG(INFO, "reserved =%u\n", attr->reserved);
> > > +
> > > +	for (; pattern->type != RTE_FLOW_ITEM_TYPE_END; pattern++) {
> > > +		RTE_FLOW_LOG(INFO, "pattern type  =%u\n", pattern-
> > >type);
> > > +		switch (pattern->type) {
> > > +		case RTE_FLOW_ITEM_TYPE_ETH: {
> > > +			RTE_FLOW_LOG(INFO, "
> > name=RTE_FLOW_ITEM_TYPE_ETH\n");
> > > +			if (pattern->spec) {
> > > +				const struct rte_flow_item_eth *spec =
> > > +					pattern->spec;
> > > +				RTE_FLOW_LOG(INFO, "  spec type=0x%x, "
> > > +				"src="RTE_ETH_ADDR_FMT",
> > dst="RTE_ETH_ADDR_FMT
> > > +				"\n",
> > > +				spec->type,
> > > +				RTE_ETH_ADDR_BYTES_ARGS(spec-
> > >src.addr_bytes),
> > > +				RTE_ETH_ADDR_BYTES_ARGS(spec-
> > >dst.addr_bytes));
> > > +			}
> > > +
> > > +			if (pattern->mask) {
> > > +				const struct rte_flow_item_eth *mask =
> > > +					pattern->mask;
> > > +				RTE_FLOW_LOG(INFO, "  mask type=0x%x, "
> > > +				"src="RTE_ETH_ADDR_FMT",
> > dst="RTE_ETH_ADDR_FMT
> > > +				"\n",
> > > +				mask->type,
> > > +				RTE_ETH_ADDR_BYTES_ARGS(mask-
> > >src.addr_bytes),
> > > +				RTE_ETH_ADDR_BYTES_ARGS(mask-
> > >dst.addr_bytes));
> > > +			}
> > > +			break;
> > > +		}
> > > +		case RTE_FLOW_ITEM_TYPE_VLAN: {
> > > +			RTE_FLOW_LOG(INFO, "
> > name=RTE_FLOW_ITEM_TYPE_VLAN\n");
> > > +			if (pattern->spec) {
> > > +				const struct rte_flow_item_vlan *spec =
> > > +					pattern->spec;
> > > +				RTE_FLOW_LOG(INFO, "  spec tci=0x%x\n",
> > > +					spec->tci);
> > > +				RTE_FLOW_LOG(INFO, "  spec
> > inner_type=%u\n",
> > > +					spec->inner_type);
> > > +			}
> > > +			if (pattern->mask) {
> > > +				const struct rte_flow_item_vlan *mask =
> > > +					pattern->mask;
> > > +				RTE_FLOW_LOG(INFO, "  mask tci=0x%x\n",
> > > +					mask->tci);
> > > +				RTE_FLOW_LOG(INFO, "  mask
> > inner_type=%u\n",
> > > +					mask->inner_type);
> > > +			}
> > > +			break;
> > > +		}
> > > +		case RTE_FLOW_ITEM_TYPE_IPV4: {
> > > +			RTE_FLOW_LOG(INFO, "
> > name=RTE_FLOW_ITEM_TYPE_IPV4\n");
> > > +			if (pattern->spec) {
> > > +				const struct rte_flow_item_ipv4 *spec =
> > > +					pattern->spec;
> > > +				RTE_FLOW_LOG(INFO, "  spec
> > version_ihl=%u\n",
> > > +						spec->hdr.version_ihl);
> > > +				RTE_FLOW_LOG(INFO, "  spec
> > type_of_service=%u\n",
> > > +						spec->hdr.type_of_service);
> > > +				RTE_FLOW_LOG(INFO, "  spec
> > total_length=%u\n",
> > > +						spec->hdr.total_length);
> > > +				RTE_FLOW_LOG(INFO, "  spec
> > packet_id=%u\n",
> > > +						spec->hdr.packet_id);
> > > +				RTE_FLOW_LOG(INFO, "  spec
> > fragment_offset=%u\n",
> > > +						spec->hdr.fragment_offset);
> > > +				RTE_FLOW_LOG(INFO, "  spec
> > time_to_live=%u\n",
> > > +						spec->hdr.time_to_live);
> > > +				RTE_FLOW_LOG(INFO, "  spec
> > next_proto_id=%u\n",
> > > +						spec->hdr.next_proto_id);
> > > +				RTE_FLOW_LOG(INFO, "  spec
> > hdr_checksum=0x%x\n",
> > > +						spec->hdr.hdr_checksum);
> > > +				RTE_FLOW_LOG(INFO,
> > > +					"  spec src_addr=%d.%d.%d.%d\n",
> > > +					(spec->hdr.src_addr & 0x000000FF),
> > > +					(spec->hdr.src_addr & 0x0000FF00)
> > >>  8,
> > > +					(spec->hdr.src_addr & 0x00FF0000)
> > >> 16,
> > > +					(spec->hdr.src_addr & 0xFF000000)
> > > +						>> 24);
> > > +				RTE_FLOW_LOG(INFO,
> > > +					"  spec dst_addr=%d.%d.%d.%d\n",
> > > +					(spec->hdr.dst_addr & 0x000000FF),
> > > +					(spec->hdr.dst_addr & 0x0000FF00)
> > >>  8,
> > > +					(spec->hdr.dst_addr & 0x00FF0000)
> > >> 16,
> > > +					(spec->hdr.dst_addr & 0xFF000000)
> > > +						>> 24);
> > > +			}
> > > +
> > > +			if (pattern->mask) {
> > > +				const struct rte_flow_item_ipv4 *mask =
> > > +					pattern->mask;
> > > +				RTE_FLOW_LOG(INFO, "  mask
> > version_ihl=%u\n",
> > > +					mask->hdr.version_ihl);
> > > +				RTE_FLOW_LOG(INFO, "  mask
> > type_of_service=%u\n",
> > > +						mask->hdr.type_of_service);
> > > +				RTE_FLOW_LOG(INFO, "  mask
> > total_length=%u\n",
> > > +						mask->hdr.total_length);
> > > +				RTE_FLOW_LOG(INFO, "  mask
> > packet_id=%u\n",
> > > +						mask->hdr.packet_id);
> > > +				RTE_FLOW_LOG(INFO, "  mask
> > fragment_offset=%u\n",
> > > +						mask->hdr.fragment_offset);
> > > +				RTE_FLOW_LOG(INFO, "  mask
> > time_to_live=%u\n",
> > > +						mask->hdr.time_to_live);
> > > +				RTE_FLOW_LOG(INFO, "  mask
> > next_proto_id=%u\n",
> > > +						mask->hdr.next_proto_id);
> > > +				RTE_FLOW_LOG(INFO, "  mask
> > hdr_checksum=0x%x\n",
> > > +						mask->hdr.hdr_checksum);
> > > +				RTE_FLOW_LOG(INFO,
> > > +					"  mask src_addr=%d.%d.%d.%d\n",
> > > +					(mask->hdr.src_addr & 0x000000FF),
> > > +					(mask->hdr.src_addr & 0x0000FF00)
> > >>  8,
> > > +					(mask->hdr.src_addr & 0x00FF0000)
> > >> 16,
> > > +					(mask->hdr.src_addr & 0xFF000000)
> > > +						>> 24);
> > > +				RTE_FLOW_LOG(INFO,
> > > +					"  mask dst_addr=%d.%d.%d.%d\n",
> > > +					(mask->hdr.dst_addr & 0x000000FF),
> > > +					(mask->hdr.dst_addr & 0x0000FF00)
> > >>  8,
> > > +					(mask->hdr.dst_addr & 0x00FF0000)
> > >> 16,
> > > +					(mask->hdr.dst_addr & 0xFF000000)
> > > +						>> 24);
> > > +			}
> > > +			break;
> > > +		}
> > > +		default:
> > > +			RTE_FLOW_LOG(INFO, "unfamiliar pattern item\n");
> > > +		}
> > > +	}
> > > +
> > > +	for (; actions->type != RTE_FLOW_ACTION_TYPE_END; actions++) {
> > > +		RTE_FLOW_LOG(INFO, "actions type  =%u\n", actions-
> > >type);
> > > +		switch (actions->type) {
> > > +		case RTE_FLOW_ACTION_TYPE_OF_POP_VLAN: {
> > > +			RTE_FLOW_LOG(INFO,
> > > +				"
> > name=RTE_FLOW_ACTION_TYPE_OF_POP_VLAN\n");
> > > +			break;
> > > +		}
> > > +		case RTE_FLOW_ACTION_TYPE_OF_PUSH_VLAN: {
> > > +			RTE_FLOW_LOG(INFO,
> > > +				"
> > name=RTE_FLOW_ACTION_TYPE_OF_PUSH_VLAN\n");
> > > +			if (actions->conf) {
> > > +				const struct rte_flow_action_of_push_vlan
> > > +					*conf = actions->conf;
> > > +				RTE_FLOW_LOG(INFO, "
> > ethertype=0x%x\n",
> > > +					conf->ethertype);
> > > +			}
> > > +			break;
> > > +		}
> > > +		case RTE_FLOW_ACTION_TYPE_OF_SET_VLAN_VID: {
> > > +			RTE_FLOW_LOG(INFO,
> > > +				"
> > name=RTE_FLOW_ACTION_TYPE_OF_SET_VLAN_VID\n");
> > > +			if (actions->conf) {
> > > +				const struct
> > rte_flow_action_of_set_vlan_vid
> > > +					*conf = actions->conf;
> > > +				RTE_FLOW_LOG(INFO, "  vlan_vid=0x%x\n",
> > > +					conf->vlan_vid);
> > > +			}
> > > +			break;
> > > +		}
> > > +		case RTE_FLOW_ACTION_TYPE_OF_SET_VLAN_PCP: {
> > > +			RTE_FLOW_LOG(INFO,
> > > +				"
> > name=RTE_FLOW_ACTION_TYPE_OF_SET_VLAN_PCP\n");
> > > +			if (actions->conf) {
> > > +				const struct
> > rte_flow_action_of_set_vlan_pcp
> > > +					*conf = actions->conf;
> > > +				RTE_FLOW_LOG(INFO, "  vlan_pcp=%u\n",
> > > +					conf->vlan_pcp);
> > > +			}
> > > +			break;
> > > +		}
> > > +		case RTE_FLOW_ACTION_TYPE_PORT_ID: {
> > > +			RTE_FLOW_LOG(INFO,
> > > +				"
> > name=RTE_FLOW_ACTION_TYPE_PORT_ID\n");
> > > +			if (actions->conf) {
> > > +				const struct rte_flow_action_port_id
> > > +					*conf = actions->conf;
> > > +				RTE_FLOW_LOG(INFO, "  id=%u\n", conf-
> > >id);
> > > +				RTE_FLOW_LOG(INFO, "  reserved=%u\n",
> > > +					conf->reserved);
> > > +			}
> > > +			break;
> > > +		}
> > > +		default:
> > > +			RTE_FLOW_LOG(INFO, "unfamiliar action item\n");
> > > +		}
> > > +	}
> > > +}
> > > +
> > >  /* Create a flow rule on a given port. */  struct rte_flow *
> > > rte_flow_create(uint16_t port_id, @@ -1001,3 +1219,11 @@ enum
> > > rte_flow_conv_item_spec_type {
> > >  	};
> > >  	return lsize;
> > >  }
> > > +
> > > +RTE_INIT(flow_init_log)
> > > +{
> > > +	rte_flow_logtype = rte_log_register("lib.flow");
> > > +	if (rte_flow_logtype >= 0)
> > > +		rte_log_set_level(rte_flow_logtype, RTE_LOG_INFO); }
> > > +
> > > diff --git a/lib/librte_ethdev/rte_flow.h
> > > b/lib/librte_ethdev/rte_flow.h index c0fe879..0a7e70b 100644
> > > --- a/lib/librte_ethdev/rte_flow.h
> > > +++ b/lib/librte_ethdev/rte_flow.h
> > > @@ -28,6 +28,12 @@
> > >  #include <rte_udp.h>
> > >  #include <rte_byteorder.h>
> > >  #include <rte_esp.h>
> > > +#include <rte_log.h>
> > > +
> > > +extern int rte_flow_logtype;
> > > +
> > > +#define RTE_FLOW_LOG(level, ...) \
> > > +	rte_log(RTE_LOG_ ## level, rte_flow_logtype, "" __VA_ARGS__)
> > >
> > >  #ifdef __cplusplus
> > >  extern "C" {
> > > @@ -2414,6 +2420,31 @@ enum rte_flow_conv_op {
> > >  		  struct rte_flow_error *error);
> > >
> > >  /**
> > > + * Print the flow fields.
> > > + *
> > > + * The flow contains the port id, different attributes, the pattern's
> > > + * items and the action's items, which are all being printed.
> > > + *
> > > + * @b EXPERIMENTAL: this API may change without prior notice
> > > + *
> > > + * @param port_id
> > > + *   Port identifier of Ethernet device.
> > > + * @param[in] attr
> > > + *   Flow rule attributes.
> > > + * @param[in] pattern
> > > + *   Pattern specification (list terminated by the END pattern item).
> > > + * @param[in] actions
> > > + *   Associated actions (list terminated by the END action).
> > > + *
> > > + * @note not all enum values of patterns and actions are printed.
> > > + */
> > > +void __rte_experimental
> > > +rte_flow_print(uint16_t port_id,
> > > +		const struct rte_flow_attr *attr,
> > > +		const struct rte_flow_item pattern[],
> > > +		const struct rte_flow_action actions[]);
> > > +
> > > +/**
> > >   * Create a flow rule on a given port.
> > >   *
> > >   * @param port_id
> >
> > Since this is a debug function, it would make more sense for it to take a 'FILE
> > *'
> > for output like the other debug functions do.  That would make it consistent
> > with rte_pktmbuf_dump rte_event_dev_dump, rte_mempool_dump, etc.
> >
> > Why not name it rte_flow_dump?
> 
> Thanks for pointing this out. I'll consider renaming and make it accept a file.
> I'll wait for more comments first before uploading another version of the patch.