All of lore.kernel.org
 help / color / mirror / Atom feed
* [patch net-next 0/4] net: sched: block callbacks follow-up
@ 2017-10-25 14:34 Jiri Pirko
  2017-10-25 14:34 ` [patch net-next 1/4] net: sched: remove unused tc_should_offload helper Jiri Pirko
                   ` (4 more replies)
  0 siblings, 5 replies; 14+ messages in thread
From: Jiri Pirko @ 2017-10-25 14:34 UTC (permalink / raw)
  To: netdev
  Cc: davem, jhs, xiyou.wangcong, mlxsw, andrew, vivien.didelot,
	f.fainelli, michael.chan, ganeshgr, jeffrey.t.kirsher, saeedm,
	matanb, leonro, idosch, jakub.kicinski, simon.horman,
	pieter.jansenvanvuuren, john.hurley, alexander.h.duyck

From: Jiri Pirko <jiri@mellanox.com>

This patchset does a bit of cleanup of leftovers after block callbacks
patchset. The main part is patch 2, which restores the original handling
of tc offload feature flag.

Jiri Pirko (4):
  net: sched: remove unused tc_should_offload helper
  net: sched: move the can_offload check from binding phase to rule
    insertion phase
  net: sched: remove tc_can_offload check from egdev call
  net: sched: remove ndo_setup_tc check from tc_can_offload

 drivers/net/ethernet/broadcom/bnxt/bnxt.c           |  2 +-
 drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c       |  3 +++
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c     |  3 +++
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c       |  3 +++
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c   |  3 +++
 drivers/net/ethernet/mellanox/mlx5/core/en_rep.c    |  3 +++
 drivers/net/ethernet/mellanox/mlxsw/spectrum.c      |  3 +++
 drivers/net/ethernet/netronome/nfp/bpf/main.c       |  3 +++
 drivers/net/ethernet/netronome/nfp/flower/offload.c |  3 +++
 include/net/pkt_cls.h                               | 13 +------------
 net/dsa/slave.c                                     |  3 +++
 net/sched/cls_api.c                                 |  4 ++--
 12 files changed, 31 insertions(+), 15 deletions(-)

-- 
2.9.5

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [patch net-next 1/4] net: sched: remove unused tc_should_offload helper
  2017-10-25 14:34 [patch net-next 0/4] net: sched: block callbacks follow-up Jiri Pirko
@ 2017-10-25 14:34 ` Jiri Pirko
  2017-10-25 14:34 ` [patch net-next 2/4] net: sched: move the can_offload check from binding phase to rule insertion phase Jiri Pirko
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 14+ messages in thread
From: Jiri Pirko @ 2017-10-25 14:34 UTC (permalink / raw)
  To: netdev
  Cc: davem, jhs, xiyou.wangcong, mlxsw, andrew, vivien.didelot,
	f.fainelli, michael.chan, ganeshgr, jeffrey.t.kirsher, saeedm,
	matanb, leonro, idosch, jakub.kicinski, simon.horman,
	pieter.jansenvanvuuren, john.hurley, alexander.h.duyck

From: Jiri Pirko <jiri@mellanox.com>

tc_should_offload is no longer used, remove it.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 include/net/pkt_cls.h | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 04caa24..0d24d22 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -621,13 +621,6 @@ static inline bool tc_skip_hw(u32 flags)
 	return (flags & TCA_CLS_FLAGS_SKIP_HW) ? true : false;
 }
 
-static inline bool tc_should_offload(const struct net_device *dev, u32 flags)
-{
-	if (tc_skip_hw(flags))
-		return false;
-	return tc_can_offload(dev);
-}
-
 static inline bool tc_skip_sw(u32 flags)
 {
 	return (flags & TCA_CLS_FLAGS_SKIP_SW) ? true : false;
-- 
2.9.5

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [patch net-next 2/4] net: sched: move the can_offload check from binding phase to rule insertion phase
  2017-10-25 14:34 [patch net-next 0/4] net: sched: block callbacks follow-up Jiri Pirko
  2017-10-25 14:34 ` [patch net-next 1/4] net: sched: remove unused tc_should_offload helper Jiri Pirko
@ 2017-10-25 14:34 ` Jiri Pirko
  2017-10-25 22:09   ` Jakub Kicinski
  2017-10-26  1:01   ` David Miller
  2017-10-25 14:34 ` [patch net-next 3/4] net: sched: remove tc_can_offload check from egdev call Jiri Pirko
                   ` (2 subsequent siblings)
  4 siblings, 2 replies; 14+ messages in thread
From: Jiri Pirko @ 2017-10-25 14:34 UTC (permalink / raw)
  To: netdev
  Cc: davem, jhs, xiyou.wangcong, mlxsw, andrew, vivien.didelot,
	f.fainelli, michael.chan, ganeshgr, jeffrey.t.kirsher, saeedm,
	matanb, leonro, idosch, jakub.kicinski, simon.horman,
	pieter.jansenvanvuuren, john.hurley, alexander.h.duyck

From: Jiri Pirko <jiri@mellanox.com>

This restores the original behaviour before the block callbacks were
introduced. Allow the drivers to do binding of block always, no matter
if the NETIF_F_HW_TC feature is on or off. Move the check to the block
callback which is called for rule insertion.

Reported-by: Alexander Duyck <alexander.duyck@gmail.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c           | 2 +-
 drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c       | 3 +++
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c     | 3 +++
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c       | 3 +++
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c   | 3 +++
 drivers/net/ethernet/mellanox/mlx5/core/en_rep.c    | 3 +++
 drivers/net/ethernet/mellanox/mlxsw/spectrum.c      | 3 +++
 drivers/net/ethernet/netronome/nfp/bpf/main.c       | 3 +++
 drivers/net/ethernet/netronome/nfp/flower/offload.c | 3 +++
 net/dsa/slave.c                                     | 3 +++
 net/sched/cls_api.c                                 | 2 +-
 11 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 24d55724..e193232 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -7325,7 +7325,7 @@ static int bnxt_setup_tc_block_cb(enum tc_setup_type type, void *type_data,
 {
 	struct bnxt *bp = cb_priv;
 
-	if (BNXT_VF(bp))
+	if (!tc_can_offload(bp->dev) || BNXT_VF(bp))
 		return -EOPNOTSUPP;
 
 	switch (type) {
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
index 4ae9359..2fa614f 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
@@ -124,6 +124,9 @@ static int bnxt_vf_rep_setup_tc_block_cb(enum tc_setup_type type,
 	struct bnxt *bp = vf_rep->bp;
 	int vf_fid = bp->pf.vf[vf_rep->vf_idx].fw_fid;
 
+	if (!tc_can_offload(bp->dev))
+		return -EOPNOTSUPP;
+
 	switch (type) {
 	case TC_SETUP_CLSFLOWER:
 		return bnxt_tc_setup_flower(bp, vf_fid, type_data);
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
index e16078d..b07eecf 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
@@ -2935,6 +2935,9 @@ static int cxgb_setup_tc_block_cb(enum tc_setup_type type, void *type_data,
 		return -EINVAL;
 	}
 
+	if (!tc_can_offload(dev))
+		return -EOPNOTSUPP;
+
 	switch (type) {
 	case TC_SETUP_CLSU32:
 		return cxgb_setup_tc_cls_u32(dev, type_data);
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 7f503d3..3520f28 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -9392,6 +9392,9 @@ static int ixgbe_setup_tc_block_cb(enum tc_setup_type type, void *type_data,
 {
 	struct ixgbe_adapter *adapter = cb_priv;
 
+	if (!tc_can_offload(adapter->netdev))
+		return -EOPNOTSUPP;
+
 	switch (type) {
 	case TC_SETUP_CLSU32:
 		return ixgbe_setup_tc_cls_u32(adapter, type_data);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index 560b208..28ae00b 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -3106,6 +3106,9 @@ int mlx5e_setup_tc_block_cb(enum tc_setup_type type, void *type_data,
 {
 	struct mlx5e_priv *priv = cb_priv;
 
+	if (!tc_can_offload(priv->netdev))
+		return -EOPNOTSUPP;
+
 	switch (type) {
 	case TC_SETUP_CLSFLOWER:
 		return mlx5e_setup_tc_cls_flower(priv, type_data);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
index 0edb706..2c43606 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
@@ -682,6 +682,9 @@ static int mlx5e_rep_setup_tc_cb(enum tc_setup_type type, void *type_data,
 {
 	struct mlx5e_priv *priv = cb_priv;
 
+	if (!tc_can_offload(priv->netdev))
+		return -EOPNOTSUPP;
+
 	switch (type) {
 	case TC_SETUP_CLSFLOWER:
 		return mlx5e_rep_setup_tc_cls_flower(priv, type_data);
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index 8f8db6c..71aa603 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
@@ -1738,6 +1738,9 @@ static int mlxsw_sp_setup_tc_block_cb(enum tc_setup_type type, void *type_data,
 {
 	struct mlxsw_sp_port *mlxsw_sp_port = cb_priv;
 
+	if (!tc_can_offload(mlxsw_sp_port->dev))
+		return -EOPNOTSUPP;
+
 	switch (type) {
 	case TC_SETUP_CLSMATCHALL:
 		return mlxsw_sp_setup_tc_cls_matchall(mlxsw_sp_port, type_data,
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/main.c b/drivers/net/ethernet/netronome/nfp/bpf/main.c
index fa0ac90..55fca38 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/main.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/main.c
@@ -120,6 +120,9 @@ static int nfp_bpf_setup_tc_block_cb(enum tc_setup_type type,
 	struct tc_cls_bpf_offload *cls_bpf = type_data;
 	struct nfp_net *nn = cb_priv;
 
+	if (!tc_can_offload(nn->dp.netdev))
+		return -EOPNOTSUPP;
+
 	switch (type) {
 	case TC_SETUP_CLSBPF:
 		if (!nfp_net_ebpf_capable(nn) ||
diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
index c47753f..7c6cab1 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
@@ -470,6 +470,9 @@ static int nfp_flower_setup_tc_block_cb(enum tc_setup_type type,
 {
 	struct nfp_net *nn = cb_priv;
 
+	if (!tc_can_offload(nn->dp.netdev))
+		return -EOPNOTSUPP;
+
 	switch (type) {
 	case TC_SETUP_CLSFLOWER:
 		return nfp_flower_repr_offload(nn->app, nn->port->netdev,
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index d0ae701..3481fd6 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -799,6 +799,9 @@ static int dsa_slave_setup_tc_block_cb(enum tc_setup_type type, void *type_data,
 {
 	struct net_device *dev = cb_priv;
 
+	if (!tc_can_offload(dev))
+		return -EOPNOTSUPP;
+
 	switch (type) {
 	case TC_SETUP_CLSMATCHALL:
 		return dsa_slave_setup_tc_cls_matchall(dev, type_data, ingress);
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 0e96cda..0636c19 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -247,7 +247,7 @@ static void tcf_block_offload_cmd(struct tcf_block *block, struct Qdisc *q,
 	struct net_device *dev = q->dev_queue->dev;
 	struct tc_block_offload bo = {};
 
-	if (!tc_can_offload(dev))
+	if (!dev->netdev_ops->ndo_setup_tc)
 		return;
 	bo.command = command;
 	bo.binder_type = ei->binder_type;
-- 
2.9.5

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [patch net-next 3/4] net: sched: remove tc_can_offload check from egdev call
  2017-10-25 14:34 [patch net-next 0/4] net: sched: block callbacks follow-up Jiri Pirko
  2017-10-25 14:34 ` [patch net-next 1/4] net: sched: remove unused tc_should_offload helper Jiri Pirko
  2017-10-25 14:34 ` [patch net-next 2/4] net: sched: move the can_offload check from binding phase to rule insertion phase Jiri Pirko
@ 2017-10-25 14:34 ` Jiri Pirko
  2017-10-25 14:35 ` [patch net-next 4/4] net: sched: remove ndo_setup_tc check from tc_can_offload Jiri Pirko
  2017-10-29  9:36 ` [patch net-next 0/4] net: sched: block callbacks follow-up David Miller
  4 siblings, 0 replies; 14+ messages in thread
From: Jiri Pirko @ 2017-10-25 14:34 UTC (permalink / raw)
  To: netdev
  Cc: davem, jhs, xiyou.wangcong, mlxsw, andrew, vivien.didelot,
	f.fainelli, michael.chan, ganeshgr, jeffrey.t.kirsher, saeedm,
	matanb, leonro, idosch, jakub.kicinski, simon.horman,
	pieter.jansenvanvuuren, john.hurley, alexander.h.duyck

From: Jiri Pirko <jiri@mellanox.com>

Since the only user, mlx5 driver does the check in
mlx5e_setup_tc_block_cb, no need to check here.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 net/sched/cls_api.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 0636c19..81e1eb3 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -1194,7 +1194,7 @@ static int tc_exts_setup_cb_egdev_call(struct tcf_exts *exts,
 		if (!a->ops->get_dev)
 			continue;
 		dev = a->ops->get_dev(a);
-		if (!dev || !tc_can_offload(dev))
+		if (!dev)
 			continue;
 		ret = tc_setup_cb_egdev_call(dev, type, type_data, err_stop);
 		if (ret < 0)
-- 
2.9.5

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [patch net-next 4/4] net: sched: remove ndo_setup_tc check from tc_can_offload
  2017-10-25 14:34 [patch net-next 0/4] net: sched: block callbacks follow-up Jiri Pirko
                   ` (2 preceding siblings ...)
  2017-10-25 14:34 ` [patch net-next 3/4] net: sched: remove tc_can_offload check from egdev call Jiri Pirko
@ 2017-10-25 14:35 ` Jiri Pirko
  2017-10-29  9:36 ` [patch net-next 0/4] net: sched: block callbacks follow-up David Miller
  4 siblings, 0 replies; 14+ messages in thread
From: Jiri Pirko @ 2017-10-25 14:35 UTC (permalink / raw)
  To: netdev
  Cc: davem, jhs, xiyou.wangcong, mlxsw, andrew, vivien.didelot,
	f.fainelli, michael.chan, ganeshgr, jeffrey.t.kirsher, saeedm,
	matanb, leonro, idosch, jakub.kicinski, simon.horman,
	pieter.jansenvanvuuren, john.hurley, alexander.h.duyck

From: Jiri Pirko <jiri@mellanox.com>

Since tc_can_offload is always called from block callback or egdev
callback, no need to check if ndo_setup_tc exists.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 include/net/pkt_cls.h | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 0d24d22..4631f1a 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -609,11 +609,7 @@ struct tc_cls_u32_offload {
 
 static inline bool tc_can_offload(const struct net_device *dev)
 {
-	if (!(dev->features & NETIF_F_HW_TC))
-		return false;
-	if (!dev->netdev_ops->ndo_setup_tc)
-		return false;
-	return true;
+	return dev->features & NETIF_F_HW_TC;
 }
 
 static inline bool tc_skip_hw(u32 flags)
-- 
2.9.5

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [patch net-next 2/4] net: sched: move the can_offload check from binding phase to rule insertion phase
  2017-10-25 14:34 ` [patch net-next 2/4] net: sched: move the can_offload check from binding phase to rule insertion phase Jiri Pirko
@ 2017-10-25 22:09   ` Jakub Kicinski
  2017-10-26  1:01   ` David Miller
  1 sibling, 0 replies; 14+ messages in thread
From: Jakub Kicinski @ 2017-10-25 22:09 UTC (permalink / raw)
  To: Jiri Pirko
  Cc: netdev, davem, jhs, xiyou.wangcong, mlxsw, andrew,
	vivien.didelot, f.fainelli, michael.chan, ganeshgr,
	jeffrey.t.kirsher, saeedm, matanb, leonro, idosch, simon.horman,
	pieter.jansenvanvuuren, john.hurley, alexander.h.duyck

On Wed, 25 Oct 2017 16:34:58 +0200, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@mellanox.com>
> 
> This restores the original behaviour before the block callbacks were
> introduced. Allow the drivers to do binding of block always, no matter
> if the NETIF_F_HW_TC feature is on or off. Move the check to the block
> callback which is called for rule insertion.
> 
> Reported-by: Alexander Duyck <alexander.duyck@gmail.com>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>

Could you explain why not add this check to the core?  IIUC every
driver will have to duplicate it if we want to keep the behaviour
of tc offloads we have today.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [patch net-next 2/4] net: sched: move the can_offload check from binding phase to rule insertion phase
  2017-10-25 14:34 ` [patch net-next 2/4] net: sched: move the can_offload check from binding phase to rule insertion phase Jiri Pirko
  2017-10-25 22:09   ` Jakub Kicinski
@ 2017-10-26  1:01   ` David Miller
  2017-10-26  6:07     ` Jiri Pirko
  1 sibling, 1 reply; 14+ messages in thread
From: David Miller @ 2017-10-26  1:01 UTC (permalink / raw)
  To: jiri
  Cc: netdev, jhs, xiyou.wangcong, mlxsw, andrew, vivien.didelot,
	f.fainelli, michael.chan, ganeshgr, jeffrey.t.kirsher, saeedm,
	matanb, leonro, idosch, jakub.kicinski, simon.horman,
	pieter.jansenvanvuuren, john.hurley, alexander.h.duyck

From: Jiri Pirko <jiri@resnulli.us>
Date: Wed, 25 Oct 2017 16:34:58 +0200

> From: Jiri Pirko <jiri@mellanox.com>
> 
> This restores the original behaviour before the block callbacks were
> introduced. Allow the drivers to do binding of block always, no matter
> if the NETIF_F_HW_TC feature is on or off. Move the check to the block
> callback which is called for rule insertion.
> 
> Reported-by: Alexander Duyck <alexander.duyck@gmail.com>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>

I agree with Jakub's feedback, if every callback has to make this check
why not just do it in the core where the callback is invoked?

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [patch net-next 2/4] net: sched: move the can_offload check from binding phase to rule insertion phase
  2017-10-26  1:01   ` David Miller
@ 2017-10-26  6:07     ` Jiri Pirko
  2017-10-26 15:29       ` Alexander Duyck
  0 siblings, 1 reply; 14+ messages in thread
From: Jiri Pirko @ 2017-10-26  6:07 UTC (permalink / raw)
  To: David Miller
  Cc: netdev, jhs, xiyou.wangcong, mlxsw, andrew, vivien.didelot,
	f.fainelli, michael.chan, ganeshgr, jeffrey.t.kirsher, saeedm,
	matanb, leonro, idosch, jakub.kicinski, simon.horman,
	pieter.jansenvanvuuren, john.hurley, alexander.h.duyck

Thu, Oct 26, 2017 at 03:01:31AM CEST, davem@davemloft.net wrote:
>From: Jiri Pirko <jiri@resnulli.us>
>Date: Wed, 25 Oct 2017 16:34:58 +0200
>
>> From: Jiri Pirko <jiri@mellanox.com>
>> 
>> This restores the original behaviour before the block callbacks were
>> introduced. Allow the drivers to do binding of block always, no matter
>> if the NETIF_F_HW_TC feature is on or off. Move the check to the block
>> callback which is called for rule insertion.
>> 
>> Reported-by: Alexander Duyck <alexander.duyck@gmail.com>
>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>
>I agree with Jakub's feedback, if every callback has to make this check
>why not just do it in the core where the callback is invoked?

We cannot add it to the core. The idea of the block callbacks is to get
independent of the "dev". For the block sharing, driver registers one
block callback for multiple devs. Core has no access to "dev".

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [patch net-next 2/4] net: sched: move the can_offload check from binding phase to rule insertion phase
  2017-10-26  6:07     ` Jiri Pirko
@ 2017-10-26 15:29       ` Alexander Duyck
  2017-10-26 15:40         ` Jiri Pirko
  0 siblings, 1 reply; 14+ messages in thread
From: Alexander Duyck @ 2017-10-26 15:29 UTC (permalink / raw)
  To: Jiri Pirko
  Cc: David Miller, Netdev, Jamal Hadi Salim, Cong Wang, mlxsw,
	Andrew Lunn, Vivien Didelot, Florian Fainelli, Michael Chan,
	Ganesh Goudar, Jeff Kirsher, Saeed Mahameed, Matan Barak,
	Leon Romanovsky, idosch, Jakub Kicinski, Simon Horman,
	pieter.jansenvanvuuren, john.hurley, Duyck, Alexander H

On Wed, Oct 25, 2017 at 11:07 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Thu, Oct 26, 2017 at 03:01:31AM CEST, davem@davemloft.net wrote:
>>From: Jiri Pirko <jiri@resnulli.us>
>>Date: Wed, 25 Oct 2017 16:34:58 +0200
>>
>>> From: Jiri Pirko <jiri@mellanox.com>
>>>
>>> This restores the original behaviour before the block callbacks were
>>> introduced. Allow the drivers to do binding of block always, no matter
>>> if the NETIF_F_HW_TC feature is on or off. Move the check to the block
>>> callback which is called for rule insertion.
>>>
>>> Reported-by: Alexander Duyck <alexander.duyck@gmail.com>
>>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>>
>>I agree with Jakub's feedback, if every callback has to make this check
>>why not just do it in the core where the callback is invoked?
>
> We cannot add it to the core. The idea of the block callbacks is to get
> independent of the "dev". For the block sharing, driver registers one
> block callback for multiple devs. Core has no access to "dev".

If the plan is to have one block represent multiple devices are we
then looking at the offload flag potentially being tied together for
all those devices then at some point in the future? I assume that
means that this workaround will have to eventually go away for some of
the Mellanox devices since it wouldn't make sense to have a netdev
pointer if the context block supports multiple netdevices. Do I have
that right? If so, is the idea to just assume the offload is always-on
(fixed) or do you have some other idea in mind for how to deal with a
feature that is controlled globally for the device versus a port?

- Alex

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [patch net-next 2/4] net: sched: move the can_offload check from binding phase to rule insertion phase
  2017-10-26 15:29       ` Alexander Duyck
@ 2017-10-26 15:40         ` Jiri Pirko
  2017-10-26 20:55           ` Jakub Kicinski
  0 siblings, 1 reply; 14+ messages in thread
From: Jiri Pirko @ 2017-10-26 15:40 UTC (permalink / raw)
  To: Alexander Duyck
  Cc: David Miller, Netdev, Jamal Hadi Salim, Cong Wang, mlxsw,
	Andrew Lunn, Vivien Didelot, Florian Fainelli, Michael Chan,
	Ganesh Goudar, Jeff Kirsher, Saeed Mahameed, Matan Barak,
	Leon Romanovsky, idosch, Jakub Kicinski, Simon Horman,
	pieter.jansenvanvuuren, john.hurley, Duyck, Alexander H

Thu, Oct 26, 2017 at 05:29:08PM CEST, alexander.duyck@gmail.com wrote:
>On Wed, Oct 25, 2017 at 11:07 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Thu, Oct 26, 2017 at 03:01:31AM CEST, davem@davemloft.net wrote:
>>>From: Jiri Pirko <jiri@resnulli.us>
>>>Date: Wed, 25 Oct 2017 16:34:58 +0200
>>>
>>>> From: Jiri Pirko <jiri@mellanox.com>
>>>>
>>>> This restores the original behaviour before the block callbacks were
>>>> introduced. Allow the drivers to do binding of block always, no matter
>>>> if the NETIF_F_HW_TC feature is on or off. Move the check to the block
>>>> callback which is called for rule insertion.
>>>>
>>>> Reported-by: Alexander Duyck <alexander.duyck@gmail.com>
>>>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>>>
>>>I agree with Jakub's feedback, if every callback has to make this check
>>>why not just do it in the core where the callback is invoked?
>>
>> We cannot add it to the core. The idea of the block callbacks is to get
>> independent of the "dev". For the block sharing, driver registers one
>> block callback for multiple devs. Core has no access to "dev".
>
>If the plan is to have one block represent multiple devices are we
>then looking at the offload flag potentially being tied together for
>all those devices then at some point in the future? I assume that
>means that this workaround will have to eventually go away for some of
>the Mellanox devices since it wouldn't make sense to have a netdev
>pointer if the context block supports multiple netdevices. Do I have
>that right? If so, is the idea to just assume the offload is always-on

You do.

>(fixed) or do you have some other idea in mind for how to deal with a
>feature that is controlled globally for the device versus a port?

I plan to check the offload feature flag for all ports that share the
block and allow the offload only in case all are on.
Also, there would be forbidden to turn the offload flag off in case
there is at least one offloaded rule for the block. This is how mlx5
does it.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [patch net-next 2/4] net: sched: move the can_offload check from binding phase to rule insertion phase
  2017-10-26 15:40         ` Jiri Pirko
@ 2017-10-26 20:55           ` Jakub Kicinski
  0 siblings, 0 replies; 14+ messages in thread
From: Jakub Kicinski @ 2017-10-26 20:55 UTC (permalink / raw)
  To: Jiri Pirko
  Cc: Alexander Duyck, David Miller, Netdev, Jamal Hadi Salim,
	Cong Wang, mlxsw, Andrew Lunn, Vivien Didelot, Florian Fainelli,
	Michael Chan, Ganesh Goudar, Jeff Kirsher, Saeed Mahameed,
	Matan Barak, Leon Romanovsky, idosch, Simon Horman,
	pieter.jansenvanvuuren, john.hurley, Duyck, Alexander H

On Thu, 26 Oct 2017 17:40:19 +0200, Jiri Pirko wrote:
> Thu, Oct 26, 2017 at 05:29:08PM CEST, alexander.duyck@gmail.com wrote:
> >On Wed, Oct 25, 2017 at 11:07 PM, Jiri Pirko <jiri@resnulli.us> wrote:  
> >> Thu, Oct 26, 2017 at 03:01:31AM CEST, davem@davemloft.net wrote:  
> >>>From: Jiri Pirko <jiri@resnulli.us>
> >>>Date: Wed, 25 Oct 2017 16:34:58 +0200
> >>>  
> >>>> From: Jiri Pirko <jiri@mellanox.com>
> >>>>
> >>>> This restores the original behaviour before the block callbacks were
> >>>> introduced. Allow the drivers to do binding of block always, no matter
> >>>> if the NETIF_F_HW_TC feature is on or off. Move the check to the block
> >>>> callback which is called for rule insertion.
> >>>>
> >>>> Reported-by: Alexander Duyck <alexander.duyck@gmail.com>
> >>>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>  
> >>>
> >>>I agree with Jakub's feedback, if every callback has to make this check
> >>>why not just do it in the core where the callback is invoked?  
> >>
> >> We cannot add it to the core. The idea of the block callbacks is to get
> >> independent of the "dev". For the block sharing, driver registers one
> >> block callback for multiple devs. Core has no access to "dev".  
> >
> >If the plan is to have one block represent multiple devices are we
> >then looking at the offload flag potentially being tied together for
> >all those devices then at some point in the future? I assume that
> >means that this workaround will have to eventually go away for some of
> >the Mellanox devices since it wouldn't make sense to have a netdev
> >pointer if the context block supports multiple netdevices. Do I have
> >that right? If so, is the idea to just assume the offload is always-on  
> 
> You do.
> 
> >(fixed) or do you have some other idea in mind for how to deal with a
> >feature that is controlled globally for the device versus a port?  
> 
> I plan to check the offload feature flag for all ports that share the
> block and allow the offload only in case all are on.

Clearly we draw the line between what should the responsibility of the
driver do and what belongs higher in the stack differently :)

> Also, there would be forbidden to turn the offload flag off in case
> there is at least one offloaded rule for the block. This is how mlx5
> does it.

My grep-foo fails me I only see that check in nfp.  I though John
already did that in ixgbe.  And then everyone copied, since it's
sort-of UAPI?  Or at least users may come to expect certain
consistent behaviour.  Same story for phys_port_name for representors,
and OpenStack expect certain names by now.  That copying of behaviour
concerns me, not necessarily multiplication of code.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [patch net-next 0/4] net: sched: block callbacks follow-up
  2017-10-25 14:34 [patch net-next 0/4] net: sched: block callbacks follow-up Jiri Pirko
                   ` (3 preceding siblings ...)
  2017-10-25 14:35 ` [patch net-next 4/4] net: sched: remove ndo_setup_tc check from tc_can_offload Jiri Pirko
@ 2017-10-29  9:36 ` David Miller
  2017-10-29 11:17   ` Jiri Pirko
  4 siblings, 1 reply; 14+ messages in thread
From: David Miller @ 2017-10-29  9:36 UTC (permalink / raw)
  To: jiri
  Cc: netdev, jhs, xiyou.wangcong, mlxsw, andrew, vivien.didelot,
	f.fainelli, michael.chan, ganeshgr, jeffrey.t.kirsher, saeedm,
	matanb, leonro, idosch, jakub.kicinski, simon.horman,
	pieter.jansenvanvuuren, john.hurley, alexander.h.duyck

From: Jiri Pirko <jiri@resnulli.us>
Date: Wed, 25 Oct 2017 16:34:56 +0200

> This patchset does a bit of cleanup of leftovers after block callbacks
> patchset. The main part is patch 2, which restores the original handling
> of tc offload feature flag.

Jiri, this series does not apply cleanly to net-next.

Also, you should really work out the block offload semantics
with Jakub, continuing the discussion of patch #2.

Thanks.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [patch net-next 0/4] net: sched: block callbacks follow-up
  2017-10-29  9:36 ` [patch net-next 0/4] net: sched: block callbacks follow-up David Miller
@ 2017-10-29 11:17   ` Jiri Pirko
  2017-11-01 10:41     ` Jiri Pirko
  0 siblings, 1 reply; 14+ messages in thread
From: Jiri Pirko @ 2017-10-29 11:17 UTC (permalink / raw)
  To: David Miller
  Cc: netdev, jhs, xiyou.wangcong, mlxsw, andrew, vivien.didelot,
	f.fainelli, michael.chan, ganeshgr, jeffrey.t.kirsher, saeedm,
	matanb, leonro, idosch, jakub.kicinski, simon.horman,
	pieter.jansenvanvuuren, john.hurley, alexander.h.duyck

Sun, Oct 29, 2017 at 10:36:32AM CET, davem@davemloft.net wrote:
>From: Jiri Pirko <jiri@resnulli.us>
>Date: Wed, 25 Oct 2017 16:34:56 +0200
>
>> This patchset does a bit of cleanup of leftovers after block callbacks
>> patchset. The main part is patch 2, which restores the original handling
>> of tc offload feature flag.
>
>Jiri, this series does not apply cleanly to net-next.

I will respin.


>
>Also, you should really work out the block offload semantics
>with Jakub, continuing the discussion of patch #2.

I have some ideas. I could let driver to enable/disable already
registered callback. Will look at it.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [patch net-next 0/4] net: sched: block callbacks follow-up
  2017-10-29 11:17   ` Jiri Pirko
@ 2017-11-01 10:41     ` Jiri Pirko
  0 siblings, 0 replies; 14+ messages in thread
From: Jiri Pirko @ 2017-11-01 10:41 UTC (permalink / raw)
  To: David Miller
  Cc: netdev, jhs, xiyou.wangcong, mlxsw, andrew, vivien.didelot,
	f.fainelli, michael.chan, ganeshgr, jeffrey.t.kirsher, saeedm,
	matanb, leonro, idosch, jakub.kicinski, simon.horman,
	pieter.jansenvanvuuren, john.hurley, alexander.h.duyck

Sun, Oct 29, 2017 at 12:17:14PM CET, jiri@resnulli.us wrote:
>Sun, Oct 29, 2017 at 10:36:32AM CET, davem@davemloft.net wrote:
>>From: Jiri Pirko <jiri@resnulli.us>
>>Date: Wed, 25 Oct 2017 16:34:56 +0200
>>
>>> This patchset does a bit of cleanup of leftovers after block callbacks
>>> patchset. The main part is patch 2, which restores the original handling
>>> of tc offload feature flag.
>>
>>Jiri, this series does not apply cleanly to net-next.
>
>I will respin.
>
>
>>
>>Also, you should really work out the block offload semantics
>>with Jakub, continuing the discussion of patch #2.
>
>I have some ideas. I could let driver to enable/disable already
>registered callback. Will look at it.

The cb enable disable according to NETIF_F_HW_TC ethtool flag could be
done. It will require some additional work in many drivers. Most of the
drivers do not effect the flag change.

I will rebase and repost this patchset for now to fix the regression
and work on NETIF_F_HW_TC handling in a follow-up.

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2017-11-01 10:41 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-25 14:34 [patch net-next 0/4] net: sched: block callbacks follow-up Jiri Pirko
2017-10-25 14:34 ` [patch net-next 1/4] net: sched: remove unused tc_should_offload helper Jiri Pirko
2017-10-25 14:34 ` [patch net-next 2/4] net: sched: move the can_offload check from binding phase to rule insertion phase Jiri Pirko
2017-10-25 22:09   ` Jakub Kicinski
2017-10-26  1:01   ` David Miller
2017-10-26  6:07     ` Jiri Pirko
2017-10-26 15:29       ` Alexander Duyck
2017-10-26 15:40         ` Jiri Pirko
2017-10-26 20:55           ` Jakub Kicinski
2017-10-25 14:34 ` [patch net-next 3/4] net: sched: remove tc_can_offload check from egdev call Jiri Pirko
2017-10-25 14:35 ` [patch net-next 4/4] net: sched: remove ndo_setup_tc check from tc_can_offload Jiri Pirko
2017-10-29  9:36 ` [patch net-next 0/4] net: sched: block callbacks follow-up David Miller
2017-10-29 11:17   ` Jiri Pirko
2017-11-01 10:41     ` Jiri Pirko

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.