Re: [ovs-dev] [PATCH V2 13/14] netdev-offload-dpdk: Support vports flows offload
On 3/1/2021 1:30 PM, Sriharsha Basavapatna wrote:
On Thu, Feb 25, 2021 at 7:51 PM Eli Britstein wrote:
On 2/25/2021 9:35 AM, Sriharsha Basavapatna wrote:
On Wed, Feb 24, 2021 at 4:50 PM Eli Britstein wrote:
On 2/24/2021 12:37 PM, Sriharsha Basavapatna wrote:
On Wed, Feb 10, 2021 at 8:57 PM Eli Britstein wrote:
Vports are virtual, OVS only logical devices, so rte_flows cannot be
applied as is on them. Instead, apply the rules the physical port from
which the packet has arrived, provided by orig_in_port field.
Signed-off-by: Eli Britstein
Reviewed-by: Gaetan Rivet
---
lib/netdev-offload-dpdk.c | 169 ++
1 file changed, 137 insertions(+), 32 deletions(-)
diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
index f6e668bff..ad47d717f 100644
--- a/lib/netdev-offload-dpdk.c
+++ b/lib/netdev-offload-dpdk.c
@@ -62,6 +62,7 @@ struct ufid_to_rte_flow_data {
struct rte_flow *rte_flow;
bool actions_offloaded;
struct dpif_flow_stats stats;
+struct netdev *physdev;
};
/* Find rte_flow with @ufid. */
@@ -87,7 +88,8 @@ ufid_to_rte_flow_data_find(const ovs_u128 *ufid, bool warn)
static inline struct ufid_to_rte_flow_data *
ufid_to_rte_flow_associate(const ovs_u128 *ufid, struct netdev *netdev,
- struct rte_flow *rte_flow, bool actions_offloaded)
+ struct netdev *vport, struct rte_flow *rte_flow,
+ bool actions_offloaded)
{
size_t hash = hash_bytes(ufid, sizeof *ufid, 0);
struct ufid_to_rte_flow_data *data = xzalloc(sizeof *data);
@@ -105,7 +107,8 @@ ufid_to_rte_flow_associate(const ovs_u128 *ufid, struct
netdev *netdev,
}
data->ufid = *ufid;
-data->netdev = netdev_ref(netdev);
+data->netdev = vport ? netdev_ref(vport) : netdev_ref(netdev);
+data->physdev = netdev_ref(netdev);
For non-tunnel offloads, we end up getting two references to the same
'netdev'; can we avoid this ? That is, get a reference to physdev only
for the vport case.
I know. This is on purpose. It simplifies other places, for example
query, to always use physdev, and always close both without any
additional logic there.
Ok, please add this as an inline comment so we know why it is done
this way. Also, you missed my last comment in this patch (see
VXLAN_DECAP action below).
OK.
Sorry, see below.
data->rte_flow = rte_flow;
data->actions_offloaded = actions_offloaded;
@@ -122,6 +125,7 @@ ufid_to_rte_flow_disassociate(struct ufid_to_rte_flow_data
*data)
cmap_remove(&ufid_to_rte_flow,
CONST_CAST(struct cmap_node *, &data->node), hash);
netdev_close(data->netdev);
+netdev_close(data->physdev);
Similar comment, release reference to physdev only if we got a
reference earlier (i.e., physdev should be non-null only when netdev
is a vport).
Right. As it is written this way, no need for any additional logic here.
ovsrcu_postpone(free, data);
}
@@ -134,6 +138,8 @@ struct flow_patterns {
struct rte_flow_item *items;
int cnt;
int current_max;
+uint32_t num_of_tnl_items;
change to --> num_pmd_tnl_items
OK.
+struct ds s_tnl;
};
struct flow_actions {
@@ -154,16 +160,20 @@ struct flow_actions {
static void
dump_flow_attr(struct ds *s, struct ds *s_extra,
const struct rte_flow_attr *attr,
+ struct flow_patterns *flow_patterns,
struct flow_actions *flow_actions)
{
if (flow_actions->num_of_tnl_actions) {
ds_clone(s_extra, &flow_actions->s_tnl);
+} else if (flow_patterns->num_of_tnl_items) {
+ds_clone(s_extra, &flow_patterns->s_tnl);
}
-ds_put_format(s, "%s%spriority %"PRIu32" group %"PRIu32" %s%s",
+ds_put_format(s, "%s%spriority %"PRIu32" group %"PRIu32" %s%s%s",
attr->ingress ? "ingress " : "",
attr->egress ? "egress " : "", attr->priority,
attr->group,
attr->transfer ? "transfer " : "",
- flow_actions->num_of_tnl_actions ? "tunnel_set 1 " : "");
+ flow_actions->num_of_tnl_actions ? "tunnel_set 1 " : "",
+ flow_patterns->num_of_tnl_items ? "tunnel_match 1 " : "");
}
/* Adds one pattern item 'field' with the 'mask' to dynamic string 's' using
@@ -177,9 +187,18 @@ dump_flow_attr(struct ds *s, struct ds *s_extra,
}
static void
-dump_flow_pattern(struct ds *s, const struct rte_flow_item *item)
+dump_flow_pattern(struct ds *s,
+ struct flow_patterns *flow_patterns,
+ int pattern_index)
{
-if (item->type == RTE_FLOW_ITEM_TYPE_ETH) {
+const struct rte_flow_item *item = &flow_patterns->items[pattern_index];
+
+if (item->type == RTE_FLOW_ITEM_TYPE_END) {
+ds_put_cstr(s, "end ");
+} else if (flow_patterns-
Re: [ovs-dev] [PATCH V2 13/14] netdev-offload-dpdk: Support vports flows offload
On Thu, Feb 25, 2021 at 7:51 PM Eli Britstein wrote:
>
>
> On 2/25/2021 9:35 AM, Sriharsha Basavapatna wrote:
> > On Wed, Feb 24, 2021 at 4:50 PM Eli Britstein wrote:
> >>
> >> On 2/24/2021 12:37 PM, Sriharsha Basavapatna wrote:
> >>> On Wed, Feb 10, 2021 at 8:57 PM Eli Britstein wrote:
> Vports are virtual, OVS only logical devices, so rte_flows cannot be
> applied as is on them. Instead, apply the rules the physical port from
> which the packet has arrived, provided by orig_in_port field.
>
> Signed-off-by: Eli Britstein
> Reviewed-by: Gaetan Rivet
> ---
> lib/netdev-offload-dpdk.c | 169 ++
> 1 file changed, 137 insertions(+), 32 deletions(-)
>
> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
> index f6e668bff..ad47d717f 100644
> --- a/lib/netdev-offload-dpdk.c
> +++ b/lib/netdev-offload-dpdk.c
> @@ -62,6 +62,7 @@ struct ufid_to_rte_flow_data {
> struct rte_flow *rte_flow;
> bool actions_offloaded;
> struct dpif_flow_stats stats;
> +struct netdev *physdev;
> };
>
> /* Find rte_flow with @ufid. */
> @@ -87,7 +88,8 @@ ufid_to_rte_flow_data_find(const ovs_u128 *ufid, bool
> warn)
>
> static inline struct ufid_to_rte_flow_data *
> ufid_to_rte_flow_associate(const ovs_u128 *ufid, struct netdev
> *netdev,
> - struct rte_flow *rte_flow, bool
> actions_offloaded)
> + struct netdev *vport, struct rte_flow
> *rte_flow,
> + bool actions_offloaded)
> {
> size_t hash = hash_bytes(ufid, sizeof *ufid, 0);
> struct ufid_to_rte_flow_data *data = xzalloc(sizeof *data);
> @@ -105,7 +107,8 @@ ufid_to_rte_flow_associate(const ovs_u128 *ufid,
> struct netdev *netdev,
> }
>
> data->ufid = *ufid;
> -data->netdev = netdev_ref(netdev);
> +data->netdev = vport ? netdev_ref(vport) : netdev_ref(netdev);
> +data->physdev = netdev_ref(netdev);
> >>> For non-tunnel offloads, we end up getting two references to the same
> >>> 'netdev'; can we avoid this ? That is, get a reference to physdev only
> >>> for the vport case.
> >> I know. This is on purpose. It simplifies other places, for example
> >> query, to always use physdev, and always close both without any
> >> additional logic there.
> > Ok, please add this as an inline comment so we know why it is done
> > this way. Also, you missed my last comment in this patch (see
> > VXLAN_DECAP action below).
>
> OK.
>
> Sorry, see below.
>
> >
> >
> data->rte_flow = rte_flow;
> data->actions_offloaded = actions_offloaded;
>
> @@ -122,6 +125,7 @@ ufid_to_rte_flow_disassociate(struct
> ufid_to_rte_flow_data *data)
> cmap_remove(&ufid_to_rte_flow,
> CONST_CAST(struct cmap_node *, &data->node), hash);
> netdev_close(data->netdev);
> +netdev_close(data->physdev);
> >>> Similar comment, release reference to physdev only if we got a
> >>> reference earlier (i.e., physdev should be non-null only when netdev
> >>> is a vport).
> >> Right. As it is written this way, no need for any additional logic here.
> ovsrcu_postpone(free, data);
> }
>
> @@ -134,6 +138,8 @@ struct flow_patterns {
> struct rte_flow_item *items;
> int cnt;
> int current_max;
> +uint32_t num_of_tnl_items;
> >>> change to --> num_pmd_tnl_items
> >> OK.
> +struct ds s_tnl;
> };
>
> struct flow_actions {
> @@ -154,16 +160,20 @@ struct flow_actions {
> static void
> dump_flow_attr(struct ds *s, struct ds *s_extra,
> const struct rte_flow_attr *attr,
> + struct flow_patterns *flow_patterns,
> struct flow_actions *flow_actions)
> {
> if (flow_actions->num_of_tnl_actions) {
> ds_clone(s_extra, &flow_actions->s_tnl);
> +} else if (flow_patterns->num_of_tnl_items) {
> +ds_clone(s_extra, &flow_patterns->s_tnl);
> }
> -ds_put_format(s, "%s%spriority %"PRIu32" group %"PRIu32" %s%s",
> +ds_put_format(s, "%s%spriority %"PRIu32" group %"PRIu32" %s%s%s",
> attr->ingress ? "ingress " : "",
> attr->egress ? "egress " : "", attr->priority,
> attr->group,
> attr->transfer ? "transfer " : "",
> - flow_actions->num_of_tnl_actions ? "tunnel_set 1 " :
> "");
> + flow_actions->num_of_tnl_actions ? "tunnel_set 1 " :
> "",
> + flow_patterns->num_of_tnl_items ? "tunnel_match 1 " :
> >>>
Re: [ovs-dev] [PATCH V2 13/14] netdev-offload-dpdk: Support vports flows offload
On 2/25/2021 9:35 AM, Sriharsha Basavapatna wrote:
On Wed, Feb 24, 2021 at 4:50 PM Eli Britstein wrote:
On 2/24/2021 12:37 PM, Sriharsha Basavapatna wrote:
On Wed, Feb 10, 2021 at 8:57 PM Eli Britstein wrote:
Vports are virtual, OVS only logical devices, so rte_flows cannot be
applied as is on them. Instead, apply the rules the physical port from
which the packet has arrived, provided by orig_in_port field.
Signed-off-by: Eli Britstein
Reviewed-by: Gaetan Rivet
---
lib/netdev-offload-dpdk.c | 169 ++
1 file changed, 137 insertions(+), 32 deletions(-)
diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
index f6e668bff..ad47d717f 100644
--- a/lib/netdev-offload-dpdk.c
+++ b/lib/netdev-offload-dpdk.c
@@ -62,6 +62,7 @@ struct ufid_to_rte_flow_data {
struct rte_flow *rte_flow;
bool actions_offloaded;
struct dpif_flow_stats stats;
+struct netdev *physdev;
};
/* Find rte_flow with @ufid. */
@@ -87,7 +88,8 @@ ufid_to_rte_flow_data_find(const ovs_u128 *ufid, bool warn)
static inline struct ufid_to_rte_flow_data *
ufid_to_rte_flow_associate(const ovs_u128 *ufid, struct netdev *netdev,
- struct rte_flow *rte_flow, bool actions_offloaded)
+ struct netdev *vport, struct rte_flow *rte_flow,
+ bool actions_offloaded)
{
size_t hash = hash_bytes(ufid, sizeof *ufid, 0);
struct ufid_to_rte_flow_data *data = xzalloc(sizeof *data);
@@ -105,7 +107,8 @@ ufid_to_rte_flow_associate(const ovs_u128 *ufid, struct
netdev *netdev,
}
data->ufid = *ufid;
-data->netdev = netdev_ref(netdev);
+data->netdev = vport ? netdev_ref(vport) : netdev_ref(netdev);
+data->physdev = netdev_ref(netdev);
For non-tunnel offloads, we end up getting two references to the same
'netdev'; can we avoid this ? That is, get a reference to physdev only
for the vport case.
I know. This is on purpose. It simplifies other places, for example
query, to always use physdev, and always close both without any
additional logic there.
Ok, please add this as an inline comment so we know why it is done
this way. Also, you missed my last comment in this patch (see
VXLAN_DECAP action below).
OK.
Sorry, see below.
data->rte_flow = rte_flow;
data->actions_offloaded = actions_offloaded;
@@ -122,6 +125,7 @@ ufid_to_rte_flow_disassociate(struct ufid_to_rte_flow_data
*data)
cmap_remove(&ufid_to_rte_flow,
CONST_CAST(struct cmap_node *, &data->node), hash);
netdev_close(data->netdev);
+netdev_close(data->physdev);
Similar comment, release reference to physdev only if we got a
reference earlier (i.e., physdev should be non-null only when netdev
is a vport).
Right. As it is written this way, no need for any additional logic here.
ovsrcu_postpone(free, data);
}
@@ -134,6 +138,8 @@ struct flow_patterns {
struct rte_flow_item *items;
int cnt;
int current_max;
+uint32_t num_of_tnl_items;
change to --> num_pmd_tnl_items
OK.
+struct ds s_tnl;
};
struct flow_actions {
@@ -154,16 +160,20 @@ struct flow_actions {
static void
dump_flow_attr(struct ds *s, struct ds *s_extra,
const struct rte_flow_attr *attr,
+ struct flow_patterns *flow_patterns,
struct flow_actions *flow_actions)
{
if (flow_actions->num_of_tnl_actions) {
ds_clone(s_extra, &flow_actions->s_tnl);
+} else if (flow_patterns->num_of_tnl_items) {
+ds_clone(s_extra, &flow_patterns->s_tnl);
}
-ds_put_format(s, "%s%spriority %"PRIu32" group %"PRIu32" %s%s",
+ds_put_format(s, "%s%spriority %"PRIu32" group %"PRIu32" %s%s%s",
attr->ingress ? "ingress " : "",
attr->egress ? "egress " : "", attr->priority,
attr->group,
attr->transfer ? "transfer " : "",
- flow_actions->num_of_tnl_actions ? "tunnel_set 1 " : "");
+ flow_actions->num_of_tnl_actions ? "tunnel_set 1 " : "",
+ flow_patterns->num_of_tnl_items ? "tunnel_match 1 " : "");
}
/* Adds one pattern item 'field' with the 'mask' to dynamic string 's' using
@@ -177,9 +187,18 @@ dump_flow_attr(struct ds *s, struct ds *s_extra,
}
static void
-dump_flow_pattern(struct ds *s, const struct rte_flow_item *item)
+dump_flow_pattern(struct ds *s,
+ struct flow_patterns *flow_patterns,
+ int pattern_index)
{
-if (item->type == RTE_FLOW_ITEM_TYPE_ETH) {
+const struct rte_flow_item *item = &flow_patterns->items[pattern_index];
+
+if (item->type == RTE_FLOW_ITEM_TYPE_END) {
+ds_put_cstr(s, "end ");
+} else if (flow_patterns->num_of_tnl_items &&
+ pattern_index < flow_patterns->num_of_tnl_items) {
+return;
+} else if (item->type == RTE_FLOW_IT
Re: [ovs-dev] [PATCH V2 13/14] netdev-offload-dpdk: Support vports flows offload
On Wed, Feb 24, 2021 at 4:50 PM Eli Britstein wrote:
>
>
> On 2/24/2021 12:37 PM, Sriharsha Basavapatna wrote:
> > On Wed, Feb 10, 2021 at 8:57 PM Eli Britstein wrote:
> >> Vports are virtual, OVS only logical devices, so rte_flows cannot be
> >> applied as is on them. Instead, apply the rules the physical port from
> >> which the packet has arrived, provided by orig_in_port field.
> >>
> >> Signed-off-by: Eli Britstein
> >> Reviewed-by: Gaetan Rivet
> >> ---
> >> lib/netdev-offload-dpdk.c | 169 ++
> >> 1 file changed, 137 insertions(+), 32 deletions(-)
> >>
> >> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
> >> index f6e668bff..ad47d717f 100644
> >> --- a/lib/netdev-offload-dpdk.c
> >> +++ b/lib/netdev-offload-dpdk.c
> >> @@ -62,6 +62,7 @@ struct ufid_to_rte_flow_data {
> >> struct rte_flow *rte_flow;
> >> bool actions_offloaded;
> >> struct dpif_flow_stats stats;
> >> +struct netdev *physdev;
> >> };
> >>
> >> /* Find rte_flow with @ufid. */
> >> @@ -87,7 +88,8 @@ ufid_to_rte_flow_data_find(const ovs_u128 *ufid, bool
> >> warn)
> >>
> >> static inline struct ufid_to_rte_flow_data *
> >> ufid_to_rte_flow_associate(const ovs_u128 *ufid, struct netdev *netdev,
> >> - struct rte_flow *rte_flow, bool
> >> actions_offloaded)
> >> + struct netdev *vport, struct rte_flow
> >> *rte_flow,
> >> + bool actions_offloaded)
> >> {
> >> size_t hash = hash_bytes(ufid, sizeof *ufid, 0);
> >> struct ufid_to_rte_flow_data *data = xzalloc(sizeof *data);
> >> @@ -105,7 +107,8 @@ ufid_to_rte_flow_associate(const ovs_u128 *ufid,
> >> struct netdev *netdev,
> >> }
> >>
> >> data->ufid = *ufid;
> >> -data->netdev = netdev_ref(netdev);
> >> +data->netdev = vport ? netdev_ref(vport) : netdev_ref(netdev);
> >> +data->physdev = netdev_ref(netdev);
> > For non-tunnel offloads, we end up getting two references to the same
> > 'netdev'; can we avoid this ? That is, get a reference to physdev only
> > for the vport case.
> I know. This is on purpose. It simplifies other places, for example
> query, to always use physdev, and always close both without any
> additional logic there.
Ok, please add this as an inline comment so we know why it is done
this way. Also, you missed my last comment in this patch (see
VXLAN_DECAP action below).
> >> data->rte_flow = rte_flow;
> >> data->actions_offloaded = actions_offloaded;
> >>
> >> @@ -122,6 +125,7 @@ ufid_to_rte_flow_disassociate(struct
> >> ufid_to_rte_flow_data *data)
> >> cmap_remove(&ufid_to_rte_flow,
> >> CONST_CAST(struct cmap_node *, &data->node), hash);
> >> netdev_close(data->netdev);
> >> +netdev_close(data->physdev);
> > Similar comment, release reference to physdev only if we got a
> > reference earlier (i.e., physdev should be non-null only when netdev
> > is a vport).
> Right. As it is written this way, no need for any additional logic here.
> >> ovsrcu_postpone(free, data);
> >> }
> >>
> >> @@ -134,6 +138,8 @@ struct flow_patterns {
> >> struct rte_flow_item *items;
> >> int cnt;
> >> int current_max;
> >> +uint32_t num_of_tnl_items;
> > change to --> num_pmd_tnl_items
> OK.
> >> +struct ds s_tnl;
> >> };
> >>
> >> struct flow_actions {
> >> @@ -154,16 +160,20 @@ struct flow_actions {
> >> static void
> >> dump_flow_attr(struct ds *s, struct ds *s_extra,
> >> const struct rte_flow_attr *attr,
> >> + struct flow_patterns *flow_patterns,
> >> struct flow_actions *flow_actions)
> >> {
> >> if (flow_actions->num_of_tnl_actions) {
> >> ds_clone(s_extra, &flow_actions->s_tnl);
> >> +} else if (flow_patterns->num_of_tnl_items) {
> >> +ds_clone(s_extra, &flow_patterns->s_tnl);
> >> }
> >> -ds_put_format(s, "%s%spriority %"PRIu32" group %"PRIu32" %s%s",
> >> +ds_put_format(s, "%s%spriority %"PRIu32" group %"PRIu32" %s%s%s",
> >> attr->ingress ? "ingress " : "",
> >> attr->egress ? "egress " : "", attr->priority,
> >> attr->group,
> >> attr->transfer ? "transfer " : "",
> >> - flow_actions->num_of_tnl_actions ? "tunnel_set 1 " :
> >> "");
> >> + flow_actions->num_of_tnl_actions ? "tunnel_set 1 " : "",
> >> + flow_patterns->num_of_tnl_items ? "tunnel_match 1 " :
> >> "");
> >> }
> >>
> >> /* Adds one pattern item 'field' with the 'mask' to dynamic string 's'
> >> using
> >> @@ -177,9 +187,18 @@ dump_flow_attr(struct ds *s, struct ds *s_extra,
> >> }
> >>
> >> static void
> >> -dump_flow_pattern(struct ds *s, const struct rte_flow_item *item)
> >> +dump_flow_pattern(struct ds *s,
> >> + struct flow_patterns *flow_patterns,
> >> + int patt
Re: [ovs-dev] [PATCH V2 13/14] netdev-offload-dpdk: Support vports flows offload
On 2/24/2021 12:37 PM, Sriharsha Basavapatna wrote:
On Wed, Feb 10, 2021 at 8:57 PM Eli Britstein wrote:
Vports are virtual, OVS only logical devices, so rte_flows cannot be
applied as is on them. Instead, apply the rules the physical port from
which the packet has arrived, provided by orig_in_port field.
Signed-off-by: Eli Britstein
Reviewed-by: Gaetan Rivet
---
lib/netdev-offload-dpdk.c | 169 ++
1 file changed, 137 insertions(+), 32 deletions(-)
diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
index f6e668bff..ad47d717f 100644
--- a/lib/netdev-offload-dpdk.c
+++ b/lib/netdev-offload-dpdk.c
@@ -62,6 +62,7 @@ struct ufid_to_rte_flow_data {
struct rte_flow *rte_flow;
bool actions_offloaded;
struct dpif_flow_stats stats;
+struct netdev *physdev;
};
/* Find rte_flow with @ufid. */
@@ -87,7 +88,8 @@ ufid_to_rte_flow_data_find(const ovs_u128 *ufid, bool warn)
static inline struct ufid_to_rte_flow_data *
ufid_to_rte_flow_associate(const ovs_u128 *ufid, struct netdev *netdev,
- struct rte_flow *rte_flow, bool actions_offloaded)
+ struct netdev *vport, struct rte_flow *rte_flow,
+ bool actions_offloaded)
{
size_t hash = hash_bytes(ufid, sizeof *ufid, 0);
struct ufid_to_rte_flow_data *data = xzalloc(sizeof *data);
@@ -105,7 +107,8 @@ ufid_to_rte_flow_associate(const ovs_u128 *ufid, struct
netdev *netdev,
}
data->ufid = *ufid;
-data->netdev = netdev_ref(netdev);
+data->netdev = vport ? netdev_ref(vport) : netdev_ref(netdev);
+data->physdev = netdev_ref(netdev);
For non-tunnel offloads, we end up getting two references to the same
'netdev'; can we avoid this ? That is, get a reference to physdev only
for the vport case.
I know. This is on purpose. It simplifies other places, for example
query, to always use physdev, and always close both without any
additional logic there.
data->rte_flow = rte_flow;
data->actions_offloaded = actions_offloaded;
@@ -122,6 +125,7 @@ ufid_to_rte_flow_disassociate(struct ufid_to_rte_flow_data
*data)
cmap_remove(&ufid_to_rte_flow,
CONST_CAST(struct cmap_node *, &data->node), hash);
netdev_close(data->netdev);
+netdev_close(data->physdev);
Similar comment, release reference to physdev only if we got a
reference earlier (i.e., physdev should be non-null only when netdev
is a vport).
Right. As it is written this way, no need for any additional logic here.
ovsrcu_postpone(free, data);
}
@@ -134,6 +138,8 @@ struct flow_patterns {
struct rte_flow_item *items;
int cnt;
int current_max;
+uint32_t num_of_tnl_items;
change to --> num_pmd_tnl_items
OK.
+struct ds s_tnl;
};
struct flow_actions {
@@ -154,16 +160,20 @@ struct flow_actions {
static void
dump_flow_attr(struct ds *s, struct ds *s_extra,
const struct rte_flow_attr *attr,
+ struct flow_patterns *flow_patterns,
struct flow_actions *flow_actions)
{
if (flow_actions->num_of_tnl_actions) {
ds_clone(s_extra, &flow_actions->s_tnl);
+} else if (flow_patterns->num_of_tnl_items) {
+ds_clone(s_extra, &flow_patterns->s_tnl);
}
-ds_put_format(s, "%s%spriority %"PRIu32" group %"PRIu32" %s%s",
+ds_put_format(s, "%s%spriority %"PRIu32" group %"PRIu32" %s%s%s",
attr->ingress ? "ingress " : "",
attr->egress ? "egress " : "", attr->priority,
attr->group,
attr->transfer ? "transfer " : "",
- flow_actions->num_of_tnl_actions ? "tunnel_set 1 " : "");
+ flow_actions->num_of_tnl_actions ? "tunnel_set 1 " : "",
+ flow_patterns->num_of_tnl_items ? "tunnel_match 1 " : "");
}
/* Adds one pattern item 'field' with the 'mask' to dynamic string 's' using
@@ -177,9 +187,18 @@ dump_flow_attr(struct ds *s, struct ds *s_extra,
}
static void
-dump_flow_pattern(struct ds *s, const struct rte_flow_item *item)
+dump_flow_pattern(struct ds *s,
+ struct flow_patterns *flow_patterns,
+ int pattern_index)
{
-if (item->type == RTE_FLOW_ITEM_TYPE_ETH) {
+const struct rte_flow_item *item = &flow_patterns->items[pattern_index];
+
+if (item->type == RTE_FLOW_ITEM_TYPE_END) {
+ds_put_cstr(s, "end ");
+} else if (flow_patterns->num_of_tnl_items &&
+ pattern_index < flow_patterns->num_of_tnl_items) {
+return;
+} else if (item->type == RTE_FLOW_ITEM_TYPE_ETH) {
const struct rte_flow_item_eth *eth_spec = item->spec;
const struct rte_flow_item_eth *eth_mask = item->mask;
@@ -569,19 +588,19 @@ dump_flow_action(struct ds *s, struct ds *s_extra,
static struct ds *
dump_flow(struct ds *s, struct ds *s_extra,
const struct rte_flow_attr *attr,
-
Re: [ovs-dev] [PATCH V2 13/14] netdev-offload-dpdk: Support vports flows offload
On Wed, Feb 10, 2021 at 8:57 PM Eli Britstein wrote:
>
> Vports are virtual, OVS only logical devices, so rte_flows cannot be
> applied as is on them. Instead, apply the rules the physical port from
> which the packet has arrived, provided by orig_in_port field.
>
> Signed-off-by: Eli Britstein
> Reviewed-by: Gaetan Rivet
> ---
> lib/netdev-offload-dpdk.c | 169 ++
> 1 file changed, 137 insertions(+), 32 deletions(-)
>
> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
> index f6e668bff..ad47d717f 100644
> --- a/lib/netdev-offload-dpdk.c
> +++ b/lib/netdev-offload-dpdk.c
> @@ -62,6 +62,7 @@ struct ufid_to_rte_flow_data {
> struct rte_flow *rte_flow;
> bool actions_offloaded;
> struct dpif_flow_stats stats;
> +struct netdev *physdev;
> };
>
> /* Find rte_flow with @ufid. */
> @@ -87,7 +88,8 @@ ufid_to_rte_flow_data_find(const ovs_u128 *ufid, bool warn)
>
> static inline struct ufid_to_rte_flow_data *
> ufid_to_rte_flow_associate(const ovs_u128 *ufid, struct netdev *netdev,
> - struct rte_flow *rte_flow, bool actions_offloaded)
> + struct netdev *vport, struct rte_flow *rte_flow,
> + bool actions_offloaded)
> {
> size_t hash = hash_bytes(ufid, sizeof *ufid, 0);
> struct ufid_to_rte_flow_data *data = xzalloc(sizeof *data);
> @@ -105,7 +107,8 @@ ufid_to_rte_flow_associate(const ovs_u128 *ufid, struct
> netdev *netdev,
> }
>
> data->ufid = *ufid;
> -data->netdev = netdev_ref(netdev);
> +data->netdev = vport ? netdev_ref(vport) : netdev_ref(netdev);
> +data->physdev = netdev_ref(netdev);
For non-tunnel offloads, we end up getting two references to the same
'netdev'; can we avoid this ? That is, get a reference to physdev only
for the vport case.
> data->rte_flow = rte_flow;
> data->actions_offloaded = actions_offloaded;
>
> @@ -122,6 +125,7 @@ ufid_to_rte_flow_disassociate(struct
> ufid_to_rte_flow_data *data)
> cmap_remove(&ufid_to_rte_flow,
> CONST_CAST(struct cmap_node *, &data->node), hash);
> netdev_close(data->netdev);
> +netdev_close(data->physdev);
Similar comment, release reference to physdev only if we got a
reference earlier (i.e., physdev should be non-null only when netdev
is a vport).
> ovsrcu_postpone(free, data);
> }
>
> @@ -134,6 +138,8 @@ struct flow_patterns {
> struct rte_flow_item *items;
> int cnt;
> int current_max;
> +uint32_t num_of_tnl_items;
change to --> num_pmd_tnl_items
> +struct ds s_tnl;
> };
>
> struct flow_actions {
> @@ -154,16 +160,20 @@ struct flow_actions {
> static void
> dump_flow_attr(struct ds *s, struct ds *s_extra,
> const struct rte_flow_attr *attr,
> + struct flow_patterns *flow_patterns,
> struct flow_actions *flow_actions)
> {
> if (flow_actions->num_of_tnl_actions) {
> ds_clone(s_extra, &flow_actions->s_tnl);
> +} else if (flow_patterns->num_of_tnl_items) {
> +ds_clone(s_extra, &flow_patterns->s_tnl);
> }
> -ds_put_format(s, "%s%spriority %"PRIu32" group %"PRIu32" %s%s",
> +ds_put_format(s, "%s%spriority %"PRIu32" group %"PRIu32" %s%s%s",
>attr->ingress ? "ingress " : "",
>attr->egress ? "egress " : "", attr->priority,
> attr->group,
>attr->transfer ? "transfer " : "",
> - flow_actions->num_of_tnl_actions ? "tunnel_set 1 " : "");
> + flow_actions->num_of_tnl_actions ? "tunnel_set 1 " : "",
> + flow_patterns->num_of_tnl_items ? "tunnel_match 1 " : "");
> }
>
> /* Adds one pattern item 'field' with the 'mask' to dynamic string 's' using
> @@ -177,9 +187,18 @@ dump_flow_attr(struct ds *s, struct ds *s_extra,
> }
>
> static void
> -dump_flow_pattern(struct ds *s, const struct rte_flow_item *item)
> +dump_flow_pattern(struct ds *s,
> + struct flow_patterns *flow_patterns,
> + int pattern_index)
> {
> -if (item->type == RTE_FLOW_ITEM_TYPE_ETH) {
> +const struct rte_flow_item *item = &flow_patterns->items[pattern_index];
> +
> +if (item->type == RTE_FLOW_ITEM_TYPE_END) {
> +ds_put_cstr(s, "end ");
> +} else if (flow_patterns->num_of_tnl_items &&
> + pattern_index < flow_patterns->num_of_tnl_items) {
> +return;
> +} else if (item->type == RTE_FLOW_ITEM_TYPE_ETH) {
> const struct rte_flow_item_eth *eth_spec = item->spec;
> const struct rte_flow_item_eth *eth_mask = item->mask;
>
> @@ -569,19 +588,19 @@ dump_flow_action(struct ds *s, struct ds *s_extra,
> static struct ds *
> dump_flow(struct ds *s, struct ds *s_extra,
>const struct rte_flow_attr *attr,
> - const struct rte_flow_item *items,
> + struct flow_patterns *flow_patterns,
>struct flow_actions *flow_
[ovs-dev] [PATCH V2 13/14] netdev-offload-dpdk: Support vports flows offload
Vports are virtual, OVS only logical devices, so rte_flows cannot be
applied as is on them. Instead, apply the rules the physical port from
which the packet has arrived, provided by orig_in_port field.
Signed-off-by: Eli Britstein
Reviewed-by: Gaetan Rivet
---
lib/netdev-offload-dpdk.c | 169 ++
1 file changed, 137 insertions(+), 32 deletions(-)
diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
index f6e668bff..ad47d717f 100644
--- a/lib/netdev-offload-dpdk.c
+++ b/lib/netdev-offload-dpdk.c
@@ -62,6 +62,7 @@ struct ufid_to_rte_flow_data {
struct rte_flow *rte_flow;
bool actions_offloaded;
struct dpif_flow_stats stats;
+struct netdev *physdev;
};
/* Find rte_flow with @ufid. */
@@ -87,7 +88,8 @@ ufid_to_rte_flow_data_find(const ovs_u128 *ufid, bool warn)
static inline struct ufid_to_rte_flow_data *
ufid_to_rte_flow_associate(const ovs_u128 *ufid, struct netdev *netdev,
- struct rte_flow *rte_flow, bool actions_offloaded)
+ struct netdev *vport, struct rte_flow *rte_flow,
+ bool actions_offloaded)
{
size_t hash = hash_bytes(ufid, sizeof *ufid, 0);
struct ufid_to_rte_flow_data *data = xzalloc(sizeof *data);
@@ -105,7 +107,8 @@ ufid_to_rte_flow_associate(const ovs_u128 *ufid, struct
netdev *netdev,
}
data->ufid = *ufid;
-data->netdev = netdev_ref(netdev);
+data->netdev = vport ? netdev_ref(vport) : netdev_ref(netdev);
+data->physdev = netdev_ref(netdev);
data->rte_flow = rte_flow;
data->actions_offloaded = actions_offloaded;
@@ -122,6 +125,7 @@ ufid_to_rte_flow_disassociate(struct ufid_to_rte_flow_data
*data)
cmap_remove(&ufid_to_rte_flow,
CONST_CAST(struct cmap_node *, &data->node), hash);
netdev_close(data->netdev);
+netdev_close(data->physdev);
ovsrcu_postpone(free, data);
}
@@ -134,6 +138,8 @@ struct flow_patterns {
struct rte_flow_item *items;
int cnt;
int current_max;
+uint32_t num_of_tnl_items;
+struct ds s_tnl;
};
struct flow_actions {
@@ -154,16 +160,20 @@ struct flow_actions {
static void
dump_flow_attr(struct ds *s, struct ds *s_extra,
const struct rte_flow_attr *attr,
+ struct flow_patterns *flow_patterns,
struct flow_actions *flow_actions)
{
if (flow_actions->num_of_tnl_actions) {
ds_clone(s_extra, &flow_actions->s_tnl);
+} else if (flow_patterns->num_of_tnl_items) {
+ds_clone(s_extra, &flow_patterns->s_tnl);
}
-ds_put_format(s, "%s%spriority %"PRIu32" group %"PRIu32" %s%s",
+ds_put_format(s, "%s%spriority %"PRIu32" group %"PRIu32" %s%s%s",
attr->ingress ? "ingress " : "",
attr->egress ? "egress " : "", attr->priority, attr->group,
attr->transfer ? "transfer " : "",
- flow_actions->num_of_tnl_actions ? "tunnel_set 1 " : "");
+ flow_actions->num_of_tnl_actions ? "tunnel_set 1 " : "",
+ flow_patterns->num_of_tnl_items ? "tunnel_match 1 " : "");
}
/* Adds one pattern item 'field' with the 'mask' to dynamic string 's' using
@@ -177,9 +187,18 @@ dump_flow_attr(struct ds *s, struct ds *s_extra,
}
static void
-dump_flow_pattern(struct ds *s, const struct rte_flow_item *item)
+dump_flow_pattern(struct ds *s,
+ struct flow_patterns *flow_patterns,
+ int pattern_index)
{
-if (item->type == RTE_FLOW_ITEM_TYPE_ETH) {
+const struct rte_flow_item *item = &flow_patterns->items[pattern_index];
+
+if (item->type == RTE_FLOW_ITEM_TYPE_END) {
+ds_put_cstr(s, "end ");
+} else if (flow_patterns->num_of_tnl_items &&
+ pattern_index < flow_patterns->num_of_tnl_items) {
+return;
+} else if (item->type == RTE_FLOW_ITEM_TYPE_ETH) {
const struct rte_flow_item_eth *eth_spec = item->spec;
const struct rte_flow_item_eth *eth_mask = item->mask;
@@ -569,19 +588,19 @@ dump_flow_action(struct ds *s, struct ds *s_extra,
static struct ds *
dump_flow(struct ds *s, struct ds *s_extra,
const struct rte_flow_attr *attr,
- const struct rte_flow_item *items,
+ struct flow_patterns *flow_patterns,
struct flow_actions *flow_actions)
{
int i;
if (attr) {
-dump_flow_attr(s, s_extra, attr, flow_actions);
+dump_flow_attr(s, s_extra, attr, flow_patterns, flow_actions);
}
ds_put_cstr(s, "pattern ");
-while (items && items->type != RTE_FLOW_ITEM_TYPE_END) {
-dump_flow_pattern(s, items++);
+for (i = 0; i < flow_patterns->cnt; i++) {
+dump_flow_pattern(s, flow_patterns, i);
}
-ds_put_cstr(s, "end actions ");
+ds_put_cstr(s, "actions ");
for (i = 0; i < flow_actions->cnt; i++) {
dump_flow_action(s, s_extra, flow_actions, i);
}
@@ -59
