[dpdk-dev] [RFC 17.08] flow_classify: add librte_flow_classify library

[Ananyev, Konstantin]

Hi Ferruh,
Please see my comments/questions below.
Thanks
Konstantin

> +
> +/**
> + * @file
> + *
> + * RTE Flow Classify Library
> + *
> + * This library provides flow record information with some measured properties.
> + *
> + * Application can select variety of flow types based on various flow keys.
> + *
> + * Library only maintains flow records between rte_flow_classify_stats_get()
> + * calls and with a maximum limit.
> + *
> + * Provided flow record will be linked list rte_flow_classify_stat_xxx
> + * structure.
> + *
> + * Library is responsible from allocating and freeing memory for flow record
> + * table. Previous table freed with next rte_flow_classify_stats_get() call and
> + * all tables are freed with rte_flow_classify_type_reset() or
> + * rte_flow_classify_type_set(x, 0). Memory for table allocated on the fly while
> + * creating records.
> + *
> + * A rte_flow_classify_type_set() with a valid type will register Rx/Tx
> + * callbacks and start filling flow record table.
> + * With rte_flow_classify_stats_get(), pointer sent to caller and meanwhile
> + * library continues collecting records.
> + *
> + *  Usage:
> + *  - application calls rte_flow_classify_type_set() for a device
> + *  - library creates Rx/Tx callbacks for packets and start filling flow table

Does it necessary to use an  RX callback here?
Can library provide an API like collect(port_id, input_mbuf[], pkt_num) instead?
So the user would have a choice either setup a callback or call collect() directly. 

> + *    for that type of flow (currently only one flow type supported)
> + *  - application calls rte_flow_classify_stats_get() to get pointer to linked
> + *    listed flow table. Library assigns this pointer to another value and keeps
> + *    collecting flow data. In next rte_flow_classify_stats_get(), library first
> + *    free the previous table, and pass current table to the application, keep
> + *    collecting data.

Ok, but that means that you can't use stats_get() for the same type
from 2 different threads without explicit synchronization?

> + *  - application calls rte_flow_classify_type_reset(), library unregisters the
> + *    callbacks and free all flow table data.
> + *
> + */
> +
> +enum rte_flow_classify_type {
> +	RTE_FLOW_CLASSIFY_TYPE_GENERIC = (1 << 0),
> +	RTE_FLOW_CLASSIFY_TYPE_MAX,
> +};
> +
> +#define RTE_FLOW_CLASSIFY_TYPE_MASK = (((RTE_FLOW_CLASSIFY_TYPE_MAX - 1) << 1) - 1)
> +
> +/**
> + * Global configuration struct
> + */
> +struct rte_flow_classify_config {
> +	uint32_t type; /* bitwise enum rte_flow_classify_type values */
> +	void *flow_table_prev;
> +	uint32_t flow_table_prev_item_count;
> +	void *flow_table_current;
> +	uint32_t flow_table_current_item_count;
> +} rte_flow_classify_config[RTE_MAX_ETHPORTS];
> +
> +#define RTE_FLOW_CLASSIFY_STAT_MAX UINT16_MAX
> +
> +/**
> + * Classification stats data struct
> + */
> +struct rte_flow_classify_stat_generic {
> +	struct rte_flow_classify_stat_generic *next;
> +	uint32_t id;
> +	uint64_t timestamp;
> +
> +	struct ether_addr src_mac;
> +	struct ether_addr dst_mac;
> +	uint32_t src_ipv4;
> +	uint32_t dst_ipv4;
> +	uint8_t l3_protocol_id;
> +	uint16_t src_port;
> +	uint16_t dst_port;
> +
> +	uint64_t packet_count;
> +	uint64_t packet_size; /* bytes */
> +};

Ok, so if I understood things right, for generic type it will always classify all incoming packets by:
<src_mac, dst_mac, src_ipv4, dst_ipv4, l3_protocol_id, src_port, dst_port> 
all by absolute values, and represent results as a linked list.
Is that correct, or I misunderstood your intentions here?
If so, then I see several disadvantages here:
1) It is really hard to predict what kind of stats is required for that particular cases.
 Let say some people would like to collect stat by <dst_mac,, vlan> ,
another by <dst_ipv4,subnet_mask>, third ones by <l4 dst_port> and so on.
Having just one hardcoded filter doesn't seem very felxable/usable.
I think you need to find a way to allow user to define what type of filter they want to apply.
I think it was discussed already, but I still wonder why rte_flow_item can't be used for that approach?
2) Even  one 10G port can produce you ~14M rte_flow_classify_stat_generic entries in one second
(all packets have different ipv4/ports or so).
Accessing/retrieving items over linked list with 14M entries - doesn't sound like a good idea.
I'd say we need some better way to retrieve/present collected data.