| Message ID | 20180120014450.29666-7-jakub.kicinski@netronome.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: sched: add extack support for cls offloads | expand |
Sat, Jan 20, 2018 at 02:44:48AM CET, jakub.kicinski@netronome.com wrote: >From: Quentin Monnet <quentin.monnet@netronome.com> > >Create a wrapper around tc_can_offload() that takes an additional >extack pointer argument in order to output an error message if TC >offload is disabled on the device. > >In this way, the error message is handled by the core and can be the >same for all drivers. > >Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com> >Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com> >--- > include/net/pkt_cls.h | 11 +++++++++++ > 1 file changed, 11 insertions(+) > >diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h >index f497f622580b..2f8f16a4d88e 100644 >--- a/include/net/pkt_cls.h >+++ b/include/net/pkt_cls.h >@@ -656,6 +656,17 @@ 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) I don't like to add tc_can_offload variant for this. It makes sense the original tc_can_offload to be extended and set the extack message always. It would require some more work in drivers (5), sure, but we endup with nicer and consistent code. >+{ >+ bool can = tc_can_offload(dev); >+ >+ if (!can) >+ NL_SET_ERR_MSG(extack, "TC offload is disabled on net device"); >+ >+ return can; >+} >+ > static inline bool tc_skip_hw(u32 flags) > { > return (flags & TCA_CLS_FLAGS_SKIP_HW) ? true : false; >-- >2.15.1 >
On Sat, Jan 20, 2018 at 12:59 AM, Jiri Pirko <jiri@resnulli.us> wrote: > Sat, Jan 20, 2018 at 02:44:48AM CET, jakub.kicinski@netronome.com wrote: >>From: Quentin Monnet <quentin.monnet@netronome.com> >> >>Create a wrapper around tc_can_offload() that takes an additional >>extack pointer argument in order to output an error message if TC >>offload is disabled on the device. >> >>In this way, the error message is handled by the core and can be the >>same for all drivers. >> >>Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com> >>Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com> >>--- >> include/net/pkt_cls.h | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >>diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h >>index f497f622580b..2f8f16a4d88e 100644 >>--- a/include/net/pkt_cls.h >>+++ b/include/net/pkt_cls.h >>@@ -656,6 +656,17 @@ 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) > > I don't like to add tc_can_offload variant for this. It makes sense > the original tc_can_offload to be extended and set the extack message > always. > > It would require some more work in drivers (5), sure, but we endup with > nicer and consistent code. $ git grep tc_can_offload drivers/net/ethernet/broadcom/bnxt/bnxt.c: if (!bnxt_tc_flower_enabled(bp) || !tc_can_offload(bp->dev)) drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c: if (!bnxt_tc_flower_enabled(vf_rep->bp) || !tc_can_offload(bp->dev)) drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c: if (!tc_can_offload(dev)) drivers/net/ethernet/intel/ixgbe/ixgbe_main.c: if (!tc_can_offload(adapter->netdev)) drivers/net/ethernet/mellanox/mlx5/core/en_main.c: if (!tc_can_offload(priv->netdev)) drivers/net/ethernet/mellanox/mlx5/core/en_rep.c: if (!tc_can_offload(priv->netdev)) drivers/net/ethernet/mellanox/mlxsw/spectrum.c: if (!tc_can_offload(mlxsw_sp_port->dev)) drivers/net/ethernet/netronome/nfp/bpf/main.c: !tc_can_offload(nn->dp.netdev) || drivers/net/ethernet/netronome/nfp/flower/offload.c: if (!tc_can_offload(repr->netdev)) drivers/net/ethernet/netronome/nfp/flower/offload.c: if (!tc_can_offload(repr->netdev)) drivers/net/netdevsim/bpf.c: !tc_can_offload(ns->netdev) || include/net/pkt_cls.h:static inline bool tc_can_offload(const struct net_device *dev) net/dsa/slave.c: if (!tc_can_offload(dev)) net/sched/cls_api.c: if (!tc_can_offload(dev) && tcf_block_offload_in_use(block)) net/sched/sch_mq.c: if (!tc_can_offload(dev) || !dev->netdev_ops->ndo_setup_tc) net/sched/sch_prio.c: if (!tc_can_offload(dev) || !dev->netdev_ops->ndo_setup_tc) net/sched/sch_prio.c: if (!tc_can_offload(dev) || !dev->netdev_ops->ndo_setup_tc) net/sched/sch_red.c: if (!tc_can_offload(dev) || !dev->netdev_ops->ndo_setup_tc) net/sched/sch_red.c: if (!tc_can_offload(dev) || !dev->netdev_ops->ndo_setup_tc) Do you mean the qdisc offloads too? The whole lot?
Sat, Jan 20, 2018 at 11:12:25AM CET, jakub.kicinski@netronome.com wrote: >On Sat, Jan 20, 2018 at 12:59 AM, Jiri Pirko <jiri@resnulli.us> wrote: >> Sat, Jan 20, 2018 at 02:44:48AM CET, jakub.kicinski@netronome.com wrote: >>>From: Quentin Monnet <quentin.monnet@netronome.com> >>> >>>Create a wrapper around tc_can_offload() that takes an additional >>>extack pointer argument in order to output an error message if TC >>>offload is disabled on the device. >>> >>>In this way, the error message is handled by the core and can be the >>>same for all drivers. >>> >>>Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com> >>>Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com> >>>--- >>> include/net/pkt_cls.h | 11 +++++++++++ >>> 1 file changed, 11 insertions(+) >>> >>>diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h >>>index f497f622580b..2f8f16a4d88e 100644 >>>--- a/include/net/pkt_cls.h >>>+++ b/include/net/pkt_cls.h >>>@@ -656,6 +656,17 @@ 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) >> >> I don't like to add tc_can_offload variant for this. It makes sense >> the original tc_can_offload to be extended and set the extack message >> always. >> >> It would require some more work in drivers (5), sure, but we endup with >> nicer and consistent code. > >$ git grep tc_can_offload >drivers/net/ethernet/broadcom/bnxt/bnxt.c: if >(!bnxt_tc_flower_enabled(bp) || !tc_can_offload(bp->dev)) >drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c: if >(!bnxt_tc_flower_enabled(vf_rep->bp) || !tc_can_offload(bp->dev)) >drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c: if >(!tc_can_offload(dev)) >drivers/net/ethernet/intel/ixgbe/ixgbe_main.c: if >(!tc_can_offload(adapter->netdev)) >drivers/net/ethernet/mellanox/mlx5/core/en_main.c: if >(!tc_can_offload(priv->netdev)) >drivers/net/ethernet/mellanox/mlx5/core/en_rep.c: if >(!tc_can_offload(priv->netdev)) >drivers/net/ethernet/mellanox/mlxsw/spectrum.c: if >(!tc_can_offload(mlxsw_sp_port->dev)) >drivers/net/ethernet/netronome/nfp/bpf/main.c: >!tc_can_offload(nn->dp.netdev) || >drivers/net/ethernet/netronome/nfp/flower/offload.c: if >(!tc_can_offload(repr->netdev)) >drivers/net/ethernet/netronome/nfp/flower/offload.c: if >(!tc_can_offload(repr->netdev)) >drivers/net/netdevsim/bpf.c: !tc_can_offload(ns->netdev) || >include/net/pkt_cls.h:static inline bool tc_can_offload(const struct >net_device *dev) >net/dsa/slave.c: if (!tc_can_offload(dev)) >net/sched/cls_api.c: if (!tc_can_offload(dev) && >tcf_block_offload_in_use(block)) >net/sched/sch_mq.c: if (!tc_can_offload(dev) || >!dev->netdev_ops->ndo_setup_tc) >net/sched/sch_prio.c: if (!tc_can_offload(dev) || >!dev->netdev_ops->ndo_setup_tc) >net/sched/sch_prio.c: if (!tc_can_offload(dev) || >!dev->netdev_ops->ndo_setup_tc) >net/sched/sch_red.c: if (!tc_can_offload(dev) || >!dev->netdev_ops->ndo_setup_tc) >net/sched/sch_red.c: if (!tc_can_offload(dev) || >!dev->netdev_ops->ndo_setup_tc) > >Do you mean the qdisc offloads too? The whole lot? Yes.
On Sat, Jan 20, 2018 at 2:22 AM, Jiri Pirko <jiri@resnulli.us> wrote: > Sat, Jan 20, 2018 at 11:12:25AM CET, jakub.kicinski@netronome.com wrote: >>net/sched/sch_prio.c: if (!tc_can_offload(dev) || >>!dev->netdev_ops->ndo_setup_tc) >>net/sched/sch_prio.c: if (!tc_can_offload(dev) || >>!dev->netdev_ops->ndo_setup_tc) >>net/sched/sch_red.c: if (!tc_can_offload(dev) || >>!dev->netdev_ops->ndo_setup_tc) >>net/sched/sch_red.c: if (!tc_can_offload(dev) || >>!dev->netdev_ops->ndo_setup_tc) >> >>Do you mean the qdisc offloads too? The whole lot? > > Yes. Actually looking at the qdisc code and destroy callbacks, if we plumb it through everywhere won't that mean user will see error messages on destroy of qdiscs/filters which were never offloaded? Just looking at prio_offload() as a simple example. prio_destroy() will always call tc_can_offload(). Hmm...
Sat, Jan 20, 2018 at 11:33:31AM CET, jakub.kicinski@netronome.com wrote: >On Sat, Jan 20, 2018 at 2:22 AM, Jiri Pirko <jiri@resnulli.us> wrote: >> Sat, Jan 20, 2018 at 11:12:25AM CET, jakub.kicinski@netronome.com wrote: >>>net/sched/sch_prio.c: if (!tc_can_offload(dev) || >>>!dev->netdev_ops->ndo_setup_tc) >>>net/sched/sch_prio.c: if (!tc_can_offload(dev) || >>>!dev->netdev_ops->ndo_setup_tc) >>>net/sched/sch_red.c: if (!tc_can_offload(dev) || >>>!dev->netdev_ops->ndo_setup_tc) >>>net/sched/sch_red.c: if (!tc_can_offload(dev) || >>>!dev->netdev_ops->ndo_setup_tc) >>> >>>Do you mean the qdisc offloads too? The whole lot? >> >> Yes. > >Actually looking at the qdisc code and destroy callbacks, if we plumb >it through everywhere won't that mean user will see error messages on >destroy of qdiscs/filters which were never offloaded? > >Just looking at prio_offload() as a simple example. prio_destroy() >will always call tc_can_offload(). Hmmm. You are right. Either we pass null from there (NL_SET_ERR_MSG can cope), or we leave tc_can_offload helper as is and let the caller to set the extack. I see that tc_can_offload_extack is probably good idea then. But please use it in all drivers that are calling tc_can_offload so the user experience is consistent for all. Thanks!
On Sat, Jan 20, 2018 at 2:54 AM, Jiri Pirko <jiri@resnulli.us> wrote: > Sat, Jan 20, 2018 at 11:33:31AM CET, jakub.kicinski@netronome.com wrote: >>On Sat, Jan 20, 2018 at 2:22 AM, Jiri Pirko <jiri@resnulli.us> wrote: >>> Sat, Jan 20, 2018 at 11:12:25AM CET, jakub.kicinski@netronome.com wrote: >>>>net/sched/sch_prio.c: if (!tc_can_offload(dev) || >>>>!dev->netdev_ops->ndo_setup_tc) >>>>net/sched/sch_prio.c: if (!tc_can_offload(dev) || >>>>!dev->netdev_ops->ndo_setup_tc) >>>>net/sched/sch_red.c: if (!tc_can_offload(dev) || >>>>!dev->netdev_ops->ndo_setup_tc) >>>>net/sched/sch_red.c: if (!tc_can_offload(dev) || >>>>!dev->netdev_ops->ndo_setup_tc) >>>> >>>>Do you mean the qdisc offloads too? The whole lot? >>> >>> Yes. >> >>Actually looking at the qdisc code and destroy callbacks, if we plumb >>it through everywhere won't that mean user will see error messages on >>destroy of qdiscs/filters which were never offloaded? >> >>Just looking at prio_offload() as a simple example. prio_destroy() >>will always call tc_can_offload(). > > Hmmm. You are right. > Either we pass null from there (NL_SET_ERR_MSG can cope), or we leave > tc_can_offload helper as is and let the caller to set the extack. I'm afraid it doesn't stop there though :/ Even now if one install a filter on a netdev with offload abilities and no skip_* flag there is this: # tc filter add dev eth0 ingress bpf obj drop.o da Warning: TC offload is disabled on net device. I'm not sure we should warn the user if there are no skip_* flags, just because the device has *some* offload capabilities? Perhaps we should put the flags into tc_cls_common_offload and add a helper to only set extack if skip_sw is set.. > I see that tc_can_offload_extack is probably good idea then. > > But please use it in all drivers that are calling tc_can_offload so the > user experience is consistent for all. Certainly, will do.
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h index f497f622580b..2f8f16a4d88e 100644 --- a/include/net/pkt_cls.h +++ b/include/net/pkt_cls.h @@ -656,6 +656,17 @@ 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) +{ + bool can = tc_can_offload(dev); + + if (!can) + NL_SET_ERR_MSG(extack, "TC offload is disabled on net device"); + + return can; +} + static inline bool tc_skip_hw(u32 flags) { return (flags & TCA_CLS_FLAGS_SKIP_HW) ? true : false;