| Message ID | 20180123213340.19235-2-jakub.kicinski@netronome.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | use tc_can_offload_extack() throughout the drivers | expand |
Tue, Jan 23, 2018 at 10:33:33PM CET, jakub.kicinski@netronome.com wrote: >No upstream drivers seem to allow offload of chains other than 0. mlxsw does. And I know that Intel was talking about adding the support to i40e iirc. >Save driver developers typing and make tc_can_offload_extack() >check for that condition as well. Rename the function to >tc_can_offload_cls() to better represent its application. > >Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> >--- [...] >--- a/include/net/pkt_cls.h >+++ b/include/net/pkt_cls.h >@@ -645,15 +645,21 @@ static inline bool tc_can_offload(const struct net_device *dev) > return dev->features & NETIF_F_HW_TC; > } > >-static inline bool tc_can_offload_extack(const struct net_device *dev, >- struct netlink_ext_ack *extack) >+static inline bool tc_can_offload_cls(const struct net_device *dev, >+ struct tc_cls_common_offload *common) > { >- bool can = tc_can_offload(dev); >- >- if (!can) >- NL_SET_ERR_MSG(extack, "TC offload is disabled on net device"); >+ if (common->chain_index) { This check here is wrong. It is up to the driver if it supports multichain offload or not, should not be checked in a generic code along with tc_can_offload. >+ NL_SET_ERR_MSG(common->extack, >+ "Driver supports only offload of chain 0"); >+ return false; >+ } >+ if (!tc_can_offload(dev)) { >+ NL_SET_ERR_MSG(common->extack, >+ "TC offload is disabled on net device"); >+ return false; >+ } > >- return can; >+ return true; > } > > static inline bool tc_skip_hw(u32 flags) >-- >2.15.1 >
On Wed, 24 Jan 2018 09:28:44 +0100, Jiri Pirko wrote: > Tue, Jan 23, 2018 at 10:33:33PM CET, jakub.kicinski@netronome.com wrote: > >No upstream drivers seem to allow offload of chains other than 0. > > mlxsw does. And I know that Intel was talking about adding the support > to i40e iirc. An, I wrote that commit message before looking deeper into mlxsw. My understanding is that you only use the simple tc_can_offload() check for matchall egress redirect. For flower which actually makes use of multiple chains, you also allow block sharing and have the driver count the number of ports with tc offloads disabled. Therefore you don't use tc_can_offload() there. Is that correct? > >Save driver developers typing and make tc_can_offload_extack() > >check for that condition as well. Rename the function to > >tc_can_offload_cls() to better represent its application. > > > >Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> > >--- > > [...] > > >--- a/include/net/pkt_cls.h > >+++ b/include/net/pkt_cls.h > >@@ -645,15 +645,21 @@ static inline bool tc_can_offload(const struct net_device *dev) > > return dev->features & NETIF_F_HW_TC; > > } > > > >-static inline bool tc_can_offload_extack(const struct net_device *dev, > >- struct netlink_ext_ack *extack) > >+static inline bool tc_can_offload_cls(const struct net_device *dev, > >+ struct tc_cls_common_offload *common) > > { > >- bool can = tc_can_offload(dev); > >- > >- if (!can) > >- NL_SET_ERR_MSG(extack, "TC offload is disabled on net device"); > >+ if (common->chain_index) { > > This check here is wrong. It is up to the driver if it supports > multichain offload or not, should not be checked in a generic code along > with tc_can_offload. Mm.. maybe the name is a bit unfortunate now.. How about keeping tc_can_offload_extack() as is and adding: static inline bool tc_can_offload_simple(const struct net_device *dev, struct tc_cls_common_offload *common) { if (common->chain_index) { NL_SET_ERR_MSG(common->extack, "Driver supports only offload of chain 0"); return false; } return tc_can_offload_extack(dev, common->extack); } "simple" standing for simple driver. My gut feeling is that the drivers will fall into two categories: simple drivers which don't share blocks and chains and advanced drivers which do what mlxsw does and therefore doesn't use those helpers. IOW there will be no tc_can_offload_extack() callers. CCing Intel folks.
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/main.c b/drivers/net/ethernet/netronome/nfp/bpf/main.c index b3206855535a..552e2657b536 100644 --- a/drivers/net/ethernet/netronome/nfp/bpf/main.c +++ b/drivers/net/ethernet/netronome/nfp/bpf/main.c @@ -130,7 +130,7 @@ static int nfp_bpf_setup_tc_block_cb(enum tc_setup_type type, "only offload of BPF classifiers supported"); return -EOPNOTSUPP; } - if (!tc_can_offload_extack(nn->dp.netdev, cls_bpf->common.extack)) + if (!tc_can_offload_cls(nn->dp.netdev, &cls_bpf->common)) return -EOPNOTSUPP; if (!nfp_net_ebpf_capable(nn)) { NL_SET_ERR_MSG_MOD(cls_bpf->common.extack, @@ -142,8 +142,6 @@ static int nfp_bpf_setup_tc_block_cb(enum tc_setup_type type, "only ETH_P_ALL supported as filter protocol"); return -EOPNOTSUPP; } - if (cls_bpf->common.chain_index) - return -EOPNOTSUPP; /* Only support TC direct action */ if (!cls_bpf->exts_integrated || diff --git a/drivers/net/netdevsim/bpf.c b/drivers/net/netdevsim/bpf.c index 8166f121bbcc..bbcb8ec42208 100644 --- a/drivers/net/netdevsim/bpf.c +++ b/drivers/net/netdevsim/bpf.c @@ -135,7 +135,7 @@ int nsim_bpf_setup_tc_block_cb(enum tc_setup_type type, return -EOPNOTSUPP; } - if (!tc_can_offload_extack(ns->netdev, cls_bpf->common.extack)) + if (!tc_can_offload_cls(ns->netdev, &cls_bpf->common)) return -EOPNOTSUPP; if (cls_bpf->common.protocol != htons(ETH_P_ALL)) { @@ -144,9 +144,6 @@ int nsim_bpf_setup_tc_block_cb(enum tc_setup_type type, return -EOPNOTSUPP; } - if (cls_bpf->common.chain_index) - return -EOPNOTSUPP; - if (!ns->bpf_tc_accept) { NSIM_EA(cls_bpf->common.extack, "netdevsim configured to reject BPF TC offload"); diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h index 1a41513cec7f..84932226da67 100644 --- a/include/net/pkt_cls.h +++ b/include/net/pkt_cls.h @@ -645,15 +645,21 @@ static inline bool tc_can_offload(const struct net_device *dev) return dev->features & NETIF_F_HW_TC; } -static inline bool tc_can_offload_extack(const struct net_device *dev, - struct netlink_ext_ack *extack) +static inline bool tc_can_offload_cls(const struct net_device *dev, + struct tc_cls_common_offload *common) { - bool can = tc_can_offload(dev); - - if (!can) - NL_SET_ERR_MSG(extack, "TC offload is disabled on net device"); + if (common->chain_index) { + NL_SET_ERR_MSG(common->extack, + "Driver supports only offload of chain 0"); + return false; + } + if (!tc_can_offload(dev)) { + NL_SET_ERR_MSG(common->extack, + "TC offload is disabled on net device"); + return false; + } - return can; + return true; } static inline bool tc_skip_hw(u32 flags)
No upstream drivers seem to allow offload of chains other than 0. Save driver developers typing and make tc_can_offload_extack() check for that condition as well. Rename the function to tc_can_offload_cls() to better represent its application. Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> --- drivers/net/ethernet/netronome/nfp/bpf/main.c | 4 +--- drivers/net/netdevsim/bpf.c | 5 +---- include/net/pkt_cls.h | 20 +++++++++++++------- 3 files changed, 15 insertions(+), 14 deletions(-)