Hi Adrien,
On Thu, Jul 06, 2017 at 06:56:54PM +0200, Adrien Mazarguil wrote:
> From: Gaetan Rivet <gaetan.ri...@6wind.com>
>
> This allows PMDs and applications to save flow rules in their generic
> format for later processing. This is useful when rules cannot be applied
> immediately, such as when the device is not properly initialized.
>
> Signed-off-by: Gaetan Rivet <gaetan.ri...@6wind.com>
> Signed-off-by: Adrien Mazarguil <adrien.mazarg...@6wind.com>
>
> ---
>
> Gaetan, sorry for taking over and modifying your patch, took me a while to
> review it and given there is not much time left before RC1, here is my
> suggestion in the form of an updated patch for reasons described below.
>
> I'm convinced exposing rte_flow_copy() is useful and mandatory not only for
> the fail-safe PMD, but also to avoid code duplication later.
>
> However since you took this code from testpmd, you've inherited its main
> drawback which is rte_flow_desc_item[] and rte_flow_desc_action[] are a
> pain to maintain. Every time new flow item/action are added, one has to
> update these arrays as well.
>
> Moreover it forces you to expose and version a bunch of additional symbols
> so far only useful to testpmd.
>
> Therefore I'm thinking about generating these arrays automatically at
> compilation time from enum definitions and not expose extra symbols at the
> same time. I'll try to submit these changes before the next RC.
>
That would be easier to maintain but I'd be curious to see an elegant
solution to this issue.
> So in the meantime, here's a v3 in order not to break existing series that
> depend on this patch and not introduce unnecessary symbols.
>
Thanks, however E_TAG and NVGRE are not covered (I should have updated
my version after the remark from Thomas). I will send a v4 shortly
including those.
> v2 -> v3:
>
> * Revert testpmd changes.
> * Do not expose extra symbols (rte_flow_desc_action, rte_flow_desc_item,
> rte_flow_nb_action and rte_flow_nb_item).
> * Do not expose struct rte_flow_desc_data.
> * Add missing #include directives to rte_flow.c.
>
> v1 -> v2:
>
> * fix checkpatch warnings
> ---
> lib/librte_ether/rte_ether_version.map | 1 +
> lib/librte_ether/rte_flow.c | 225 ++++++++++++++++++++++++++++
> lib/librte_ether/rte_flow.h | 40 +++++
> 3 files changed, 266 insertions(+)
>
> diff --git a/lib/librte_ether/rte_ether_version.map
> b/lib/librte_ether/rte_ether_version.map
> index 019a93d..6f65f83 100644
> --- a/lib/librte_ether/rte_ether_version.map
> +++ b/lib/librte_ether/rte_ether_version.map
> @@ -152,6 +152,7 @@ DPDK_17.08 {
> global:
>
> _rte_eth_dev_callback_process;
> + rte_flow_copy;
> rte_flow_isolate;
>
> } DPDK_17.05;
> diff --git a/lib/librte_ether/rte_flow.c b/lib/librte_ether/rte_flow.c
> index c1de31b..59ca85a 100644
> --- a/lib/librte_ether/rte_flow.c
> +++ b/lib/librte_ether/rte_flow.c
> @@ -31,14 +31,79 @@
> * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> */
>
> +#include <errno.h>
> +#include <stddef.h>
> #include <stdint.h>
> +#include <string.h>
>
> +#include <rte_common.h>
> #include <rte_errno.h>
> #include <rte_branch_prediction.h>
> #include "rte_ethdev.h"
> #include "rte_flow_driver.h"
> #include "rte_flow.h"
>
> +/**
> + * Flow elements description tables.
> + */
> +struct rte_flow_desc_data {
> + const char *name;
> + size_t size;
> +};
> +
> +/** Generate flow_item[] entry. */
> +#define MK_FLOW_ITEM(t, s) \
> + [RTE_FLOW_ITEM_TYPE_ ## t] = { \
> + .name = # t, \
> + .size = s, \
> + }
> +
> +/** Information about known flow pattern items. */
> +static const struct rte_flow_desc_data rte_flow_desc_item[] = {
> + MK_FLOW_ITEM(END, 0),
> + MK_FLOW_ITEM(VOID, 0),
> + MK_FLOW_ITEM(INVERT, 0),
> + MK_FLOW_ITEM(ANY, sizeof(struct rte_flow_item_any)),
> + MK_FLOW_ITEM(PF, 0),
> + MK_FLOW_ITEM(VF, sizeof(struct rte_flow_item_vf)),
> + MK_FLOW_ITEM(PORT, sizeof(struct rte_flow_item_port)),
> + MK_FLOW_ITEM(RAW, sizeof(struct rte_flow_item_raw)), /* +pattern[] */
> + MK_FLOW_ITEM(ETH, sizeof(struct rte_flow_item_eth)),
> + MK_FLOW_ITEM(VLAN, sizeof(struct rte_flow_item_vlan)),
> + MK_FLOW_ITEM(IPV4, sizeof(struct rte_flow_item_ipv4)),
> + MK_FLOW_ITEM(IPV6, sizeof(struct rte_flow_item_ipv6)),
> + MK_FLOW_ITEM(ICMP, sizeof(struct rte_flow_item_icmp)),
> + MK_FLOW_ITEM(UDP, sizeof(struct rte_flow_item_udp)),
> + MK_FLOW_ITEM(TCP, sizeof(struct rte_flow_item_tcp)),
> + MK_FLOW_ITEM(SCTP, sizeof(struct rte_flow_item_sctp)),
> + MK_FLOW_ITEM(VXLAN, sizeof(struct rte_flow_item_vxlan)),
> + MK_FLOW_ITEM(MPLS, sizeof(struct rte_flow_item_mpls)),
> + MK_FLOW_ITEM(GRE, sizeof(struct rte_flow_item_gre)),
> +};
> +
> +/** Generate flow_action[] entry. */
> +#define MK_FLOW_ACTION(t, s) \
> + [RTE_FLOW_ACTION_TYPE_ ## t] = { \
> + .name = # t, \
> + .size = s, \
> + }
> +
> +/** Information about known flow actions. */
> +static const struct rte_flow_desc_data rte_flow_desc_action[] = {
> + MK_FLOW_ACTION(END, 0),
> + MK_FLOW_ACTION(VOID, 0),
> + MK_FLOW_ACTION(PASSTHRU, 0),
> + MK_FLOW_ACTION(MARK, sizeof(struct rte_flow_action_mark)),
> + MK_FLOW_ACTION(FLAG, 0),
> + MK_FLOW_ACTION(QUEUE, sizeof(struct rte_flow_action_queue)),
> + MK_FLOW_ACTION(DROP, 0),
> + MK_FLOW_ACTION(COUNT, 0),
> + MK_FLOW_ACTION(DUP, sizeof(struct rte_flow_action_dup)),
> + MK_FLOW_ACTION(RSS, sizeof(struct rte_flow_action_rss)), /* +queue[] */
> + MK_FLOW_ACTION(PF, 0),
> + MK_FLOW_ACTION(VF, sizeof(struct rte_flow_action_vf)),
> +};
> +
> /* Get generic flow operations structure from a port. */
> const struct rte_flow_ops *
> rte_flow_ops_get(uint8_t port_id, struct rte_flow_error *error)
> @@ -175,3 +240,163 @@ rte_flow_isolate(uint8_t port_id,
> RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> NULL, rte_strerror(ENOSYS));
> }
> +
> +/** Compute storage space needed by item specification. */
> +static void
> +flow_item_spec_size(const struct rte_flow_item *item,
> + size_t *size, size_t *pad)
> +{
> + if (!item->spec)
> + goto empty;
> + switch (item->type) {
> + union {
> + const struct rte_flow_item_raw *raw;
> + } spec;
> +
> + /* Not a fall-through */
> + case RTE_FLOW_ITEM_TYPE_RAW:
> + spec.raw = item->spec;
> + *size = offsetof(struct rte_flow_item_raw, pattern) +
> + spec.raw->length * sizeof(*spec.raw->pattern);
> + break;
> + default:
> +empty:
> + *size = 0;
> + break;
> + }
> + *pad = RTE_ALIGN_CEIL(*size, sizeof(double)) - *size;
> +}
> +
> +/** Compute storage space needed by action configuration. */
> +static void
> +flow_action_conf_size(const struct rte_flow_action *action,
> + size_t *size, size_t *pad)
> +{
> + if (!action->conf)
> + goto empty;
> + switch (action->type) {
> + union {
> + const struct rte_flow_action_rss *rss;
> + } conf;
> +
> + /* Not a fall-through. */
> + case RTE_FLOW_ACTION_TYPE_RSS:
> + conf.rss = action->conf;
> + *size = offsetof(struct rte_flow_action_rss, queue) +
> + conf.rss->num * sizeof(*conf.rss->queue);
> + break;
> + default:
> +empty:
> + *size = 0;
> + break;
> + }
> + *pad = RTE_ALIGN_CEIL(*size, sizeof(double)) - *size;
> +}
> +
> +/** Store a full rte_flow description. */
> +size_t
> +rte_flow_copy(struct rte_flow_desc *desc, size_t len,
> + const struct rte_flow_attr *attr,
> + const struct rte_flow_item *items,
> + const struct rte_flow_action *actions)
> +{
> + struct rte_flow_desc *fd = NULL;
> + size_t tmp;
> + size_t pad;
> + size_t off1 = 0;
> + size_t off2 = 0;
> + size_t size = 0;
> +
> +store:
> + if (items) {
> + const struct rte_flow_item *item;
> +
> + item = items;
> + if (fd)
> + fd->items = (void *)&fd->data[off1];
> + do {
> + struct rte_flow_item *dst = NULL;
> +
> + if ((size_t)item->type >=
> + RTE_DIM(rte_flow_desc_item) ||
> + !rte_flow_desc_item[item->type].name) {
> + rte_errno = ENOTSUP;
> + return 0;
> + }
> + if (fd)
> + dst = memcpy(fd->data + off1, item,
> + sizeof(*item));
> + off1 += sizeof(*item);
> + flow_item_spec_size(item, &tmp, &pad);
> + if (item->spec) {
> + if (fd)
> + dst->spec = memcpy(fd->data + off2,
> + item->spec, tmp);
> + off2 += tmp + pad;
> + }
> + if (item->last) {
> + if (fd)
> + dst->last = memcpy(fd->data + off2,
> + item->last, tmp);
> + off2 += tmp + pad;
> + }
> + if (item->mask) {
> + if (fd)
> + dst->mask = memcpy(fd->data + off2,
> + item->mask, tmp);
> + off2 += tmp + pad;
> + }
> + off2 = RTE_ALIGN_CEIL(off2, sizeof(double));
> + } while ((item++)->type != RTE_FLOW_ITEM_TYPE_END);
> + off1 = RTE_ALIGN_CEIL(off1, sizeof(double));
> + }
> + if (actions) {
> + const struct rte_flow_action *action;
> +
> + action = actions;
> + if (fd)
> + fd->actions = (void *)&fd->data[off1];
> + do {
> + struct rte_flow_action *dst = NULL;
> +
> + if ((size_t)action->type >=
> + RTE_DIM(rte_flow_desc_action) ||
> + !rte_flow_desc_action[action->type].name) {
> + rte_errno = ENOTSUP;
> + return 0;
> + }
> + if (fd)
> + dst = memcpy(fd->data + off1, action,
> + sizeof(*action));
> + off1 += sizeof(*action);
> + flow_action_conf_size(action, &tmp, &pad);
> + if (action->conf) {
> + if (fd)
> + dst->conf = memcpy(fd->data + off2,
> + action->conf, tmp);
> + off2 += tmp + pad;
> + }
> + off2 = RTE_ALIGN_CEIL(off2, sizeof(double));
> + } while ((action++)->type != RTE_FLOW_ACTION_TYPE_END);
> + }
> + if (fd != NULL)
> + return size;
> + off1 = RTE_ALIGN_CEIL(off1, sizeof(double));
> + tmp = RTE_ALIGN_CEIL(offsetof(struct rte_flow_desc, data),
> + sizeof(double));
> + size = tmp + off1 + off2;
> + if (size > len)
> + return size;
> + fd = desc;
> + if (fd != NULL) {
> + *fd = (const struct rte_flow_desc) {
> + .size = size,
> + .attr = *attr,
> + };
> + tmp -= offsetof(struct rte_flow_desc, data);
> + off2 = tmp + off1;
> + off1 = tmp;
> + goto store;
> + }
> + return 0;
> +}
> diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h
> index cfbed30..6ac7cdb 100644
> --- a/lib/librte_ether/rte_flow.h
> +++ b/lib/librte_ether/rte_flow.h
> @@ -1266,6 +1266,46 @@ rte_flow_query(uint8_t port_id,
> int
> rte_flow_isolate(uint8_t port_id, int set, struct rte_flow_error *error);
>
> +/**
> + * Generic flow representation.
> + *
> + * This form is sufficient to describe an rte_flow independently from any
> + * PMD implementation and allows for replayability and identification.
> + */
> +struct rte_flow_desc {
> + size_t size; /**< Allocated space including data[]. */
> + struct rte_flow_attr attr; /**< Attributes. */
> + struct rte_flow_item *items; /**< Items. */
> + struct rte_flow_action *actions; /**< Actions. */
> + uint8_t data[]; /**< Storage for items/actions. */
> +};
> +
> +/**
> + * Copy an rte_flow rule description.
> + *
> + * @param[in] fd
> + * Flow rule description.
> + * @param[in] len
> + * Total size of allocated data for the flow description.
> + * @param[in] attr
> + * Flow rule attributes.
> + * @param[in] items
> + * Pattern specification (list terminated by the END pattern item).
> + * @param[in] actions
> + * Associated actions (list terminated by the END action).
> + *
> + * @return
> + * If len is greater or equal to the size of the flow, the total size of
> the
> + * flow description and its data.
> + * If len is lower than the size of the flow, the number of bytes that
> would
> + * have been written to desc had it been sufficient. Nothing is written.
> + */
> +size_t
> +rte_flow_copy(struct rte_flow_desc *fd, size_t len,
> + const struct rte_flow_attr *attr,
> + const struct rte_flow_item *items,
> + const struct rte_flow_action *actions);
> +
> #ifdef __cplusplus
> }
> #endif
> --
> 2.1.4
>
--
Gaëtan Rivet
6WIND
