Re: [ovs-dev] [PATCH v2 4/8] netdev-dpdk: implement flow put with rte flow
On Thu, Sep 14, 2017 at 10:35:10AM +0800, Yuanhan Liu wrote:
> On Wed, Sep 13, 2017 at 11:45:30AM +0200, Simon Horman wrote:
> > > > > +rte_memcpy(ð_spec.dst, &match->flow.dl_dst,
> > > sizeof(eth_spec.dst));
> > > > > +rte_memcpy(ð_spec.src, &match->flow.dl_src,
> > > sizeof(eth_spec.src));
> > > > > +eth_spec.type = match->flow.dl_type;
> > > > > +
> > > > > +rte_memcpy(ð_mask.dst, &match->wc.masks.dl_dst,
> > > > > + sizeof(eth_mask.dst));
> > > > > +rte_memcpy(ð_mask.src, &match->wc.masks.dl_src,
> > > > > + sizeof(eth_mask.src));
> > > > > +eth_mask.type = match->wc.masks.dl_type;
> > > > > +
> > > > > +add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_ETH,
> > > > > + ð_spec, ð_mask);
> > > > > +} else {
> > > > > +/*
> > > > > + * If user specifies a flow (like UDP flow) without L2
> > > patterns,
> > > > > + * OVS will at least set the dl_type. Normally, it's
> > > enough to
> > > > > + * create an eth pattern just with it. Unluckily, some
> > > Intel's
> > > > > + * NIC (such as XL710) doesn't support that. Below is a
> > > workaround,
> > > > > + * which simply matches any L2 pkts.
> > > > > + */
> > > > > +add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_ETH,
> > > NULL, NULL);
> > > > > +}
> > > >
> > > > This feels a lot like a layer violation - making the core aware
> > > > of specific implementation details of lower layers.
> > >
> > > I agree with you, but unlickily, without it, Intel NIC simply won't
> > > work
> > > (according to Finn), unless you have mac addr being provided.
> >
> > I think its reasonable to provide hardware-workarounds. But is it
> > appropriate to enable it for all hardware
>
> Without the underlaying hardware info/cap given, yes, I'm afraid that
> is what we can get best now.
Agreed.
> > and is this the most appropriate
> > place for a hardware work-around?
>
> Maybe not. Maybe this should be workarounded inside the DPDK PMD driver.
Pushing it lower down sounds better to me.
But I'm not really in a position to judge how practical
it is to handle this in the DPDK PMD driver.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 4/8] netdev-dpdk: implement flow put with rte flow
On Wed, Sep 13, 2017 at 05:18:03AM +, Darrell Ball wrote:
> +/* IP v4 */
> +uint8_t proto = 0;
> +struct rte_flow_item_ipv4 ipv4_spec;
> +struct rte_flow_item_ipv4 ipv4_mask;
> +memset(&ipv4_mask, 0, sizeof(ipv4_mask));
> +if ((match->flow.dl_type == ntohs(ETH_TYPE_IP)) &&
> +(match->wc.masks.nw_src || match->wc.masks.nw_dst ||
> + match->wc.masks.nw_tos || match->wc.masks.nw_ttl ||
> + match->wc.masks.nw_proto)) {
> +ipv4_spec.hdr.type_of_service = match->flow.nw_tos;
> +ipv4_spec.hdr.time_to_live= match->flow.nw_tos;
>
> [Darrell] should be TTL; lots of e-mails but I don’t remember seeing this
> mentioned
That's really a good catch! And actually, I have also noticed it few days,
that I have fixed in my local v3 branch.
> +
> +CHECK_NONZERO_BYTES(&match_zero_wc.flow.tunnel,
> +sizeof(match_zero_wc.flow.tunnel));
> +CHECK_NONZERO(match->wc.masks.metadata);
> +CHECK_NONZERO(match->wc.masks.skb_priority);
> +CHECK_NONZERO(match->wc.masks.pkt_mark);
> +CHECK_NONZERO(match->wc.masks.dp_hash);
> +
> +/* recirc id must be zero */
> +CHECK_NONZERO(match_zero_wc.flow.recirc_id);
> +
> +CHECK_NONZERO(match->wc.masks.ct_state);
> +CHECK_NONZERO(match->wc.masks.ct_zone);
> +CHECK_NONZERO(match->wc.masks.ct_mark);
>
> [Darrell]
> check the other pkt_metadata fields; some are missing here.
I will give it a try.
--yliu
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 4/8] netdev-dpdk: implement flow put with rte flow
On Wed, Sep 13, 2017 at 11:45:30AM +0200, Simon Horman wrote:
> > > > +rte_memcpy(ð_spec.dst, &match->flow.dl_dst,
> > sizeof(eth_spec.dst));
> > > > +rte_memcpy(ð_spec.src, &match->flow.dl_src,
> > sizeof(eth_spec.src));
> > > > +eth_spec.type = match->flow.dl_type;
> > > > +
> > > > +rte_memcpy(ð_mask.dst, &match->wc.masks.dl_dst,
> > > > + sizeof(eth_mask.dst));
> > > > +rte_memcpy(ð_mask.src, &match->wc.masks.dl_src,
> > > > + sizeof(eth_mask.src));
> > > > +eth_mask.type = match->wc.masks.dl_type;
> > > > +
> > > > +add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_ETH,
> > > > + ð_spec, ð_mask);
> > > > +} else {
> > > > +/*
> > > > + * If user specifies a flow (like UDP flow) without L2
> > patterns,
> > > > + * OVS will at least set the dl_type. Normally, it's
> > enough to
> > > > + * create an eth pattern just with it. Unluckily, some
> > Intel's
> > > > + * NIC (such as XL710) doesn't support that. Below is a
> > workaround,
> > > > + * which simply matches any L2 pkts.
> > > > + */
> > > > +add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_ETH, NULL,
> > NULL);
> > > > +}
> > >
> > > This feels a lot like a layer violation - making the core aware
> > > of specific implementation details of lower layers.
> >
> > I agree with you, but unlickily, without it, Intel NIC simply won't work
> > (according to Finn), unless you have mac addr being provided.
>
> I think its reasonable to provide hardware-workarounds. But is it
> appropriate to enable it for all hardware
Without the underlaying hardware info/cap given, yes, I'm afraid that
is what we can get best now.
> and is this the most appropriate
> place for a hardware work-around?
Maybe not. Maybe this should be workarounded inside the DPDK PMD driver.
--yliu
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 4/8] netdev-dpdk: implement flow put with rte flow
On Tue, Sep 12, 2017 at 08:34:44AM +, Darrell Ball wrote:
>
>
> On 9/10/17, 11:11 PM, "ovs-dev-boun...@openvswitch.org on behalf of Yuanhan
> Liu"
> wrote:
>
> On Fri, Sep 08, 2017 at 06:47:55PM +0200, Simon Horman wrote:
> > On Tue, Sep 05, 2017 at 05:22:57PM +0800, Yuanhan Liu wrote:
> > > From: Finn Christensen
> > >
> > > The basic yet the major part of this patch is to translate the "match"
> > > to rte flow patterns. And then, we create a rte flow with a MARK
> action.
> > > Afterwards, all pkts matches the flow will have the mark id in the
> mbuf.
> > >
> > > For any unsupported flows, such as MPLS, -1 is returned, meaning the
> > > flow offload is failed and then skipped.
> >
> > ...
> >
> > > +static int
> > > +netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
> > > + const struct match *match,
> > > + struct nlattr *nl_actions
> OVS_UNUSED,
> > > + size_t actions_len,
> > > + const ovs_u128 *ufid,
> > > + struct offload_info *info)
> > > +{
> > > +struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> > > +const struct rte_flow_attr flow_attr = {
> > > +.group = 0,
> > > +.priority = 0,
> > > +.ingress = 1,
> > > +.egress = 0
> > > +};
> > > +struct flow_patterns patterns = { .items = NULL, .cnt = 0 };
> > > +struct flow_actions actions = { .actions = NULL, .cnt = 0 };
> >
> > I believe the following will give the same result as the above,
> > less verbosely.
> >
> > +const struct rte_flow_attr flow_attr = { .ingress = 1 };
> > +struct flow_patterns patterns = { .cnt = 0 };
> > +struct flow_actions actions = { .cnt = 0 };
>
> I'm not quite sure on that. If the compiler doesn't do zero assigment
> correctly, something deadly wrong could happen. They all are short
> structs, that I think it's fine, IMO. If they are big, I'd prefer an
> explicit memset.
I'm pretty sure that the compiler will zero all other fields when the
scheme I described above is used. However, its a moot point if the approach
you used is preferred. So I think what you have is fine.
>
> > > +struct rte_flow *flow;
> > > +struct rte_flow_error error;
> > > +uint8_t *ipv4_next_proto_mask = NULL;
> > > +int ret = 0;
> > > +
> > > +/* Eth */
> > > +struct rte_flow_item_eth eth_spec;
> > > +struct rte_flow_item_eth eth_mask;
> >
> > Empty line here please.
>
> I actually want to bind the two declartions with the following code piece,
> to show that they are tightly related.
As I think Darrell pointed out elsewhere it is common practice to
put declarations close to code that uses them. This is different from
my assumption that they should be grouped at the top of a context.
With this in mind what you already have makes sense to me.
>
> > > +memset(ð_mask, 0, sizeof(eth_mask));
> > > +if (match->wc.masks.dl_src.be16[0] ||
> > > +match->wc.masks.dl_src.be16[1] ||
> > > +match->wc.masks.dl_src.be16[2] ||
> > > +match->wc.masks.dl_dst.be16[0] ||
> > > +match->wc.masks.dl_dst.be16[1] ||
> > > +match->wc.masks.dl_dst.be16[2]) {
> >
> > It looks like eth_addr_is_zero() can be used in the above.
>
> Yes, we could. Thanks for the tip.
>
> > > +rte_memcpy(ð_spec.dst, &match->flow.dl_dst,
> sizeof(eth_spec.dst));
> > > +rte_memcpy(ð_spec.src, &match->flow.dl_src,
> sizeof(eth_spec.src));
> > > +eth_spec.type = match->flow.dl_type;
> > > +
> > > +rte_memcpy(ð_mask.dst, &match->wc.masks.dl_dst,
> > > + sizeof(eth_mask.dst));
> > > +rte_memcpy(ð_mask.src, &match->wc.masks.dl_src,
> > > + sizeof(eth_mask.src));
> > > +eth_mask.type = match->wc.masks.dl_type;
> > > +
> > > +add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_ETH,
> > > + ð_spec, ð_mask);
> > > +} else {
> > > +/*
> > > + * If user specifies a flow (like UDP flow) without L2
> patterns,
> > > + * OVS will at least set the dl_type. Normally, it's enough
> to
> > > + * create an eth pattern just with it. Unluckily, some
> Intel's
> > > + * NIC (such as XL710) doesn't support that. Below is a
> workaround,
> > > + * which simply matches any L2 pkts.
> > > + */
> > > +add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_ETH, NULL,
> NULL);
> > > +}
> >
> > This feels a lot like a layer violation - making the core
Re: [ovs-dev] [PATCH v2 4/8] netdev-dpdk: implement flow put with rte flow
More comments
On 9/12/17, 9:15 PM, "Darrell Ball" wrote:
Still working my way through this patch, but I had a couple comments inline
On 9/5/17, 2:23 AM, "Yuanhan Liu" wrote:
From: Finn Christensen
The basic yet the major part of this patch is to translate the "match"
to rte flow patterns. And then, we create a rte flow with a MARK action.
Afterwards, all pkts matches the flow will have the mark id in the mbuf.
For any unsupported flows, such as MPLS, -1 is returned, meaning the
flow offload is failed and then skipped.
Co-authored-by: Yuanhan Liu
Signed-off-by: Finn Christensen
Signed-off-by: Yuanhan Liu
---
v2: - convert some macros to functions
- do not hardcode the max number of flow/action
- fix L2 patterns for Intel nic
- add comments for not implemented offload methods
---
lib/netdev-dpdk.c | 421
+-
1 file changed, 420 insertions(+), 1 deletion(-)
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 46f9885..37b0f99 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -58,6 +58,7 @@
#include "smap.h"
#include "sset.h"
#include "unaligned.h"
+#include "uuid.h"
#include "timeval.h"
#include "unixctl.h"
@@ -3400,6 +3401,424 @@ get_rte_flow_by_ufid(const ovs_u128 *ufid)
}
+struct flow_patterns {
+struct rte_flow_item *items;
+int cnt;
+int max;
+};
+
+struct flow_actions {
+struct rte_flow_action *actions;
+int cnt;
+int max;
+};
+
+static void
+add_flow_pattern(struct flow_patterns *patterns, enum
rte_flow_item_type type,
+ const void *spec, const void *mask)
+{
+int cnt = patterns->cnt;
+
+if (cnt == 0) {
+patterns->max = 8;
+patterns->items = xcalloc(patterns->max, sizeof(struct
rte_flow_item));
+} else if (cnt == patterns->max) {
+patterns->max *= 2;
+patterns->items = xrealloc(patterns->items, patterns->max *
+ sizeof(struct rte_flow_item));
+}
+
+patterns->items[cnt].type = type;
+patterns->items[cnt].spec = spec;
+patterns->items[cnt].mask = mask;
+patterns->items[cnt].last = NULL;
+patterns->cnt++;
+}
+
+static void
+add_flow_action(struct flow_actions *actions, enum
rte_flow_action_type type,
+const void *conf)
+{
+int cnt = actions->cnt;
+
+if (cnt == 0) {
+actions->max = 8;
+actions->actions = xcalloc(actions->max,
+ sizeof(struct rte_flow_action));
+} else if (cnt == actions->max) {
+actions->max *= 2;
+actions->actions = xrealloc(actions->actions, actions->max *
+sizeof(struct rte_flow_action));
+}
+
+actions->actions[cnt].type = type;
+actions->actions[cnt].conf = conf;
+actions->cnt++;
+}
+
+static int
+netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
+ const struct match *match,
+ struct nlattr *nl_actions OVS_UNUSED,
+ size_t actions_len,
+ const ovs_u128 *ufid,
+ struct offload_info *info)
+{
+struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
+const struct rte_flow_attr flow_attr = {
+.group = 0,
+.priority = 0,
+.ingress = 1,
+.egress = 0
+};
+struct flow_patterns patterns = { .items = NULL, .cnt = 0 };
+struct flow_actions actions = { .actions = NULL, .cnt = 0 };
+struct rte_flow *flow;
+struct rte_flow_error error;
+uint8_t *ipv4_next_proto_mask = NULL;
+int ret = 0;
+
+/* Eth */
+struct rte_flow_item_eth eth_spec;
+struct rte_flow_item_eth eth_mask;
+memset(ð_mask, 0, sizeof(eth_mask));
+if (match->wc.masks.dl_src.be16[0] ||
+match->wc.masks.dl_src.be16[1] ||
+match->wc.masks.dl_src.be16[2] ||
+match->wc.masks.dl_dst.be16[0] ||
+
Re: [ovs-dev] [PATCH v2 4/8] netdev-dpdk: implement flow put with rte flow
Still working my way through this patch, but I had a couple comments inline
On 9/5/17, 2:23 AM, "Yuanhan Liu" wrote:
From: Finn Christensen
The basic yet the major part of this patch is to translate the "match"
to rte flow patterns. And then, we create a rte flow with a MARK action.
Afterwards, all pkts matches the flow will have the mark id in the mbuf.
For any unsupported flows, such as MPLS, -1 is returned, meaning the
flow offload is failed and then skipped.
Co-authored-by: Yuanhan Liu
Signed-off-by: Finn Christensen
Signed-off-by: Yuanhan Liu
---
v2: - convert some macros to functions
- do not hardcode the max number of flow/action
- fix L2 patterns for Intel nic
- add comments for not implemented offload methods
---
lib/netdev-dpdk.c | 421
+-
1 file changed, 420 insertions(+), 1 deletion(-)
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 46f9885..37b0f99 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -58,6 +58,7 @@
#include "smap.h"
#include "sset.h"
#include "unaligned.h"
+#include "uuid.h"
#include "timeval.h"
#include "unixctl.h"
@@ -3400,6 +3401,424 @@ get_rte_flow_by_ufid(const ovs_u128 *ufid)
}
+struct flow_patterns {
+struct rte_flow_item *items;
+int cnt;
+int max;
+};
+
+struct flow_actions {
+struct rte_flow_action *actions;
+int cnt;
+int max;
+};
+
+static void
+add_flow_pattern(struct flow_patterns *patterns, enum rte_flow_item_type
type,
+ const void *spec, const void *mask)
+{
+int cnt = patterns->cnt;
+
+if (cnt == 0) {
+patterns->max = 8;
+patterns->items = xcalloc(patterns->max, sizeof(struct
rte_flow_item));
+} else if (cnt == patterns->max) {
+patterns->max *= 2;
+patterns->items = xrealloc(patterns->items, patterns->max *
+ sizeof(struct rte_flow_item));
+}
+
+patterns->items[cnt].type = type;
+patterns->items[cnt].spec = spec;
+patterns->items[cnt].mask = mask;
+patterns->items[cnt].last = NULL;
+patterns->cnt++;
+}
+
+static void
+add_flow_action(struct flow_actions *actions, enum rte_flow_action_type
type,
+const void *conf)
+{
+int cnt = actions->cnt;
+
+if (cnt == 0) {
+actions->max = 8;
+actions->actions = xcalloc(actions->max,
+ sizeof(struct rte_flow_action));
+} else if (cnt == actions->max) {
+actions->max *= 2;
+actions->actions = xrealloc(actions->actions, actions->max *
+sizeof(struct rte_flow_action));
+}
+
+actions->actions[cnt].type = type;
+actions->actions[cnt].conf = conf;
+actions->cnt++;
+}
+
+static int
+netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
+ const struct match *match,
+ struct nlattr *nl_actions OVS_UNUSED,
+ size_t actions_len,
+ const ovs_u128 *ufid,
+ struct offload_info *info)
+{
+struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
+const struct rte_flow_attr flow_attr = {
+.group = 0,
+.priority = 0,
+.ingress = 1,
+.egress = 0
+};
+struct flow_patterns patterns = { .items = NULL, .cnt = 0 };
+struct flow_actions actions = { .actions = NULL, .cnt = 0 };
+struct rte_flow *flow;
+struct rte_flow_error error;
+uint8_t *ipv4_next_proto_mask = NULL;
+int ret = 0;
+
+/* Eth */
+struct rte_flow_item_eth eth_spec;
+struct rte_flow_item_eth eth_mask;
+memset(ð_mask, 0, sizeof(eth_mask));
+if (match->wc.masks.dl_src.be16[0] ||
+match->wc.masks.dl_src.be16[1] ||
+match->wc.masks.dl_src.be16[2] ||
+match->wc.masks.dl_dst.be16[0] ||
+match->wc.masks.dl_dst.be16[1] ||
+match->wc.masks.dl_dst.be16[2]) {
+rte_memcpy(ð_spec.dst, &match->flow.dl_dst,
sizeof(eth_spec.dst));
+rte_memcpy(ð_spec.src, &match->flow.dl_src,
sizeof(eth_spec.src));
+eth_spec.type = match->flow.dl_type;
+
+rte_memcpy(ð_mask.dst, &match->wc.masks.dl_dst,
+ sizeof(eth_mask.dst));
+rte_memcpy(ð_mask.src, &match->wc.masks.dl_src,
+ sizeof(eth_mask.src));
+eth_mask.type = match->wc.masks.dl_type;
Re: [ovs-dev] [PATCH v2 4/8] netdev-dpdk: implement flow put with rte flow
On 9/11/17, 3:02 AM, "ovs-dev-boun...@openvswitch.org on behalf of Yuanhan Liu" wrote: On Sun, Sep 10, 2017 at 04:32:19PM +, Chandran, Sugesh wrote: > As mentioned earlier in the cover letter we also have a similar implementation to do the flow translate. > I feel its good to make bit more modular for this translation. The reason being its easy to extend and add more protocols in the future. Honestly, I don't see a strong need to make it that complex first. Yes, it's a big function with a chunk of codes. And yes, I'm also a fun of splitting big monsters to many small functions. However, if you look at it closer, you will see it's nicely organized: the function is split nicely with chunks: something like one chunk for one protocol. Extending is also not that hard: just adding another chunk of the code. Besides, if you see tc offloads, they do it in a same way. [...] > > + > > +/* > > + * Validate for later rte flow offload creation. If any unsupported > > + * flow are specified, return -1. > > + */ > [Sugesh] I feel this is very hardware centric. There are chances hardware can support > Ipv6 or other fields in a packet. This has to be based on what flow fields a hardware can support. Yes, you are right. Again, we need capabilities feedback from DPDK. [Darrell] There have been several mentions of capability queries in version 1 and version 2 of the patchset. 1/ We asked for HW scaling checks to better support manageability and predictability. 2/ We asked for a ‘queue action workaround requirement’ query, which would be a nice enhancement, to avoid an unnecessary first failed attempt at flow create for nics that don’t allow the mark action in isolation. 3/ Here we ask for flow matching capability checking, with fallback to what we already support today. Similar kinds of queries were also mentioned in regard to the patchset “prioritizing latency sensitive traffic”. TCP flags matching mentioned in Patch 2 also falls into this category. --yliu ___ dev mailing list d...@openvswitch.org https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=SbdNOA5djgzhAIm49EWuBLui1KsaSl-tOsvhgb685Js&s=oKRfV8aE2G8A_Te761VR9n29SJuhr4-SJshaFetgICw&e= ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 4/8] netdev-dpdk: implement flow put with rte flow
On 9/10/17, 11:11 PM, "ovs-dev-boun...@openvswitch.org on behalf of Yuanhan
Liu" wrote:
On Fri, Sep 08, 2017 at 06:47:55PM +0200, Simon Horman wrote:
> On Tue, Sep 05, 2017 at 05:22:57PM +0800, Yuanhan Liu wrote:
> > From: Finn Christensen
> >
> > The basic yet the major part of this patch is to translate the "match"
> > to rte flow patterns. And then, we create a rte flow with a MARK action.
> > Afterwards, all pkts matches the flow will have the mark id in the mbuf.
> >
> > For any unsupported flows, such as MPLS, -1 is returned, meaning the
> > flow offload is failed and then skipped.
>
> ...
>
> > +static int
> > +netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
> > + const struct match *match,
> > + struct nlattr *nl_actions OVS_UNUSED,
> > + size_t actions_len,
> > + const ovs_u128 *ufid,
> > + struct offload_info *info)
> > +{
> > +struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> > +const struct rte_flow_attr flow_attr = {
> > +.group = 0,
> > +.priority = 0,
> > +.ingress = 1,
> > +.egress = 0
> > +};
> > +struct flow_patterns patterns = { .items = NULL, .cnt = 0 };
> > +struct flow_actions actions = { .actions = NULL, .cnt = 0 };
>
> I believe the following will give the same result as the above,
> less verbosely.
>
> +const struct rte_flow_attr flow_attr = { .ingress = 1 };
> +struct flow_patterns patterns = { .cnt = 0 };
> +struct flow_actions actions = { .cnt = 0 };
I'm not quite sure on that. If the compiler doesn't do zero assigment
correctly, something deadly wrong could happen. They all are short
structs, that I think it's fine, IMO. If they are big, I'd prefer an
explicit memset.
> > +struct rte_flow *flow;
> > +struct rte_flow_error error;
> > +uint8_t *ipv4_next_proto_mask = NULL;
> > +int ret = 0;
> > +
> > +/* Eth */
> > +struct rte_flow_item_eth eth_spec;
> > +struct rte_flow_item_eth eth_mask;
>
> Empty line here please.
I actually want to bind the two declartions with the following code piece,
to show that they are tightly related.
> > +memset(ð_mask, 0, sizeof(eth_mask));
> > +if (match->wc.masks.dl_src.be16[0] ||
> > +match->wc.masks.dl_src.be16[1] ||
> > +match->wc.masks.dl_src.be16[2] ||
> > +match->wc.masks.dl_dst.be16[0] ||
> > +match->wc.masks.dl_dst.be16[1] ||
> > +match->wc.masks.dl_dst.be16[2]) {
>
> It looks like eth_addr_is_zero() can be used in the above.
Yes, we could. Thanks for the tip.
> > +rte_memcpy(ð_spec.dst, &match->flow.dl_dst,
sizeof(eth_spec.dst));
> > +rte_memcpy(ð_spec.src, &match->flow.dl_src,
sizeof(eth_spec.src));
> > +eth_spec.type = match->flow.dl_type;
> > +
> > +rte_memcpy(ð_mask.dst, &match->wc.masks.dl_dst,
> > + sizeof(eth_mask.dst));
> > +rte_memcpy(ð_mask.src, &match->wc.masks.dl_src,
> > + sizeof(eth_mask.src));
> > +eth_mask.type = match->wc.masks.dl_type;
> > +
> > +add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_ETH,
> > + ð_spec, ð_mask);
> > +} else {
> > +/*
> > + * If user specifies a flow (like UDP flow) without L2
patterns,
> > + * OVS will at least set the dl_type. Normally, it's enough to
> > + * create an eth pattern just with it. Unluckily, some Intel's
> > + * NIC (such as XL710) doesn't support that. Below is a
workaround,
> > + * which simply matches any L2 pkts.
> > + */
> > +add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_ETH, NULL,
NULL);
> > +}
>
> This feels a lot like a layer violation - making the core aware
> of specific implementation details of lower layers.
I agree with you, but unlickily, without it, Intel NIC simply won't work
(according to Finn), unless you have mac addr being provided.
> >From a functional point of view, is the idea that
> a eth_type+5-tuple match is converted to a 5-tuple match?
Unluckily, yes.
> > +/* VLAN */
> > +struct rte_flow_item_vlan vlan_spec;
> > +struct rte_flow_item_vlan vlan_mask;
>
> Please declare all local variables at the top of the context
> (in this case function).
>
> ...
>
> > +struct rte_flow_item_udp udp_spec;
> > +struct rte_flow_item_udp udp_mask;
> > +memset(&udp_mask, 0, siz
Re: [ovs-dev] [PATCH v2 4/8] netdev-dpdk: implement flow put with rte flow
On Mon, Sep 11, 2017 at 10:04:21AM +, Chandran, Sugesh wrote: > > > Regards > _Sugesh > > > > -Original Message- > > From: Yuanhan Liu [mailto:y...@fridaylinux.org] > > Sent: Monday, September 11, 2017 10:58 AM > > To: Chandran, Sugesh > > Cc: d...@openvswitch.org > > Subject: Re: [ovs-dev] [PATCH v2 4/8] netdev-dpdk: implement flow put with > > rte flow > > > > On Sun, Sep 10, 2017 at 04:32:19PM +, Chandran, Sugesh wrote: > > > As mentioned earlier in the cover letter we also have a similar > > implementation to do the flow translate. > > > I feel its good to make bit more modular for this translation. The reason > > being its easy to extend and add more protocols in the future. > > > > Honestly, I don't see a strong need to make it that complex first. Yes, > > it's a big > > function with a chunk of codes. And yes, I'm also a fun of splitting big > > monsters to many small functions. However, if you look at it closer, you > > will > > see it's nicely organized: the function is split nicely with chunks: > > something > > like one chunk for one protocol. > > > > Extending is also not that hard: just adding another chunk of the code. > [Sugesh] Sure, I just wanted to share our proposal which works fine for our > usecase. :) Thanks for sharing! --yliu ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 4/8] netdev-dpdk: implement flow put with rte flow
Regards _Sugesh > -Original Message- > From: Yuanhan Liu [mailto:y...@fridaylinux.org] > Sent: Monday, September 11, 2017 10:58 AM > To: Chandran, Sugesh > Cc: d...@openvswitch.org > Subject: Re: [ovs-dev] [PATCH v2 4/8] netdev-dpdk: implement flow put with > rte flow > > On Sun, Sep 10, 2017 at 04:32:19PM +, Chandran, Sugesh wrote: > > As mentioned earlier in the cover letter we also have a similar > implementation to do the flow translate. > > I feel its good to make bit more modular for this translation. The reason > being its easy to extend and add more protocols in the future. > > Honestly, I don't see a strong need to make it that complex first. Yes, it's > a big > function with a chunk of codes. And yes, I'm also a fun of splitting big > monsters to many small functions. However, if you look at it closer, you will > see it's nicely organized: the function is split nicely with chunks: something > like one chunk for one protocol. > > Extending is also not that hard: just adding another chunk of the code. [Sugesh] Sure, I just wanted to share our proposal which works fine for our usecase. :) > > Besides, if you see tc offloads, they do it in a same way. [Sugesh] yes. > > [...] > > > + > > > +/* > > > + * Validate for later rte flow offload creation. If any unsupported > > > + * flow are specified, return -1. > > > + */ > > [Sugesh] I feel this is very hardware centric. There are chances > > hardware can support > > Ipv6 or other fields in a packet. This has to be based on what flow fields a > hardware can support. > > Yes, you are right. Again, we need capabilities feedback from DPDK. [Sugesh] Ok. > > --yliu ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 4/8] netdev-dpdk: implement flow put with rte flow
On Sun, Sep 10, 2017 at 04:32:19PM +, Chandran, Sugesh wrote: > As mentioned earlier in the cover letter we also have a similar > implementation to do the flow translate. > I feel its good to make bit more modular for this translation. The reason > being its easy to extend and add more protocols in the future. Honestly, I don't see a strong need to make it that complex first. Yes, it's a big function with a chunk of codes. And yes, I'm also a fun of splitting big monsters to many small functions. However, if you look at it closer, you will see it's nicely organized: the function is split nicely with chunks: something like one chunk for one protocol. Extending is also not that hard: just adding another chunk of the code. Besides, if you see tc offloads, they do it in a same way. [...] > > + > > +/* > > + * Validate for later rte flow offload creation. If any unsupported > > + * flow are specified, return -1. > > + */ > [Sugesh] I feel this is very hardware centric. There are chances hardware can > support > Ipv6 or other fields in a packet. This has to be based on what flow fields a > hardware can support. Yes, you are right. Again, we need capabilities feedback from DPDK. --yliu ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 4/8] netdev-dpdk: implement flow put with rte flow
On Fri, Sep 08, 2017 at 06:47:55PM +0200, Simon Horman wrote:
> On Tue, Sep 05, 2017 at 05:22:57PM +0800, Yuanhan Liu wrote:
> > From: Finn Christensen
> >
> > The basic yet the major part of this patch is to translate the "match"
> > to rte flow patterns. And then, we create a rte flow with a MARK action.
> > Afterwards, all pkts matches the flow will have the mark id in the mbuf.
> >
> > For any unsupported flows, such as MPLS, -1 is returned, meaning the
> > flow offload is failed and then skipped.
>
> ...
>
> > +static int
> > +netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
> > + const struct match *match,
> > + struct nlattr *nl_actions OVS_UNUSED,
> > + size_t actions_len,
> > + const ovs_u128 *ufid,
> > + struct offload_info *info)
> > +{
> > +struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> > +const struct rte_flow_attr flow_attr = {
> > +.group = 0,
> > +.priority = 0,
> > +.ingress = 1,
> > +.egress = 0
> > +};
> > +struct flow_patterns patterns = { .items = NULL, .cnt = 0 };
> > +struct flow_actions actions = { .actions = NULL, .cnt = 0 };
>
> I believe the following will give the same result as the above,
> less verbosely.
>
> +const struct rte_flow_attr flow_attr = { .ingress = 1 };
> +struct flow_patterns patterns = { .cnt = 0 };
> +struct flow_actions actions = { .cnt = 0 };
I'm not quite sure on that. If the compiler doesn't do zero assigment
correctly, something deadly wrong could happen. They all are short
structs, that I think it's fine, IMO. If they are big, I'd prefer an
explicit memset.
> > +struct rte_flow *flow;
> > +struct rte_flow_error error;
> > +uint8_t *ipv4_next_proto_mask = NULL;
> > +int ret = 0;
> > +
> > +/* Eth */
> > +struct rte_flow_item_eth eth_spec;
> > +struct rte_flow_item_eth eth_mask;
>
> Empty line here please.
I actually want to bind the two declartions with the following code piece,
to show that they are tightly related.
> > +memset(ð_mask, 0, sizeof(eth_mask));
> > +if (match->wc.masks.dl_src.be16[0] ||
> > +match->wc.masks.dl_src.be16[1] ||
> > +match->wc.masks.dl_src.be16[2] ||
> > +match->wc.masks.dl_dst.be16[0] ||
> > +match->wc.masks.dl_dst.be16[1] ||
> > +match->wc.masks.dl_dst.be16[2]) {
>
> It looks like eth_addr_is_zero() can be used in the above.
Yes, we could. Thanks for the tip.
> > +rte_memcpy(ð_spec.dst, &match->flow.dl_dst,
> > sizeof(eth_spec.dst));
> > +rte_memcpy(ð_spec.src, &match->flow.dl_src,
> > sizeof(eth_spec.src));
> > +eth_spec.type = match->flow.dl_type;
> > +
> > +rte_memcpy(ð_mask.dst, &match->wc.masks.dl_dst,
> > + sizeof(eth_mask.dst));
> > +rte_memcpy(ð_mask.src, &match->wc.masks.dl_src,
> > + sizeof(eth_mask.src));
> > +eth_mask.type = match->wc.masks.dl_type;
> > +
> > +add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_ETH,
> > + ð_spec, ð_mask);
> > +} else {
> > +/*
> > + * If user specifies a flow (like UDP flow) without L2 patterns,
> > + * OVS will at least set the dl_type. Normally, it's enough to
> > + * create an eth pattern just with it. Unluckily, some Intel's
> > + * NIC (such as XL710) doesn't support that. Below is a workaround,
> > + * which simply matches any L2 pkts.
> > + */
> > +add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_ETH, NULL, NULL);
> > +}
>
> This feels a lot like a layer violation - making the core aware
> of specific implementation details of lower layers.
I agree with you, but unlickily, without it, Intel NIC simply won't work
(according to Finn), unless you have mac addr being provided.
> >From a functional point of view, is the idea that
> a eth_type+5-tuple match is converted to a 5-tuple match?
Unluckily, yes.
> > +/* VLAN */
> > +struct rte_flow_item_vlan vlan_spec;
> > +struct rte_flow_item_vlan vlan_mask;
>
> Please declare all local variables at the top of the context
> (in this case function).
>
> ...
>
> > +struct rte_flow_item_udp udp_spec;
> > +struct rte_flow_item_udp udp_mask;
> > +memset(&udp_mask, 0, sizeof(udp_mask));
> > +if ((proto == IPPROTO_UDP) &&
> > +(match->wc.masks.tp_src || match->wc.masks.tp_dst)) {
> > +udp_spec.hdr.src_port = match->flow.tp_src;
> > +udp_spec.hdr.dst_port = match->flow.tp_dst;
> > +
> > +udp_mask.hdr.src_port = match->wc.masks.tp_src;
> > +udp_mask.hdr.dst_port = match->wc.masks.tp_dst;
> > +
> > +add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_UDP,
> > + &udp_spec, &udp_mask);
> > +
> > +/* proto == UDP and ITEM_TYPE_UDP, thus n
Re: [ovs-dev] [PATCH v2 4/8] netdev-dpdk: implement flow put with rte flow
As mentioned earlier in the cover letter we also have a similar implementation
to do the flow translate.
I feel its good to make bit more modular for this translation. The reason
being its easy to extend and add more protocols in the future.
Please find below our similar implementation. We have started with single
function implementation as in patch series, and eventually moved
to this approach.
I can share the code as a patch if you are interested. Again the approach
below work well for our use case. Feel free to
discard if it doesn't make any sense/it's an overhead.
The flow and actions translate were invoked from dpdkhw_rte_flow_xlate and '
dpdkhw_rte_action_xlate'.
struct flow_xlate_dic {
enum rte_flow_item_type rte_flow_type;
/*
* Flow xlate function to translate specific header match into rtl format.
* Each rte_flow_item_type, it is necessary to define a corresponding
* xlate function in this structure. Return 0 if the flow is being
translated
* successfully and error code otherwise.
*/
int (*flow_xlate)(struct match *match, struct rte_flow_item *hw_flow_item,
const void *md);
};
static int do_inport_flow_xlate(struct match *match,
struct rte_flow_item *hw_flow_item, const void *md);
static int do_l2_flow_xlate(struct match *match,
struct rte_flow_item *hw_flow_item, const void *md);
static int do_l3_flow_xlate(struct match *match,
struct rte_flow_item *hw_flow_item, const void *md);
static int do_l4_flow_xlate(struct match *match,
struct rte_flow_item *hw_flow_item, const void *md);
static int do_end_flow_xlate(struct match *match,
struct rte_flow_item *hw_flow_item, const void *md);
struct flow_xlate_dic PORT_FLOW_XLATE = {
RTE_FLOW_ITEM_TYPE_PORT,
do_inport_flow_xlate
};
struct flow_xlate_dic L2_FLOW_XLATE = {
RTE_FLOW_ITEM_TYPE_ETH,
do_l2_flow_xlate
};
struct flow_xlate_dic L3_FLOW_XLATE = {
RTE_FLOW_ITEM_TYPE_VOID, /* L3 flow item can be different */
do_l3_flow_xlate
};
struct flow_xlate_dic L4_FLOW_XLATE = {
RTE_FLOW_ITEM_TYPE_VOID, /* Can be UDP/TCP */
do_l4_flow_xlate
};
struct flow_xlate_dic END_FLOW_XLATE = {
RTE_FLOW_ITEM_TYPE_END,
do_end_flow_xlate
};
/*
* Convert the mac address to little endian for flow programming
*/
static void
ntoh_mac(struct eth_addr *src, struct eth_addr *dst)
{
int i;
for (i = 0; i < 6; i++) {
dst->ea[6 - i - 1] = src->ea[i];
}
}
static int
do_inport_flow_xlate(struct match *match, struct rte_flow_item *hw_flow_item,
const void *md)
{
struct flow *flow;
struct rte_flow_item_port *port_flow_item;
struct rte_flow_item_port *port_flow_item_mask;
struct netdev *netdev;
uint16_t hw_portno;
struct offload_info *ofld_info = (struct offload_info *)md;
flow = &match->flow;
port_flow_item = xzalloc (sizeof *port_flow_item);
port_flow_item_mask = xzalloc(sizeof *port_flow_item_mask);
if(!port_flow_item) {
VLOG_ERR("Failed to allocate the memory for hardware flow item");
return ENOMEM;
}
netdev = get_hw_netdev(flow->in_port.odp_port, ofld_info->port_hmap_obj);
if (!netdev) {
VLOG_WARN("Inport %u is not a valid hardware accelerated port.",
odp_to_u32(flow->in_port.odp_port));
return EOPNOTSUPP;
}
/* The inport should be the hardware port number, not the ovs portno */
hw_portno = netdev_get_hw_portno(netdev);
port_flow_item->index = hw_portno;
hw_flow_item->type = PORT_FLOW_XLATE.rte_flow_type;
hw_flow_item->spec = port_flow_item;
hw_flow_item->last = NULL;
/* Set the mask for the rte port flow */
port_flow_item_mask->index = 0x;
hw_flow_item->mask = port_flow_item_mask;
return 0;
}
static int
do_l2_flow_xlate(struct match *match, struct rte_flow_item *hw_flow_item,
const void *md)
{
struct flow *flow, *mask;
struct rte_flow_item_eth *eth_flow_item;
struct rte_flow_item_eth *eth_flow_mask;
struct eth_addr mac_le;
flow = &match->flow;
mask = &match->wc.masks;
bool is_l2_zero = 0;
is_l2_zero = eth_addr_is_zero(mask->dl_dst);
is_l2_zero &= eth_addr_is_zero(mask->dl_src);
if(is_l2_zero) {
VLOG_INFO("Cannot install flow with zero eth addr");
return EOPNOTSUPP;
}
eth_flow_item = xzalloc(sizeof *eth_flow_item);
eth_flow_mask = xzalloc(sizeof *eth_flow_mask);
if(!eth_flow_item || !eth_flow_mask) {
VLOG_ERR("Failed to allocate the memory for flow item");
return ENOMEM;
}
ntoh_mac(&flow->dl_dst, &mac_le);
memcpy(ð_flow_item->dst, &mac_le, sizeof(eth_flow_item->dst));
ntoh_mac(&flow->dl
Re: [ovs-dev] [PATCH v2 4/8] netdev-dpdk: implement flow put with rte flow
On 9/8/17, 9:47 AM, "ovs-dev-boun...@openvswitch.org on behalf of Simon Horman"
wrote:
On Tue, Sep 05, 2017 at 05:22:57PM +0800, Yuanhan Liu wrote:
> From: Finn Christensen
>
> The basic yet the major part of this patch is to translate the "match"
> to rte flow patterns. And then, we create a rte flow with a MARK action.
> Afterwards, all pkts matches the flow will have the mark id in the mbuf.
>
> For any unsupported flows, such as MPLS, -1 is returned, meaning the
> flow offload is failed and then skipped.
...
> +static int
> +netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
> + const struct match *match,
> + struct nlattr *nl_actions OVS_UNUSED,
> + size_t actions_len,
> + const ovs_u128 *ufid,
> + struct offload_info *info)
> +{
> +struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> +const struct rte_flow_attr flow_attr = {
> +.group = 0,
> +.priority = 0,
> +.ingress = 1,
> +.egress = 0
> +};
> +struct flow_patterns patterns = { .items = NULL, .cnt = 0 };
> +struct flow_actions actions = { .actions = NULL, .cnt = 0 };
I believe the following will give the same result as the above,
less verbosely.
+const struct rte_flow_attr flow_attr = { .ingress = 1 };
+struct flow_patterns patterns = { .cnt = 0 };
+struct flow_actions actions = { .cnt = 0 };
[Darrell] I understand your good point about succinctness.
Prior art seems to be towards showing all used members because
it makes it clear nothing was missed and initializing to zero
was intentional.
> +struct rte_flow *flow;
> +struct rte_flow_error error;
> +uint8_t *ipv4_next_proto_mask = NULL;
> +int ret = 0;
> +
> +/* Eth */
> +struct rte_flow_item_eth eth_spec;
> +struct rte_flow_item_eth eth_mask;
Empty line here please.
> +memset(ð_mask, 0, sizeof(eth_mask));
> +if (match->wc.masks.dl_src.be16[0] ||
> +match->wc.masks.dl_src.be16[1] ||
> +match->wc.masks.dl_src.be16[2] ||
> +match->wc.masks.dl_dst.be16[0] ||
> +match->wc.masks.dl_dst.be16[1] ||
> +match->wc.masks.dl_dst.be16[2]) {
It looks like eth_addr_is_zero() can be used in the above.
> +rte_memcpy(ð_spec.dst, &match->flow.dl_dst,
sizeof(eth_spec.dst));
> +rte_memcpy(ð_spec.src, &match->flow.dl_src,
sizeof(eth_spec.src));
> +eth_spec.type = match->flow.dl_type;
> +
> +rte_memcpy(ð_mask.dst, &match->wc.masks.dl_dst,
> + sizeof(eth_mask.dst));
> +rte_memcpy(ð_mask.src, &match->wc.masks.dl_src,
> + sizeof(eth_mask.src));
> +eth_mask.type = match->wc.masks.dl_type;
> +
> +add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_ETH,
> + ð_spec, ð_mask);
> +} else {
> +/*
> + * If user specifies a flow (like UDP flow) without L2 patterns,
> + * OVS will at least set the dl_type. Normally, it's enough to
> + * create an eth pattern just with it. Unluckily, some Intel's
> + * NIC (such as XL710) doesn't support that. Below is a
workaround,
> + * which simply matches any L2 pkts.
> + */
> +add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_ETH, NULL, NULL);
> +}
This feels a lot like a layer violation - making the core aware
of specific implementation details of lower layers.
From a functional point of view, is the idea that
a eth_type+5-tuple match is converted to a 5-tuple match?
> +
> +/* VLAN */
> +struct rte_flow_item_vlan vlan_spec;
> +struct rte_flow_item_vlan vlan_mask;
Please declare all local variables at the top of the context
(in this case function).
[Darrell] In OVS, we try to keep the variable declarations as close to their
usage as possible, including inside condition checks when
appropriate.
This is easily forgotten and sometimes missed in review.
...
> +struct rte_flow_item_udp udp_spec;
> +struct rte_flow_item_udp udp_mask;
> +memset(&udp_mask, 0, sizeof(udp_mask));
> +if ((proto == IPPROTO_UDP) &&
> +(match->wc.masks.tp_src || match->wc.masks.tp_dst)) {
> +udp_spec.hdr.src_port = match->flow.tp_src;
> +udp_spec.hdr.dst_port = match->flow.tp_dst;
> +
> +udp_mask.hdr.src_port = match->wc.masks.tp_src;
> +udp_mask.hdr.dst_port = match-
Re: [ovs-dev] [PATCH v2 4/8] netdev-dpdk: implement flow put with rte flow
On Tue, Sep 05, 2017 at 05:22:57PM +0800, Yuanhan Liu wrote:
> From: Finn Christensen
>
> The basic yet the major part of this patch is to translate the "match"
> to rte flow patterns. And then, we create a rte flow with a MARK action.
> Afterwards, all pkts matches the flow will have the mark id in the mbuf.
>
> For any unsupported flows, such as MPLS, -1 is returned, meaning the
> flow offload is failed and then skipped.
...
> +static int
> +netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
> + const struct match *match,
> + struct nlattr *nl_actions OVS_UNUSED,
> + size_t actions_len,
> + const ovs_u128 *ufid,
> + struct offload_info *info)
> +{
> +struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> +const struct rte_flow_attr flow_attr = {
> +.group = 0,
> +.priority = 0,
> +.ingress = 1,
> +.egress = 0
> +};
> +struct flow_patterns patterns = { .items = NULL, .cnt = 0 };
> +struct flow_actions actions = { .actions = NULL, .cnt = 0 };
I believe the following will give the same result as the above,
less verbosely.
+const struct rte_flow_attr flow_attr = { .ingress = 1 };
+struct flow_patterns patterns = { .cnt = 0 };
+struct flow_actions actions = { .cnt = 0 };
> +struct rte_flow *flow;
> +struct rte_flow_error error;
> +uint8_t *ipv4_next_proto_mask = NULL;
> +int ret = 0;
> +
> +/* Eth */
> +struct rte_flow_item_eth eth_spec;
> +struct rte_flow_item_eth eth_mask;
Empty line here please.
> +memset(ð_mask, 0, sizeof(eth_mask));
> +if (match->wc.masks.dl_src.be16[0] ||
> +match->wc.masks.dl_src.be16[1] ||
> +match->wc.masks.dl_src.be16[2] ||
> +match->wc.masks.dl_dst.be16[0] ||
> +match->wc.masks.dl_dst.be16[1] ||
> +match->wc.masks.dl_dst.be16[2]) {
It looks like eth_addr_is_zero() can be used in the above.
> +rte_memcpy(ð_spec.dst, &match->flow.dl_dst, sizeof(eth_spec.dst));
> +rte_memcpy(ð_spec.src, &match->flow.dl_src, sizeof(eth_spec.src));
> +eth_spec.type = match->flow.dl_type;
> +
> +rte_memcpy(ð_mask.dst, &match->wc.masks.dl_dst,
> + sizeof(eth_mask.dst));
> +rte_memcpy(ð_mask.src, &match->wc.masks.dl_src,
> + sizeof(eth_mask.src));
> +eth_mask.type = match->wc.masks.dl_type;
> +
> +add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_ETH,
> + ð_spec, ð_mask);
> +} else {
> +/*
> + * If user specifies a flow (like UDP flow) without L2 patterns,
> + * OVS will at least set the dl_type. Normally, it's enough to
> + * create an eth pattern just with it. Unluckily, some Intel's
> + * NIC (such as XL710) doesn't support that. Below is a workaround,
> + * which simply matches any L2 pkts.
> + */
> +add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_ETH, NULL, NULL);
> +}
This feels a lot like a layer violation - making the core aware
of specific implementation details of lower layers.
>From a functional point of view, is the idea that
a eth_type+5-tuple match is converted to a 5-tuple match?
> +
> +/* VLAN */
> +struct rte_flow_item_vlan vlan_spec;
> +struct rte_flow_item_vlan vlan_mask;
Please declare all local variables at the top of the context
(in this case function).
...
> +struct rte_flow_item_udp udp_spec;
> +struct rte_flow_item_udp udp_mask;
> +memset(&udp_mask, 0, sizeof(udp_mask));
> +if ((proto == IPPROTO_UDP) &&
> +(match->wc.masks.tp_src || match->wc.masks.tp_dst)) {
> +udp_spec.hdr.src_port = match->flow.tp_src;
> +udp_spec.hdr.dst_port = match->flow.tp_dst;
> +
> +udp_mask.hdr.src_port = match->wc.masks.tp_src;
> +udp_mask.hdr.dst_port = match->wc.masks.tp_dst;
> +
> +add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_UDP,
> + &udp_spec, &udp_mask);
> +
> +/* proto == UDP and ITEM_TYPE_UDP, thus no need for proto match */
> +if (ipv4_next_proto_mask) {
> +*ipv4_next_proto_mask = 0;
I think this should be:
+*ipv4_next_proto_mask = NULL;
> +}
> +}
> +
> +struct rte_flow_item_sctp sctp_spec;
> +struct rte_flow_item_sctp sctp_mask;
> +memset(&sctp_mask, 0, sizeof(sctp_mask));
> +if ((proto == IPPROTO_SCTP) &&
It seems there are unnecessary () in the line above.
...
> +/*
> + * Validate for later rte flow offload creation. If any unsupported
> + * flow are specified, return -1.
> + */
> +static int
> +netdev_dpdk_validate_flow(const struct match *match)
> +{
> +struct match match_zero_wc;
> +
> +/* Create a wc-zeroed version of flow */
> +match_init(&match_zero_wc, &match->flow, &match->wc);
> +
> +#define CHECK_NONZE
