* [PATCH net 0/5] nfp: flower fixes and typo in ethtool stats name
@ 2017-11-17 1:06 Jakub Kicinski
2017-11-17 1:06 ` [PATCH net 1/5] nfp: fix flower offload metadata flag usage Jakub Kicinski
` (5 more replies)
0 siblings, 6 replies; 12+ messages in thread
From: Jakub Kicinski @ 2017-11-17 1:06 UTC (permalink / raw)
To: netdev; +Cc: oss-drivers, Jakub Kicinski
Hi!
This set comes from the flower offload team. From Pieter we have a fix
to the semantics of the flag telling FW whether to allocate or free
a mask and correction of a typo in name of one of the MAC statistics
(reveive -> received, we use past participle to match HW docs).
Dirk fixes propagation of max MTU to representors.
John improves VXLAN offload. The old code was not using egress_dev at
all, so Jiri missed it in his conversion. The validation of ingress
port is still not perfect, we will have to wait for shared block dust
to settle to tackle it. This is how John explains the cases:
The following example rule is now correctly offloaded in net-next kernel:
tc filter add dev vxlan0 ... enc_dst_port 4789 ... skip_sw \
action redirect dev nfp_p0
The following rule will not be offloaded to the NFP (previously it
incorrectly matched vxlan packets - it shouldn't as ingress dev is not
a vxlan netdev):
tc filter add dev nfp_p0 ... enc_dst_port 4789 ... skip_sw \
action redirect dev nfp_p0
Rules that are not matching on tunnels and are an egress offload are
rejected. The standard match code assumes the offloaded repr is the
ingress port. Rejecting egress offloads removes the chances of false
interpretation of the rules on the NFP.
A know issue is that the following rule example could still be offloaded
and incorrectly match tunnel data:
tc filter add dev dummy ... enc_dst_port 4789 ... skip_sw \
action redirect dev nfp_p0
Because the egress register callback does not give information on the
ingress netdev, the patch assumes that if it is not a repr then it is
the correct tunnel netdev. This may not be the case. The chances of this
happening is reduced as it is enforced that the rule match on the well
known vxlan port but it is still possible.
Dirk van der Merwe (1):
nfp: inherit the max_mtu from the PF netdev
John Hurley (2):
nfp: register flower reprs for egress dev offload
nfp: remove false positive offloads in flower vxlan
Pieter Jansen van Vuuren (2):
nfp: fix flower offload metadata flag usage
nfp: fix vlan receive MAC statistics typo
drivers/net/ethernet/netronome/nfp/flower/main.c | 18 +++++++++++
drivers/net/ethernet/netronome/nfp/flower/main.h | 5 +--
.../net/ethernet/netronome/nfp/flower/metadata.c | 7 +++--
.../net/ethernet/netronome/nfp/flower/offload.c | 36 ++++++++++++++++++----
drivers/net/ethernet/netronome/nfp/nfp_app.h | 20 ++++++++++++
.../net/ethernet/netronome/nfp/nfp_net_ethtool.c | 2 +-
drivers/net/ethernet/netronome/nfp/nfp_net_repr.c | 11 ++++++-
drivers/net/ethernet/netronome/nfp/nfp_port.h | 2 +-
8 files changed, 88 insertions(+), 13 deletions(-)
--
2.14.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH net 1/5] nfp: fix flower offload metadata flag usage
2017-11-17 1:06 [PATCH net 0/5] nfp: flower fixes and typo in ethtool stats name Jakub Kicinski
@ 2017-11-17 1:06 ` Jakub Kicinski
2017-11-17 1:06 ` [PATCH net 2/5] nfp: fix vlan receive MAC statistics typo Jakub Kicinski
` (4 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Jakub Kicinski @ 2017-11-17 1:06 UTC (permalink / raw)
To: netdev; +Cc: oss-drivers, Pieter Jansen van Vuuren
From: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
Hardware has no notion of new or last mask id, instead it makes use of the
message type (i.e. add flow or del flow) in combination with a single bit
in metadata flags to determine when to add or delete a mask id. Previously
we made use of the new or last flags to indicate that a new mask should be
allocated or deallocated, respectively. This incorrect behaviour is fixed
by making use single bit in metadata flags to indicate mask allocation or
deallocation.
Fixes: 43f84b72c50d ("nfp: add metadata to each flow offload")
Signed-off-by: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
drivers/net/ethernet/netronome/nfp/flower/main.h | 3 +--
drivers/net/ethernet/netronome/nfp/flower/metadata.c | 7 +++++--
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/netronome/nfp/flower/main.h b/drivers/net/ethernet/netronome/nfp/flower/main.h
index c90e72b7ff5a..a69ea62e9c9c 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/main.h
+++ b/drivers/net/ethernet/netronome/nfp/flower/main.h
@@ -52,8 +52,7 @@ struct nfp_app;
#define NFP_FLOWER_MASK_ELEMENT_RS 1
#define NFP_FLOWER_MASK_HASH_BITS 10
-#define NFP_FL_META_FLAG_NEW_MASK 128
-#define NFP_FL_META_FLAG_LAST_MASK 1
+#define NFP_FL_META_FLAG_MANAGE_MASK BIT(7)
#define NFP_FL_MASK_REUSE_TIME_NS 40000
#define NFP_FL_MASK_ID_LOCATION 1
diff --git a/drivers/net/ethernet/netronome/nfp/flower/metadata.c b/drivers/net/ethernet/netronome/nfp/flower/metadata.c
index 193520ef23f0..db977cf8e933 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/metadata.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/metadata.c
@@ -282,7 +282,7 @@ nfp_check_mask_add(struct nfp_app *app, char *mask_data, u32 mask_len,
id = nfp_add_mask_table(app, mask_data, mask_len);
if (id < 0)
return false;
- *meta_flags |= NFP_FL_META_FLAG_NEW_MASK;
+ *meta_flags |= NFP_FL_META_FLAG_MANAGE_MASK;
}
*mask_id = id;
@@ -299,6 +299,9 @@ nfp_check_mask_remove(struct nfp_app *app, char *mask_data, u32 mask_len,
if (!mask_entry)
return false;
+ if (meta_flags)
+ *meta_flags &= ~NFP_FL_META_FLAG_MANAGE_MASK;
+
*mask_id = mask_entry->mask_id;
mask_entry->ref_cnt--;
if (!mask_entry->ref_cnt) {
@@ -306,7 +309,7 @@ nfp_check_mask_remove(struct nfp_app *app, char *mask_data, u32 mask_len,
nfp_release_mask_id(app, *mask_id);
kfree(mask_entry);
if (meta_flags)
- *meta_flags |= NFP_FL_META_FLAG_LAST_MASK;
+ *meta_flags |= NFP_FL_META_FLAG_MANAGE_MASK;
}
return true;
--
2.14.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH net 2/5] nfp: fix vlan receive MAC statistics typo
2017-11-17 1:06 [PATCH net 0/5] nfp: flower fixes and typo in ethtool stats name Jakub Kicinski
2017-11-17 1:06 ` [PATCH net 1/5] nfp: fix flower offload metadata flag usage Jakub Kicinski
@ 2017-11-17 1:06 ` Jakub Kicinski
2017-11-17 1:06 ` [PATCH net 3/5] nfp: inherit the max_mtu from the PF netdev Jakub Kicinski
` (3 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Jakub Kicinski @ 2017-11-17 1:06 UTC (permalink / raw)
To: netdev; +Cc: oss-drivers, Pieter Jansen van Vuuren
From: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
Correct typo in vlan receive MAC stats. Previously the MAC statistics
reported in ethtool for vlan receive contained a typo resulting in ethtool
reporting rx_vlan_reveive_ok instead of rx_vlan_received_ok.
Fixes: a5950182c00e ("nfp: map mac_stats and vf_cfg BARs")
Fixes: 098ce840c9ef ("nfp: report MAC statistics in ethtool")
Reported-by: Brendan Galloway <brendan.galloway@netronome.com>
Signed-off-by: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
---
drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c | 2 +-
drivers/net/ethernet/netronome/nfp/nfp_port.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
index 60c8d733a37d..2801ecd09eab 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
@@ -104,7 +104,7 @@ static const struct nfp_et_stat nfp_mac_et_stats[] = {
{ "rx_frame_too_long_errors",
NFP_MAC_STATS_RX_FRAME_TOO_LONG_ERRORS, },
{ "rx_range_length_errors", NFP_MAC_STATS_RX_RANGE_LENGTH_ERRORS, },
- { "rx_vlan_reveive_ok", NFP_MAC_STATS_RX_VLAN_REVEIVE_OK, },
+ { "rx_vlan_received_ok", NFP_MAC_STATS_RX_VLAN_RECEIVED_OK, },
{ "rx_errors", NFP_MAC_STATS_RX_IN_ERRORS, },
{ "rx_broadcast_pkts", NFP_MAC_STATS_RX_IN_BROADCAST_PKTS, },
{ "rx_drop_events", NFP_MAC_STATS_RX_DROP_EVENTS, },
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_port.h b/drivers/net/ethernet/netronome/nfp/nfp_port.h
index 51dcb9c603ee..21bd4aa32646 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_port.h
+++ b/drivers/net/ethernet/netronome/nfp/nfp_port.h
@@ -157,7 +157,7 @@ void nfp_devlink_port_unregister(struct nfp_port *port);
/* unused 0x008 */
#define NFP_MAC_STATS_RX_FRAME_TOO_LONG_ERRORS (NFP_MAC_STATS_BASE + 0x010)
#define NFP_MAC_STATS_RX_RANGE_LENGTH_ERRORS (NFP_MAC_STATS_BASE + 0x018)
-#define NFP_MAC_STATS_RX_VLAN_REVEIVE_OK (NFP_MAC_STATS_BASE + 0x020)
+#define NFP_MAC_STATS_RX_VLAN_RECEIVED_OK (NFP_MAC_STATS_BASE + 0x020)
#define NFP_MAC_STATS_RX_IN_ERRORS (NFP_MAC_STATS_BASE + 0x028)
#define NFP_MAC_STATS_RX_IN_BROADCAST_PKTS (NFP_MAC_STATS_BASE + 0x030)
#define NFP_MAC_STATS_RX_DROP_EVENTS (NFP_MAC_STATS_BASE + 0x038)
--
2.14.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH net 3/5] nfp: inherit the max_mtu from the PF netdev
2017-11-17 1:06 [PATCH net 0/5] nfp: flower fixes and typo in ethtool stats name Jakub Kicinski
2017-11-17 1:06 ` [PATCH net 1/5] nfp: fix flower offload metadata flag usage Jakub Kicinski
2017-11-17 1:06 ` [PATCH net 2/5] nfp: fix vlan receive MAC statistics typo Jakub Kicinski
@ 2017-11-17 1:06 ` Jakub Kicinski
2017-11-17 1:06 ` [PATCH net 4/5] nfp: register flower reprs for egress dev offload Jakub Kicinski
` (2 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Jakub Kicinski @ 2017-11-17 1:06 UTC (permalink / raw)
To: netdev; +Cc: oss-drivers, Dirk van der Merwe
From: Dirk van der Merwe <dirk.vandermerwe@netronome.com>
The PF netdev is used for data transfer for reprs, so reprs inherit the
maximum MTU settings of the PF netdev.
Fixes: 5de73ee46704 ("nfp: general representor implementation")
Signed-off-by: Dirk van der Merwe <dirk.vandermerwe@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
drivers/net/ethernet/netronome/nfp/nfp_net_repr.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c b/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c
index 1bce8c131bb9..fa052a929170 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c
@@ -297,6 +297,8 @@ int nfp_repr_init(struct nfp_app *app, struct net_device *netdev,
netdev->netdev_ops = &nfp_repr_netdev_ops;
netdev->ethtool_ops = &nfp_port_ethtool_ops;
+ netdev->max_mtu = pf_netdev->max_mtu;
+
SWITCHDEV_SET_OPS(netdev, &nfp_port_switchdev_ops);
if (nfp_app_has_tc(app)) {
--
2.14.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH net 4/5] nfp: register flower reprs for egress dev offload
2017-11-17 1:06 [PATCH net 0/5] nfp: flower fixes and typo in ethtool stats name Jakub Kicinski
` (2 preceding siblings ...)
2017-11-17 1:06 ` [PATCH net 3/5] nfp: inherit the max_mtu from the PF netdev Jakub Kicinski
@ 2017-11-17 1:06 ` Jakub Kicinski
2017-11-17 1:06 ` [PATCH net 5/5] nfp: remove false positive offloads in flower vxlan Jakub Kicinski
2017-11-17 5:10 ` [PATCH net 0/5] nfp: flower fixes and typo in ethtool stats name David Miller
5 siblings, 0 replies; 12+ messages in thread
From: Jakub Kicinski @ 2017-11-17 1:06 UTC (permalink / raw)
To: netdev; +Cc: oss-drivers, John Hurley
From: John Hurley <john.hurley@netronome.com>
Register a callback for offloading flows that have a repr as their egress
device. The new egdev_register function is added to net-next for the 4.15
release.
Signed-off-by: John Hurley <john.hurley@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
drivers/net/ethernet/netronome/nfp/flower/main.c | 18 ++++++++++++++++++
drivers/net/ethernet/netronome/nfp/flower/main.h | 2 ++
drivers/net/ethernet/netronome/nfp/flower/offload.c | 6 ++++++
drivers/net/ethernet/netronome/nfp/nfp_app.h | 20 ++++++++++++++++++++
drivers/net/ethernet/netronome/nfp/nfp_net_repr.c | 9 ++++++++-
5 files changed, 54 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/netronome/nfp/flower/main.c b/drivers/net/ethernet/netronome/nfp/flower/main.c
index e0283bb24f06..8fcc90c0d2d3 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/main.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/main.c
@@ -125,6 +125,21 @@ nfp_flower_repr_netdev_stop(struct nfp_app *app, struct nfp_repr *repr)
return nfp_flower_cmsg_portmod(repr, false);
}
+static int
+nfp_flower_repr_netdev_init(struct nfp_app *app, struct net_device *netdev)
+{
+ return tc_setup_cb_egdev_register(netdev,
+ nfp_flower_setup_tc_egress_cb,
+ netdev_priv(netdev));
+}
+
+static void
+nfp_flower_repr_netdev_clean(struct nfp_app *app, struct net_device *netdev)
+{
+ tc_setup_cb_egdev_unregister(netdev, nfp_flower_setup_tc_egress_cb,
+ netdev_priv(netdev));
+}
+
static void nfp_flower_sriov_disable(struct nfp_app *app)
{
struct nfp_flower_priv *priv = app->priv;
@@ -452,6 +467,9 @@ const struct nfp_app_type app_flower = {
.vnic_init = nfp_flower_vnic_init,
.vnic_clean = nfp_flower_vnic_clean,
+ .repr_init = nfp_flower_repr_netdev_init,
+ .repr_clean = nfp_flower_repr_netdev_clean,
+
.repr_open = nfp_flower_repr_netdev_open,
.repr_stop = nfp_flower_repr_netdev_stop,
diff --git a/drivers/net/ethernet/netronome/nfp/flower/main.h b/drivers/net/ethernet/netronome/nfp/flower/main.h
index a69ea62e9c9c..e6b26c5ae6e0 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/main.h
+++ b/drivers/net/ethernet/netronome/nfp/flower/main.h
@@ -196,5 +196,7 @@ void nfp_tunnel_del_ipv4_off(struct nfp_app *app, __be32 ipv4);
void nfp_tunnel_add_ipv4_off(struct nfp_app *app, __be32 ipv4);
void nfp_tunnel_request_route(struct nfp_app *app, struct sk_buff *skb);
void nfp_tunnel_keep_alive(struct nfp_app *app, struct sk_buff *skb);
+int nfp_flower_setup_tc_egress_cb(enum tc_setup_type type, void *type_data,
+ void *cb_priv);
#endif
diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
index cdbb5464b790..a0193e0c24a0 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
@@ -465,6 +465,12 @@ nfp_flower_repr_offload(struct nfp_app *app, struct net_device *netdev,
return -EOPNOTSUPP;
}
+int nfp_flower_setup_tc_egress_cb(enum tc_setup_type type, void *type_data,
+ void *cb_priv)
+{
+ return -EINVAL;
+}
+
static int nfp_flower_setup_tc_block_cb(enum tc_setup_type type,
void *type_data, void *cb_priv)
{
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_app.h b/drivers/net/ethernet/netronome/nfp/nfp_app.h
index 54b67c9b8d5b..0e5e0305ad1c 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_app.h
+++ b/drivers/net/ethernet/netronome/nfp/nfp_app.h
@@ -76,6 +76,8 @@ extern const struct nfp_app_type app_flower;
* @vnic_free: free up app's vNIC state
* @vnic_init: vNIC netdev was registered
* @vnic_clean: vNIC netdev about to be unregistered
+ * @repr_init: representor about to be registered
+ * @repr_clean: representor about to be unregistered
* @repr_open: representor netdev open callback
* @repr_stop: representor netdev stop callback
* @start: start application logic
@@ -109,6 +111,9 @@ struct nfp_app_type {
int (*vnic_init)(struct nfp_app *app, struct nfp_net *nn);
void (*vnic_clean)(struct nfp_app *app, struct nfp_net *nn);
+ int (*repr_init)(struct nfp_app *app, struct net_device *netdev);
+ void (*repr_clean)(struct nfp_app *app, struct net_device *netdev);
+
int (*repr_open)(struct nfp_app *app, struct nfp_repr *repr);
int (*repr_stop)(struct nfp_app *app, struct nfp_repr *repr);
@@ -212,6 +217,21 @@ static inline int nfp_app_repr_stop(struct nfp_app *app, struct nfp_repr *repr)
return app->type->repr_stop(app, repr);
}
+static inline int
+nfp_app_repr_init(struct nfp_app *app, struct net_device *netdev)
+{
+ if (!app->type->repr_init)
+ return 0;
+ return app->type->repr_init(app, netdev);
+}
+
+static inline void
+nfp_app_repr_clean(struct nfp_app *app, struct net_device *netdev)
+{
+ if (app->type->repr_clean)
+ app->type->repr_clean(app, netdev);
+}
+
static inline int nfp_app_start(struct nfp_app *app, struct nfp_net *ctrl)
{
app->ctrl = ctrl;
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c b/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c
index fa052a929170..924a05e05da0 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c
@@ -258,6 +258,7 @@ const struct net_device_ops nfp_repr_netdev_ops = {
static void nfp_repr_clean(struct nfp_repr *repr)
{
unregister_netdev(repr->netdev);
+ nfp_app_repr_clean(repr->app, repr->netdev);
dst_release((struct dst_entry *)repr->dst);
nfp_port_free(repr->port);
}
@@ -306,12 +307,18 @@ int nfp_repr_init(struct nfp_app *app, struct net_device *netdev,
netdev->hw_features |= NETIF_F_HW_TC;
}
- err = register_netdev(netdev);
+ err = nfp_app_repr_init(app, netdev);
if (err)
goto err_clean;
+ err = register_netdev(netdev);
+ if (err)
+ goto err_repr_clean;
+
return 0;
+err_repr_clean:
+ nfp_app_repr_clean(app, netdev);
err_clean:
dst_release((struct dst_entry *)repr->dst);
return err;
--
2.14.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH net 5/5] nfp: remove false positive offloads in flower vxlan
2017-11-17 1:06 [PATCH net 0/5] nfp: flower fixes and typo in ethtool stats name Jakub Kicinski
` (3 preceding siblings ...)
2017-11-17 1:06 ` [PATCH net 4/5] nfp: register flower reprs for egress dev offload Jakub Kicinski
@ 2017-11-17 1:06 ` Jakub Kicinski
2018-04-18 7:43 ` Or Gerlitz
2017-11-17 5:10 ` [PATCH net 0/5] nfp: flower fixes and typo in ethtool stats name David Miller
5 siblings, 1 reply; 12+ messages in thread
From: Jakub Kicinski @ 2017-11-17 1:06 UTC (permalink / raw)
To: netdev; +Cc: oss-drivers, John Hurley
From: John Hurley <john.hurley@netronome.com>
Pass information to the match offload on whether or not the repr is the
ingress or egress dev. Only accept tunnel matches if repr is the egress
dev.
This means rules such as the following are successfully offloaded:
tc .. add dev vxlan0 .. enc_dst_port 4789 .. action redirect dev nfp_p0
While rules such as the following are rejected:
tc .. add dev nfp_p0 .. enc_dst_port 4789 .. action redirect dev vxlan0
Also reject non tunnel flows that are offloaded to an egress dev.
Non tunnel matches assume that the offload dev is the ingress port and
offload a match accordingly.
Fixes: 611aec101ab7 ("nfp: compile flower vxlan tunnel metadata match fields")
Signed-off-by: John Hurley <john.hurley@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
.../net/ethernet/netronome/nfp/flower/offload.c | 32 +++++++++++++++++-----
1 file changed, 25 insertions(+), 7 deletions(-)
diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
index a0193e0c24a0..f5d73b83dcc2 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
@@ -131,7 +131,8 @@ static bool nfp_flower_check_higher_than_mac(struct tc_cls_flower_offload *f)
static int
nfp_flower_calculate_key_layers(struct nfp_fl_key_ls *ret_key_ls,
- struct tc_cls_flower_offload *flow)
+ struct tc_cls_flower_offload *flow,
+ bool egress)
{
struct flow_dissector_key_basic *mask_basic = NULL;
struct flow_dissector_key_basic *key_basic = NULL;
@@ -167,6 +168,9 @@ nfp_flower_calculate_key_layers(struct nfp_fl_key_ls *ret_key_ls,
skb_flow_dissector_target(flow->dissector,
FLOW_DISSECTOR_KEY_ENC_CONTROL,
flow->key);
+ if (!egress)
+ return -EOPNOTSUPP;
+
if (mask_enc_ctl->addr_type != 0xffff ||
enc_ctl->addr_type != FLOW_DISSECTOR_KEY_IPV4_ADDRS)
return -EOPNOTSUPP;
@@ -194,6 +198,9 @@ nfp_flower_calculate_key_layers(struct nfp_fl_key_ls *ret_key_ls,
key_layer |= NFP_FLOWER_LAYER_VXLAN;
key_size += sizeof(struct nfp_flower_vxlan);
+ } else if (egress) {
+ /* Reject non tunnel matches offloaded to egress repr. */
+ return -EOPNOTSUPP;
}
if (dissector_uses_key(flow->dissector, FLOW_DISSECTOR_KEY_BASIC)) {
@@ -315,7 +322,7 @@ nfp_flower_allocate_new(struct nfp_fl_key_ls *key_layer)
*/
static int
nfp_flower_add_offload(struct nfp_app *app, struct net_device *netdev,
- struct tc_cls_flower_offload *flow)
+ struct tc_cls_flower_offload *flow, bool egress)
{
struct nfp_flower_priv *priv = app->priv;
struct nfp_fl_payload *flow_pay;
@@ -326,7 +333,7 @@ nfp_flower_add_offload(struct nfp_app *app, struct net_device *netdev,
if (!key_layer)
return -ENOMEM;
- err = nfp_flower_calculate_key_layers(key_layer, flow);
+ err = nfp_flower_calculate_key_layers(key_layer, flow, egress);
if (err)
goto err_free_key_ls;
@@ -447,7 +454,7 @@ nfp_flower_get_stats(struct nfp_app *app, struct tc_cls_flower_offload *flow)
static int
nfp_flower_repr_offload(struct nfp_app *app, struct net_device *netdev,
- struct tc_cls_flower_offload *flower)
+ struct tc_cls_flower_offload *flower, bool egress)
{
if (!eth_proto_is_802_3(flower->common.protocol) ||
flower->common.chain_index)
@@ -455,7 +462,7 @@ nfp_flower_repr_offload(struct nfp_app *app, struct net_device *netdev,
switch (flower->command) {
case TC_CLSFLOWER_REPLACE:
- return nfp_flower_add_offload(app, netdev, flower);
+ return nfp_flower_add_offload(app, netdev, flower, egress);
case TC_CLSFLOWER_DESTROY:
return nfp_flower_del_offload(app, netdev, flower);
case TC_CLSFLOWER_STATS:
@@ -468,7 +475,18 @@ nfp_flower_repr_offload(struct nfp_app *app, struct net_device *netdev,
int nfp_flower_setup_tc_egress_cb(enum tc_setup_type type, void *type_data,
void *cb_priv)
{
- return -EINVAL;
+ struct nfp_repr *repr = cb_priv;
+
+ if (!tc_can_offload(repr->netdev))
+ return -EOPNOTSUPP;
+
+ switch (type) {
+ case TC_SETUP_CLSFLOWER:
+ return nfp_flower_repr_offload(repr->app, repr->netdev,
+ type_data, true);
+ default:
+ return -EOPNOTSUPP;
+ }
}
static int nfp_flower_setup_tc_block_cb(enum tc_setup_type type,
@@ -482,7 +500,7 @@ static int nfp_flower_setup_tc_block_cb(enum tc_setup_type type,
switch (type) {
case TC_SETUP_CLSFLOWER:
return nfp_flower_repr_offload(repr->app, repr->netdev,
- type_data);
+ type_data, false);
default:
return -EOPNOTSUPP;
}
--
2.14.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH net 0/5] nfp: flower fixes and typo in ethtool stats name
2017-11-17 1:06 [PATCH net 0/5] nfp: flower fixes and typo in ethtool stats name Jakub Kicinski
` (4 preceding siblings ...)
2017-11-17 1:06 ` [PATCH net 5/5] nfp: remove false positive offloads in flower vxlan Jakub Kicinski
@ 2017-11-17 5:10 ` David Miller
5 siblings, 0 replies; 12+ messages in thread
From: David Miller @ 2017-11-17 5:10 UTC (permalink / raw)
To: jakub.kicinski; +Cc: netdev, oss-drivers
From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Thu, 16 Nov 2017 17:06:38 -0800
> This set comes from the flower offload team.
Series applied, thank you.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net 5/5] nfp: remove false positive offloads in flower vxlan
2017-11-17 1:06 ` [PATCH net 5/5] nfp: remove false positive offloads in flower vxlan Jakub Kicinski
@ 2018-04-18 7:43 ` Or Gerlitz
2018-04-18 12:31 ` John Hurley
0 siblings, 1 reply; 12+ messages in thread
From: Or Gerlitz @ 2018-04-18 7:43 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: Linux Netdev List, oss-drivers, John Hurley
On Fri, Nov 17, 2017 at 4:06 AM, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
> From: John Hurley <john.hurley@netronome.com>
>
> Pass information to the match offload on whether or not the repr is the
> ingress or egress dev. Only accept tunnel matches if repr is the egress dev.
>
> This means rules such as the following are successfully offloaded:
> tc .. add dev vxlan0 .. enc_dst_port 4789 .. action redirect dev nfp_p0
>
> While rules such as the following are rejected:
> tc .. add dev nfp_p0 .. enc_dst_port 4789 .. action redirect dev vxlan0
cool
> Also reject non tunnel flows that are offloaded to an egress dev.
> Non tunnel matches assume that the offload dev is the ingress port and
> offload a match accordingly.
not following on the "Also" here, see below
> diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
> index a0193e0c24a0..f5d73b83dcc2 100644
> --- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
> +++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
> @@ -131,7 +131,8 @@ static bool nfp_flower_check_higher_than_mac(struct tc_cls_flower_offload *f)
>
> static int
> nfp_flower_calculate_key_layers(struct nfp_fl_key_ls *ret_key_ls,
> - struct tc_cls_flower_offload *flow)
> + struct tc_cls_flower_offload *flow,
> + bool egress)
> {
> struct flow_dissector_key_basic *mask_basic = NULL;
> struct flow_dissector_key_basic *key_basic = NULL;
> @@ -167,6 +168,9 @@ nfp_flower_calculate_key_layers(struct nfp_fl_key_ls *ret_key_ls,
> skb_flow_dissector_target(flow->dissector,
> FLOW_DISSECTOR_KEY_ENC_CONTROL,
> flow->key);
> + if (!egress)
> + return -EOPNOTSUPP;
> +
> if (mask_enc_ctl->addr_type != 0xffff ||
> enc_ctl->addr_type != FLOW_DISSECTOR_KEY_IPV4_ADDRS)
> return -EOPNOTSUPP;
> @@ -194,6 +198,9 @@ nfp_flower_calculate_key_layers(struct nfp_fl_key_ls *ret_key_ls,
>
> key_layer |= NFP_FLOWER_LAYER_VXLAN;
> key_size += sizeof(struct nfp_flower_vxlan);
> + } else if (egress) {
> + /* Reject non tunnel matches offloaded to egress repr. */
> + return -EOPNOTSUPP;
> }
with these two hunks we get: egress <- IFF -> encap match, right?
(1) we can't offload the egress way if there isn't matching on encap headers
(2) we can't go the matching on encap headers way if we are not egress
what other cases are rejected by this logic?
e.g If we add a rule with SW device (veth. tap) being the ingress, and
HW device (vf rep)
being the egress while not using skip_sw (just no flags == both) we
get the TC stack
go along the egdev callback from the vf rep hw device and add an
(uplink --> vf rep) rule
which will not be rejected if there is matching on tunnel headers, it
will also not be rejected
by some driver logic as the one we discussed to identify and ignore
rules that are attempted to being added twice.
Or.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net 5/5] nfp: remove false positive offloads in flower vxlan
2018-04-18 7:43 ` Or Gerlitz
@ 2018-04-18 12:31 ` John Hurley
2018-04-18 18:18 ` Or Gerlitz
0 siblings, 1 reply; 12+ messages in thread
From: John Hurley @ 2018-04-18 12:31 UTC (permalink / raw)
To: Or Gerlitz; +Cc: Jakub Kicinski, Linux Netdev List, oss-drivers, Simon Horman
On Wed, Apr 18, 2018 at 8:43 AM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
> On Fri, Nov 17, 2017 at 4:06 AM, Jakub Kicinski
> <jakub.kicinski@netronome.com> wrote:
>> From: John Hurley <john.hurley@netronome.com>
>>
>> Pass information to the match offload on whether or not the repr is the
>> ingress or egress dev. Only accept tunnel matches if repr is the egress dev.
>>
>> This means rules such as the following are successfully offloaded:
>> tc .. add dev vxlan0 .. enc_dst_port 4789 .. action redirect dev nfp_p0
>>
>> While rules such as the following are rejected:
>> tc .. add dev nfp_p0 .. enc_dst_port 4789 .. action redirect dev vxlan0
>
> cool
>
>
>> Also reject non tunnel flows that are offloaded to an egress dev.
>> Non tunnel matches assume that the offload dev is the ingress port and
>> offload a match accordingly.
>
> not following on the "Also" here, see below
>
>
>> diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
>> index a0193e0c24a0..f5d73b83dcc2 100644
>> --- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
>> +++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
>> @@ -131,7 +131,8 @@ static bool nfp_flower_check_higher_than_mac(struct tc_cls_flower_offload *f)
>>
>> static int
>> nfp_flower_calculate_key_layers(struct nfp_fl_key_ls *ret_key_ls,
>> - struct tc_cls_flower_offload *flow)
>> + struct tc_cls_flower_offload *flow,
>> + bool egress)
>> {
>> struct flow_dissector_key_basic *mask_basic = NULL;
>> struct flow_dissector_key_basic *key_basic = NULL;
>> @@ -167,6 +168,9 @@ nfp_flower_calculate_key_layers(struct nfp_fl_key_ls *ret_key_ls,
>> skb_flow_dissector_target(flow->dissector,
>> FLOW_DISSECTOR_KEY_ENC_CONTROL,
>> flow->key);
>> + if (!egress)
>> + return -EOPNOTSUPP;
>> +
>> if (mask_enc_ctl->addr_type != 0xffff ||
>> enc_ctl->addr_type != FLOW_DISSECTOR_KEY_IPV4_ADDRS)
>> return -EOPNOTSUPP;
>> @@ -194,6 +198,9 @@ nfp_flower_calculate_key_layers(struct nfp_fl_key_ls *ret_key_ls,
>>
>> key_layer |= NFP_FLOWER_LAYER_VXLAN;
>> key_size += sizeof(struct nfp_flower_vxlan);
>> + } else if (egress) {
>> + /* Reject non tunnel matches offloaded to egress repr. */
>> + return -EOPNOTSUPP;
>> }
>
> with these two hunks we get: egress <- IFF -> encap match, right?
>
> (1) we can't offload the egress way if there isn't matching on encap headers
> (2) we can't go the matching on encap headers way if we are not egress
>
yes, this is correct.
With the block code and egdev offload, we do not have access to the
ingress netdev when doing an offload.
We need to use the encap headers (especially the enc_port) to
distinguish the type of tunnel used and, therefore, require that the
encap matches be present before offloading.
> what other cases are rejected by this logic?
>
Yes, some other cases may be rejected (like veth mentioned below).
However, this is better than allowing rules to be incorrectly
offloaded (as could have happened before these changes).
Currently, we are looking at offloading flows on other ingress devices
such as bonds so this will require a change to the driver code here.
IMO, the cleanest solution will also require tc core changes to either
avoid egdev offload or to have access to the ingress netdev of a rule.
> e.g If we add a rule with SW device (veth. tap) being the ingress, and
> HW device (vf rep)
> being the egress while not using skip_sw (just no flags == both) we
> get the TC stack
> go along the egdev callback from the vf rep hw device and add an
> (uplink --> vf rep) rule
> which will not be rejected if there is matching on tunnel headers, it
> will also not be rejected
> by some driver logic as the one we discussed to identify and ignore
> rules that are attempted to being added twice.
>
> Or.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net 5/5] nfp: remove false positive offloads in flower vxlan
2018-04-18 12:31 ` John Hurley
@ 2018-04-18 18:18 ` Or Gerlitz
2018-04-18 22:31 ` John Hurley
0 siblings, 1 reply; 12+ messages in thread
From: Or Gerlitz @ 2018-04-18 18:18 UTC (permalink / raw)
To: John Hurley; +Cc: Jakub Kicinski, Linux Netdev List, oss-drivers, Simon Horman
On Wed, Apr 18, 2018 at 3:31 PM, John Hurley <john.hurley@netronome.com> wrote:
> On Wed, Apr 18, 2018 at 8:43 AM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
>> On Fri, Nov 17, 2017 at 4:06 AM, Jakub Kicinski
>> <jakub.kicinski@netronome.com> wrote:
>>> From: John Hurley <john.hurley@netronome.com>
>>>
>>> Pass information to the match offload on whether or not the repr is the
>>> ingress or egress dev. Only accept tunnel matches if repr is the egress dev.
>>>
>>> This means rules such as the following are successfully offloaded:
>>> tc .. add dev vxlan0 .. enc_dst_port 4789 .. action redirect dev nfp_p0
>>>
>>> While rules such as the following are rejected:
>>> tc .. add dev nfp_p0 .. enc_dst_port 4789 .. action redirect dev vxlan0
>>
>> cool
>>
>>
>>> Also reject non tunnel flows that are offloaded to an egress dev.
>>> Non tunnel matches assume that the offload dev is the ingress port and
>>> offload a match accordingly.
>>
>> not following on the "Also" here, see below
>>
>>
>>> diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
>>> index a0193e0c24a0..f5d73b83dcc2 100644
>>> --- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
>>> +++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
>>> @@ -131,7 +131,8 @@ static bool nfp_flower_check_higher_than_mac(struct tc_cls_flower_offload *f)
>>>
>>> static int
>>> nfp_flower_calculate_key_layers(struct nfp_fl_key_ls *ret_key_ls,
>>> - struct tc_cls_flower_offload *flow)
>>> + struct tc_cls_flower_offload *flow,
>>> + bool egress)
>>> {
>>> struct flow_dissector_key_basic *mask_basic = NULL;
>>> struct flow_dissector_key_basic *key_basic = NULL;
>>> @@ -167,6 +168,9 @@ nfp_flower_calculate_key_layers(struct nfp_fl_key_ls *ret_key_ls,
>>> skb_flow_dissector_target(flow->dissector,
>>> FLOW_DISSECTOR_KEY_ENC_CONTROL,
>>> flow->key);
>>> + if (!egress)
>>> + return -EOPNOTSUPP;
>>> +
>>> if (mask_enc_ctl->addr_type != 0xffff ||
>>> enc_ctl->addr_type != FLOW_DISSECTOR_KEY_IPV4_ADDRS)
>>> return -EOPNOTSUPP;
>>> @@ -194,6 +198,9 @@ nfp_flower_calculate_key_layers(struct nfp_fl_key_ls *ret_key_ls,
>>>
>>> key_layer |= NFP_FLOWER_LAYER_VXLAN;
>>> key_size += sizeof(struct nfp_flower_vxlan);
>>> + } else if (egress) {
>>> + /* Reject non tunnel matches offloaded to egress repr. */
>>> + return -EOPNOTSUPP;
>>> }
>>
>> with these two hunks we get: egress <- IFF -> encap match, right?
>>
>> (1) we can't offload the egress way if there isn't matching on encap headers
>> (2) we can't go the matching on encap headers way if we are not egress
>>
>
> yes, this is correct.
> With the block code and egdev offload, we do not have access to the
> ingress netdev when doing an offload.
> We need to use the encap headers (especially the enc_port) to
> distinguish the type of tunnel used and, therefore, require that the
> encap matches be present before offloading.
>
>> what other cases are rejected by this logic?
>>
>
> Yes, some other cases may be rejected (like veth mentioned below).
my claim is that the veth case I mentioned below will not be rejected
if it has the matching on encap headers, and a wrong rule will be set
into hw, agree?
> However, this is better than allowing rules to be incorrectly
> offloaded (as could have happened before these changes).
> Currently, we are looking at offloading flows on other ingress devices
> such as bonds so this will require a change to the driver code here.
for the ingress side, Jiri suggested that the slave devices (uplink reps),
will be just getting all the rules set on the bond, so I am not sure what
problem you see here... for decap it will be still vxlan --> vf rep and your
egress logic will allow it.
> IMO, the cleanest solution will also require tc core changes to either
> avoid egdev offload or to have access to the ingress netdev of a rule.
>> e.g If we add a rule with SW device (veth. tap) being the ingress, and
>> HW device (vf rep)
>> being the egress while not using skip_sw (just no flags == both) we
>> get the TC stack
>> go along the egdev callback from the vf rep hw device and add an
>> (uplink --> vf rep) rule
>> which will not be rejected if there is matching on tunnel headers, it
>> will also not be rejected
>> by some driver logic as the one we discussed to identify and ignore
>> rules that are attempted to being added twice.
>>
>> Or.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net 5/5] nfp: remove false positive offloads in flower vxlan
2018-04-18 18:18 ` Or Gerlitz
@ 2018-04-18 22:31 ` John Hurley
2018-04-19 21:11 ` Or Gerlitz
0 siblings, 1 reply; 12+ messages in thread
From: John Hurley @ 2018-04-18 22:31 UTC (permalink / raw)
To: Or Gerlitz; +Cc: Jakub Kicinski, Linux Netdev List, oss-drivers, Simon Horman
On Wed, Apr 18, 2018 at 7:18 PM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
> On Wed, Apr 18, 2018 at 3:31 PM, John Hurley <john.hurley@netronome.com> wrote:
>> On Wed, Apr 18, 2018 at 8:43 AM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
>>> On Fri, Nov 17, 2017 at 4:06 AM, Jakub Kicinski
>>> <jakub.kicinski@netronome.com> wrote:
>>>> From: John Hurley <john.hurley@netronome.com>
>>>>
>>>> Pass information to the match offload on whether or not the repr is the
>>>> ingress or egress dev. Only accept tunnel matches if repr is the egress dev.
>>>>
>>>> This means rules such as the following are successfully offloaded:
>>>> tc .. add dev vxlan0 .. enc_dst_port 4789 .. action redirect dev nfp_p0
>>>>
>>>> While rules such as the following are rejected:
>>>> tc .. add dev nfp_p0 .. enc_dst_port 4789 .. action redirect dev vxlan0
>>>
>>> cool
>>>
>>>
>>>> Also reject non tunnel flows that are offloaded to an egress dev.
>>>> Non tunnel matches assume that the offload dev is the ingress port and
>>>> offload a match accordingly.
>>>
>>> not following on the "Also" here, see below
>>>
>>>
>>>> diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
>>>> index a0193e0c24a0..f5d73b83dcc2 100644
>>>> --- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
>>>> +++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
>>>> @@ -131,7 +131,8 @@ static bool nfp_flower_check_higher_than_mac(struct tc_cls_flower_offload *f)
>>>>
>>>> static int
>>>> nfp_flower_calculate_key_layers(struct nfp_fl_key_ls *ret_key_ls,
>>>> - struct tc_cls_flower_offload *flow)
>>>> + struct tc_cls_flower_offload *flow,
>>>> + bool egress)
>>>> {
>>>> struct flow_dissector_key_basic *mask_basic = NULL;
>>>> struct flow_dissector_key_basic *key_basic = NULL;
>>>> @@ -167,6 +168,9 @@ nfp_flower_calculate_key_layers(struct nfp_fl_key_ls *ret_key_ls,
>>>> skb_flow_dissector_target(flow->dissector,
>>>> FLOW_DISSECTOR_KEY_ENC_CONTROL,
>>>> flow->key);
>>>> + if (!egress)
>>>> + return -EOPNOTSUPP;
>>>> +
>>>> if (mask_enc_ctl->addr_type != 0xffff ||
>>>> enc_ctl->addr_type != FLOW_DISSECTOR_KEY_IPV4_ADDRS)
>>>> return -EOPNOTSUPP;
>>>> @@ -194,6 +198,9 @@ nfp_flower_calculate_key_layers(struct nfp_fl_key_ls *ret_key_ls,
>>>>
>>>> key_layer |= NFP_FLOWER_LAYER_VXLAN;
>>>> key_size += sizeof(struct nfp_flower_vxlan);
>>>> + } else if (egress) {
>>>> + /* Reject non tunnel matches offloaded to egress repr. */
>>>> + return -EOPNOTSUPP;
>>>> }
>>>
>>> with these two hunks we get: egress <- IFF -> encap match, right?
>>>
>>> (1) we can't offload the egress way if there isn't matching on encap headers
>>> (2) we can't go the matching on encap headers way if we are not egress
>>>
>>
>> yes, this is correct.
>> With the block code and egdev offload, we do not have access to the
>> ingress netdev when doing an offload.
>> We need to use the encap headers (especially the enc_port) to
>> distinguish the type of tunnel used and, therefore, require that the
>> encap matches be present before offloading.
>>
>>> what other cases are rejected by this logic?
>>>
>>
>> Yes, some other cases may be rejected (like veth mentioned below).
>
> my claim is that the veth case I mentioned below will not be rejected
> if it has the matching on encap headers, and a wrong rule will be set
> into hw, agree?
>
yes, unfortunately this is correct.
Without having access to the ingress netdev we have to put as many
restrictions as possible to ensure it is 'almost certainly' a given
ingress netdev but extreme cases can bypass this.
>> However, this is better than allowing rules to be incorrectly
>> offloaded (as could have happened before these changes).
>
>> Currently, we are looking at offloading flows on other ingress devices
>> such as bonds so this will require a change to the driver code here.
>
> for the ingress side, Jiri suggested that the slave devices (uplink reps),
> will be just getting all the rules set on the bond, so I am not sure what
> problem you see here... for decap it will be still vxlan --> vf rep and your
> egress logic will allow it.
>
Yes, Jiri suggested on another thread that the bonds simply relay
rules to their slaves.
This will work fine if uplink reprs are enslaved by a bond before
rules are added to it.
It would also assume that uplink reprs are not removed from/added to
the bond at later stages.
Doing this would require flushing the bond rules or writing all
existing rules to one of the slaves but not others.
Do you have any opinions on handling such situations?
>> IMO, the cleanest solution will also require tc core changes to either
>> avoid egdev offload or to have access to the ingress netdev of a rule.
>
>>> e.g If we add a rule with SW device (veth. tap) being the ingress, and
>>> HW device (vf rep)
>>> being the egress while not using skip_sw (just no flags == both) we
>>> get the TC stack
>>> go along the egdev callback from the vf rep hw device and add an
>>> (uplink --> vf rep) rule
>>> which will not be rejected if there is matching on tunnel headers, it
>>> will also not be rejected
>>> by some driver logic as the one we discussed to identify and ignore
>>> rules that are attempted to being added twice.
>>>
>>> Or.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net 5/5] nfp: remove false positive offloads in flower vxlan
2018-04-18 22:31 ` John Hurley
@ 2018-04-19 21:11 ` Or Gerlitz
0 siblings, 0 replies; 12+ messages in thread
From: Or Gerlitz @ 2018-04-19 21:11 UTC (permalink / raw)
To: John Hurley; +Cc: Jakub Kicinski, Linux Netdev List, oss-drivers, Simon Horman
On Thu, Apr 19, 2018 at 1:31 AM, John Hurley <john.hurley@netronome.com> wrote:
> On Wed, Apr 18, 2018 at 7:18 PM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
>> On Wed, Apr 18, 2018 at 3:31 PM, John Hurley <john.hurley@netronome.com> wrote:
>>> On Wed, Apr 18, 2018 at 8:43 AM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
>>>> On Fri, Nov 17, 2017 at 4:06 AM, Jakub Kicinski
>>>> <jakub.kicinski@netronome.com> wrote:
>>>>> From: John Hurley <john.hurley@netronome.com>
>>>>>
>>>>> Pass information to the match offload on whether or not the repr is the
>>>>> ingress or egress dev. Only accept tunnel matches if repr is the egress dev.
>>>>>
>>>>> This means rules such as the following are successfully offloaded:
>>>>> tc .. add dev vxlan0 .. enc_dst_port 4789 .. action redirect dev nfp_p0
>>>>>
>>>>> While rules such as the following are rejected:
>>>>> tc .. add dev nfp_p0 .. enc_dst_port 4789 .. action redirect dev vxlan0
>>>>
>>>> cool
>>>>
>>>>
>>>>> Also reject non tunnel flows that are offloaded to an egress dev.
>>>>> Non tunnel matches assume that the offload dev is the ingress port and
>>>>> offload a match accordingly.
>>>>
>>>> not following on the "Also" here, see below
>>>>
>>>>
>>>>> diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
>>>>> index a0193e0c24a0..f5d73b83dcc2 100644
>>>>> --- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
>>>>> +++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
>>>>> @@ -131,7 +131,8 @@ static bool nfp_flower_check_higher_than_mac(struct tc_cls_flower_offload *f)
>>>>>
>>>>> static int
>>>>> nfp_flower_calculate_key_layers(struct nfp_fl_key_ls *ret_key_ls,
>>>>> - struct tc_cls_flower_offload *flow)
>>>>> + struct tc_cls_flower_offload *flow,
>>>>> + bool egress)
>>>>> {
>>>>> struct flow_dissector_key_basic *mask_basic = NULL;
>>>>> struct flow_dissector_key_basic *key_basic = NULL;
>>>>> @@ -167,6 +168,9 @@ nfp_flower_calculate_key_layers(struct nfp_fl_key_ls *ret_key_ls,
>>>>> skb_flow_dissector_target(flow->dissector,
>>>>> FLOW_DISSECTOR_KEY_ENC_CONTROL,
>>>>> flow->key);
>>>>> + if (!egress)
>>>>> + return -EOPNOTSUPP;
>>>>> +
>>>>> if (mask_enc_ctl->addr_type != 0xffff ||
>>>>> enc_ctl->addr_type != FLOW_DISSECTOR_KEY_IPV4_ADDRS)
>>>>> return -EOPNOTSUPP;
>>>>> @@ -194,6 +198,9 @@ nfp_flower_calculate_key_layers(struct nfp_fl_key_ls *ret_key_ls,
>>>>>
>>>>> key_layer |= NFP_FLOWER_LAYER_VXLAN;
>>>>> key_size += sizeof(struct nfp_flower_vxlan);
>>>>> + } else if (egress) {
>>>>> + /* Reject non tunnel matches offloaded to egress repr. */
>>>>> + return -EOPNOTSUPP;
>>>>> }
>>>>
>>>> with these two hunks we get: egress <- IFF -> encap match, right?
>>>>
>>>> (1) we can't offload the egress way if there isn't matching on encap headers
>>>> (2) we can't go the matching on encap headers way if we are not egress
>>>>
>>>
>>> yes, this is correct.
>>> With the block code and egdev offload, we do not have access to the
>>> ingress netdev when doing an offload.
>>> We need to use the encap headers (especially the enc_port) to
>>> distinguish the type of tunnel used and, therefore, require that the
>>> encap matches be present before offloading.
>>>
>>>> what other cases are rejected by this logic?
>>>>
>>>
>>> Yes, some other cases may be rejected (like veth mentioned below).
>>
>> my claim is that the veth case I mentioned below will not be rejected
>> if it has the matching on encap headers, and a wrong rule will be set
>> into hw, agree?
>>
>
> yes, unfortunately this is correct.
> Without having access to the ingress netdev we have to put as many
> restrictions as possible to ensure it is 'almost certainly' a given
> ingress netdev but extreme cases can bypass this.
>
>>> However, this is better than allowing rules to be incorrectly
>>> offloaded (as could have happened before these changes).
>>
>>> Currently, we are looking at offloading flows on other ingress devices
>>> such as bonds so this will require a change to the driver code here.
>>
>> for the ingress side, Jiri suggested that the slave devices (uplink reps),
>> will be just getting all the rules set on the bond, so I am not sure what
>> problem you see here... for decap it will be still vxlan --> vf rep and your
>> egress logic will allow it.
>>
>
> Yes, Jiri suggested on another thread that the bonds simply relay
> rules to their slaves.
> This will work fine if uplink reprs are enslaved by a bond before
> rules are added to it.
> It would also assume that uplink reprs are not removed from/added to
> the bond at later stages.
> Doing this would require flushing the bond rules or writing all
> existing rules to one of the slaves but not others.
> Do you have any opinions on handling such situations?
I looked now on the thread you've posted lately, there were some responses
on the matters you brought here. We'll (MLNX) get there soon I guess too.
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2018-04-19 21:11 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-11-17 1:06 [PATCH net 0/5] nfp: flower fixes and typo in ethtool stats name Jakub Kicinski
2017-11-17 1:06 ` [PATCH net 1/5] nfp: fix flower offload metadata flag usage Jakub Kicinski
2017-11-17 1:06 ` [PATCH net 2/5] nfp: fix vlan receive MAC statistics typo Jakub Kicinski
2017-11-17 1:06 ` [PATCH net 3/5] nfp: inherit the max_mtu from the PF netdev Jakub Kicinski
2017-11-17 1:06 ` [PATCH net 4/5] nfp: register flower reprs for egress dev offload Jakub Kicinski
2017-11-17 1:06 ` [PATCH net 5/5] nfp: remove false positive offloads in flower vxlan Jakub Kicinski
2018-04-18 7:43 ` Or Gerlitz
2018-04-18 12:31 ` John Hurley
2018-04-18 18:18 ` Or Gerlitz
2018-04-18 22:31 ` John Hurley
2018-04-19 21:11 ` Or Gerlitz
2017-11-17 5:10 ` [PATCH net 0/5] nfp: flower fixes and typo in ethtool stats name David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).