Re: [ovs-dev] [PATCH v3 3/3] netdev-offload-dpdk: use flow transfer proxy mechanism
Bleep bloop. Greetings Ivan Malov, I am a robot and I have tried out your
patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
WARNING: Line is 80 characters long (recommended limit is 79)
#55 FILE: lib/netdev-dpdk.c:1207:
>flow_transfer_proxy_port_id, NULL);
WARNING: Line is 80 characters long (recommended limit is 79)
#105 FILE: lib/netdev-dpdk.c:3891:
response = xasprintf("Device '%s' can not be detached (flow proxy)",
ERROR: Inappropriate bracing around statement
#112 FILE: lib/netdev-dpdk.c:3898:
if (dev_self != NULL)
WARNING: Line is 80 characters long (recommended limit is 79)
#134 FILE: lib/netdev-dpdk.c:5282:
ret = flow_transfer_proxy ? dev->flow_transfer_proxy_port_id : dev->port_id;
WARNING: Line is 80 characters long (recommended limit is 79)
#225 FILE: lib/netdev-dpdk.c:5469:
ret = rte_flow_tunnel_action_decap_release(dev->flow_transfer_proxy_port_id,
Lines checked: 436, Warnings: 4, Errors: 1
Please check this out. If you feel there has been an error, please email
acon...@redhat.com
Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 3/3] netdev-offload-dpdk: use flow transfer proxy mechanism
Hi Eli,
Thanks a lot for a prompt review. PSB.
On Wed, 20 Jul 2022, Eli Britstein wrote:
-Original Message-
From: Ivan Malov
Sent: Wednesday, July 20, 2022 3:18 PM
To: d...@openvswitch.org
Cc: Eli Britstein ; Stephen Hemminger
; Ilya Maximets ; Ori
Kam ; Maxime Coquelin
; David Marchand
; Andrew Rybchenko
Subject: [PATCH v3 3/3] netdev-offload-dpdk: use flow transfer proxy
mechanism
External email: Use caution opening links or attachments
Manage "transfer" flows via the corresponding mechanism.
Doing so requires that the traffic source be specified explicitly, via the
corresponding pattern item.
Signed-off-by: Ivan Malov
Acked-by: Andrew Rybchenko
---
lib/netdev-dpdk.c | 99 ---
lib/netdev-dpdk.h | 4 +-
lib/netdev-offload-dpdk.c | 61
3 files changed, 135 insertions(+), 29 deletions(-)
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
45e5d26d2..01fb40255 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -420,6 +420,7 @@ enum dpdk_hw_ol_features {
struct netdev_dpdk {
PADDED_MEMBERS_CACHELINE_MARKER(CACHE_LINE_SIZE, cacheline0,
+dpdk_port_t flow_transfer_proxy_port_id;
dpdk_port_t port_id;
/* If true, device was attached by rte_eth_dev_attach(). */ @@ -1130,8
+1131,9 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
uint32_t rx_chksm_offload_capa = RTE_ETH_RX_OFFLOAD_UDP_CKSUM |
RTE_ETH_RX_OFFLOAD_TCP_CKSUM |
RTE_ETH_RX_OFFLOAD_IPV4_CKSUM;
-
#ifdef ALLOW_EXPERIMENTAL_API
There are all over the patches this ifdef, even in cases it's harmless to do
without. It makes the code less readable and might cause future cherry-picking
issues. Try to minimize it only to places of a must.
I see your point. But I'm a rookie here, in OvS, and from the existing
code it's not clear which kinds of places would be harmless to stay
without if-defs and which would not. May I kindly request that
you point out at least one example in my patch = a place where
it would be safe to remove the checks. - ?
+int ret;
+
/*
* Full tunnel offload requires that tunnel ID metadata be
* delivered with "miss" packets from the hardware to the @@ -1141,6
+1143,27 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
* Request delivery of such metadata.
*/
dpdk_eth_dev_init_rx_metadata(dev);
+
+/*
+ * Managing "transfer" flows requires that the user communicate them
+ * via a port which has the privilege to control the embedded switch.
+ * For some vendors, all ports in a given switching domain have
+ * this privilege. For other vendors, it's only one port.
+ *
+ * Get the proxy port ID and remember it for later use.
+ */
+ret = rte_flow_pick_transfer_proxy(dev->port_id,
+ >flow_transfer_proxy_port_id,
NULL);
+if (ret != 0) {
+/*
+ * The PMD does not indicate the proxy port.
+ * Assume the proxy is unneeded.
+ */
+dev->flow_transfer_proxy_port_id = dev->port_id;
+}
+#else /* ! ALLOW_EXPERIMENTAL_API */
+/* No API to get transfer proxy; assume the proxy is unneeded. */
+dev->flow_transfer_proxy_port_id = dev->port_id;
#endif /* ALLOW_EXPERIMENTAL_API */
rte_eth_dev_info_get(dev->port_id, ); @@ -3762,8 +3785,12 @@
netdev_dpdk_detach(struct unixctl_conn *conn, int argc OVS_UNUSED,
const char *argv[], void *aux OVS_UNUSED) {
struct ds used_interfaces = DS_EMPTY_INITIALIZER;
+#ifdef ALLOW_EXPERIMENTAL_API
+struct netdev_dpdk *dev_self = NULL; #endif /*
+ALLOW_EXPERIMENTAL_API */
struct rte_eth_dev_info dev_info;
dpdk_port_t sibling_port_id;
+struct netdev_dpdk *dev;
dpdk_port_t port_id;
bool used = false;
char *response;
@@ -3781,8 +3808,6 @@ netdev_dpdk_detach(struct unixctl_conn *conn, int
argc OVS_UNUSED,
argv[1]);
RTE_ETH_FOREACH_DEV_SIBLING (sibling_port_id, port_id) {
-struct netdev_dpdk *dev;
-
LIST_FOR_EACH (dev, list_node, _list) {
if (dev->port_id != sibling_port_id) {
continue;
@@ -3802,6 +3827,27 @@ netdev_dpdk_detach(struct unixctl_conn *conn,
int argc OVS_UNUSED,
}
ds_destroy(_interfaces);
+#ifdef ALLOW_EXPERIMENTAL_API
+/*
+ * The device being detached may happen to be a flow proxy port
+ * for another device (still attached). If so, do not allow to
+ * detach. Devices dependent on this one must be detached first.
I don't think this is acceptable to deny the port from being detached, or to
enforce such ordering. For example, ports are being detached upon shutdown,
with unknown order.
I see your point. However, as far as I can understand, this specific
function is not the one that unplugs devices upon the "global" OvS
shutdown. It might be the one which is invoked when the user asks
to unplug a specific
Re: [ovs-dev] [PATCH v3 3/3] netdev-offload-dpdk: use flow transfer proxy mechanism
>-Original Message-
>From: Ivan Malov
>Sent: Wednesday, July 20, 2022 3:18 PM
>To: d...@openvswitch.org
>Cc: Eli Britstein ; Stephen Hemminger
>; Ilya Maximets ; Ori
>Kam ; Maxime Coquelin
>; David Marchand
>; Andrew Rybchenko
>
>Subject: [PATCH v3 3/3] netdev-offload-dpdk: use flow transfer proxy
>mechanism
>
>External email: Use caution opening links or attachments
>
>
>Manage "transfer" flows via the corresponding mechanism.
>Doing so requires that the traffic source be specified explicitly, via the
>corresponding pattern item.
>
>Signed-off-by: Ivan Malov
>Acked-by: Andrew Rybchenko
>---
> lib/netdev-dpdk.c | 99 ---
> lib/netdev-dpdk.h | 4 +-
> lib/netdev-offload-dpdk.c | 61
> 3 files changed, 135 insertions(+), 29 deletions(-)
>
>diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
>45e5d26d2..01fb40255 100644
>--- a/lib/netdev-dpdk.c
>+++ b/lib/netdev-dpdk.c
>@@ -420,6 +420,7 @@ enum dpdk_hw_ol_features {
>
> struct netdev_dpdk {
> PADDED_MEMBERS_CACHELINE_MARKER(CACHE_LINE_SIZE, cacheline0,
>+dpdk_port_t flow_transfer_proxy_port_id;
> dpdk_port_t port_id;
>
> /* If true, device was attached by rte_eth_dev_attach(). */ @@ -1130,8
>+1131,9 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
> uint32_t rx_chksm_offload_capa = RTE_ETH_RX_OFFLOAD_UDP_CKSUM |
> RTE_ETH_RX_OFFLOAD_TCP_CKSUM |
> RTE_ETH_RX_OFFLOAD_IPV4_CKSUM;
>-
> #ifdef ALLOW_EXPERIMENTAL_API
There are all over the patches this ifdef, even in cases it's harmless to do
without. It makes the code less readable and might cause future cherry-picking
issues. Try to minimize it only to places of a must.
>+int ret;
>+
> /*
> * Full tunnel offload requires that tunnel ID metadata be
> * delivered with "miss" packets from the hardware to the @@ -1141,6
>+1143,27 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
> * Request delivery of such metadata.
> */
> dpdk_eth_dev_init_rx_metadata(dev);
>+
>+/*
>+ * Managing "transfer" flows requires that the user communicate them
>+ * via a port which has the privilege to control the embedded switch.
>+ * For some vendors, all ports in a given switching domain have
>+ * this privilege. For other vendors, it's only one port.
>+ *
>+ * Get the proxy port ID and remember it for later use.
>+ */
>+ret = rte_flow_pick_transfer_proxy(dev->port_id,
>+ >flow_transfer_proxy_port_id,
>NULL);
>+if (ret != 0) {
>+/*
>+ * The PMD does not indicate the proxy port.
>+ * Assume the proxy is unneeded.
>+ */
>+dev->flow_transfer_proxy_port_id = dev->port_id;
>+}
>+#else /* ! ALLOW_EXPERIMENTAL_API */
>+/* No API to get transfer proxy; assume the proxy is unneeded. */
>+dev->flow_transfer_proxy_port_id = dev->port_id;
> #endif /* ALLOW_EXPERIMENTAL_API */
>
> rte_eth_dev_info_get(dev->port_id, ); @@ -3762,8 +3785,12 @@
>netdev_dpdk_detach(struct unixctl_conn *conn, int argc OVS_UNUSED,
>const char *argv[], void *aux OVS_UNUSED) {
> struct ds used_interfaces = DS_EMPTY_INITIALIZER;
>+#ifdef ALLOW_EXPERIMENTAL_API
>+struct netdev_dpdk *dev_self = NULL; #endif /*
>+ALLOW_EXPERIMENTAL_API */
> struct rte_eth_dev_info dev_info;
> dpdk_port_t sibling_port_id;
>+struct netdev_dpdk *dev;
> dpdk_port_t port_id;
> bool used = false;
> char *response;
>@@ -3781,8 +3808,6 @@ netdev_dpdk_detach(struct unixctl_conn *conn, int
>argc OVS_UNUSED,
> argv[1]);
>
> RTE_ETH_FOREACH_DEV_SIBLING (sibling_port_id, port_id) {
>-struct netdev_dpdk *dev;
>-
> LIST_FOR_EACH (dev, list_node, _list) {
> if (dev->port_id != sibling_port_id) {
> continue;
>@@ -3802,6 +3827,27 @@ netdev_dpdk_detach(struct unixctl_conn *conn,
>int argc OVS_UNUSED,
> }
> ds_destroy(_interfaces);
>
>+#ifdef ALLOW_EXPERIMENTAL_API
>+/*
>+ * The device being detached may happen to be a flow proxy port
>+ * for another device (still attached). If so, do not allow to
>+ * detach. Devices dependent on this one must be detached first.
I don't think this is acceptable to deny the port from being detached, or to
enforce such ordering. For example, ports are being detached upon shutdown,
with unknown order.
Suppose A is the proxy port for ports B,C. When port A is going to be detached,
flush the offloads of ports B and C (in addition to flushing the offloads of
port A). Then, it is safe to detach it.
>+ */
>+LIST_FOR_EACH (dev, list_node, _list) {
>+if (dev->port_id == port_id) {
>+dev_self = dev;
>+} else if (dev->flow_transfer_proxy_port_id == port_id) {
>+response = xasprintf("Device '%s' can not be detached (flow
>proxy)",
>+
[ovs-dev] [PATCH v3 3/3] netdev-offload-dpdk: use flow transfer proxy mechanism
Manage "transfer" flows via the corresponding mechanism.
Doing so requires that the traffic source be specified
explicitly, via the corresponding pattern item.
Signed-off-by: Ivan Malov
Acked-by: Andrew Rybchenko
---
lib/netdev-dpdk.c | 99 ---
lib/netdev-dpdk.h | 4 +-
lib/netdev-offload-dpdk.c | 61
3 files changed, 135 insertions(+), 29 deletions(-)
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 45e5d26d2..01fb40255 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -420,6 +420,7 @@ enum dpdk_hw_ol_features {
struct netdev_dpdk {
PADDED_MEMBERS_CACHELINE_MARKER(CACHE_LINE_SIZE, cacheline0,
+dpdk_port_t flow_transfer_proxy_port_id;
dpdk_port_t port_id;
/* If true, device was attached by rte_eth_dev_attach(). */
@@ -1130,8 +1131,9 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
uint32_t rx_chksm_offload_capa = RTE_ETH_RX_OFFLOAD_UDP_CKSUM |
RTE_ETH_RX_OFFLOAD_TCP_CKSUM |
RTE_ETH_RX_OFFLOAD_IPV4_CKSUM;
-
#ifdef ALLOW_EXPERIMENTAL_API
+int ret;
+
/*
* Full tunnel offload requires that tunnel ID metadata be
* delivered with "miss" packets from the hardware to the
@@ -1141,6 +1143,27 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
* Request delivery of such metadata.
*/
dpdk_eth_dev_init_rx_metadata(dev);
+
+/*
+ * Managing "transfer" flows requires that the user communicate them
+ * via a port which has the privilege to control the embedded switch.
+ * For some vendors, all ports in a given switching domain have
+ * this privilege. For other vendors, it's only one port.
+ *
+ * Get the proxy port ID and remember it for later use.
+ */
+ret = rte_flow_pick_transfer_proxy(dev->port_id,
+ >flow_transfer_proxy_port_id,
NULL);
+if (ret != 0) {
+/*
+ * The PMD does not indicate the proxy port.
+ * Assume the proxy is unneeded.
+ */
+dev->flow_transfer_proxy_port_id = dev->port_id;
+}
+#else /* ! ALLOW_EXPERIMENTAL_API */
+/* No API to get transfer proxy; assume the proxy is unneeded. */
+dev->flow_transfer_proxy_port_id = dev->port_id;
#endif /* ALLOW_EXPERIMENTAL_API */
rte_eth_dev_info_get(dev->port_id, );
@@ -3762,8 +3785,12 @@ netdev_dpdk_detach(struct unixctl_conn *conn, int argc
OVS_UNUSED,
const char *argv[], void *aux OVS_UNUSED)
{
struct ds used_interfaces = DS_EMPTY_INITIALIZER;
+#ifdef ALLOW_EXPERIMENTAL_API
+struct netdev_dpdk *dev_self = NULL;
+#endif /* ALLOW_EXPERIMENTAL_API */
struct rte_eth_dev_info dev_info;
dpdk_port_t sibling_port_id;
+struct netdev_dpdk *dev;
dpdk_port_t port_id;
bool used = false;
char *response;
@@ -3781,8 +3808,6 @@ netdev_dpdk_detach(struct unixctl_conn *conn, int argc
OVS_UNUSED,
argv[1]);
RTE_ETH_FOREACH_DEV_SIBLING (sibling_port_id, port_id) {
-struct netdev_dpdk *dev;
-
LIST_FOR_EACH (dev, list_node, _list) {
if (dev->port_id != sibling_port_id) {
continue;
@@ -3802,6 +3827,27 @@ netdev_dpdk_detach(struct unixctl_conn *conn, int argc
OVS_UNUSED,
}
ds_destroy(_interfaces);
+#ifdef ALLOW_EXPERIMENTAL_API
+/*
+ * The device being detached may happen to be a flow proxy port
+ * for another device (still attached). If so, do not allow to
+ * detach. Devices dependent on this one must be detached first.
+ */
+LIST_FOR_EACH (dev, list_node, _list) {
+if (dev->port_id == port_id) {
+dev_self = dev;
+} else if (dev->flow_transfer_proxy_port_id == port_id) {
+response = xasprintf("Device '%s' can not be detached (flow
proxy)",
+ argv[1]);
+goto error;
+}
+}
+
+/* Indicate that the device being detached no longer needs a flow proxy. */
+if (dev_self != NULL)
+dev_self->flow_transfer_proxy_port_id = port_id;
+#endif /* ALLOW_EXPERIMENTAL_API */
+
rte_eth_dev_info_get(port_id, _info);
rte_eth_dev_close(port_id);
if (rte_dev_remove(dev_info.device) < 0) {
@@ -5167,7 +5213,8 @@ unlock:
}
int
-netdev_dpdk_get_port_id(struct netdev *netdev)
+netdev_dpdk_get_port_id(struct netdev *netdev,
+bool flow_transfer_proxy)
{
struct netdev_dpdk *dev;
int ret = -1;
@@ -5178,7 +5225,7 @@ netdev_dpdk_get_port_id(struct netdev *netdev)
dev = netdev_dpdk_cast(netdev);
ovs_mutex_lock(>mutex);
-ret = dev->port_id;
+ret = flow_transfer_proxy ? dev->flow_transfer_proxy_port_id :
dev->port_id;
ovs_mutex_unlock(>mutex);
out:
return ret;
@@ -5214,13 +5261,15 @@ out:
int
netdev_dpdk_rte_flow_destroy(struct netdev *netdev,
-
