| Message ID | 20190307093408.3425010-1-arnd@arndb.de |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | ethtool: reduce stack usage with clang | expand |
On Thu, Mar 07, 2019 at 10:33:35AM +0100, Arnd Bergmann wrote: > clang inlines the dev_ethtool() more aggressively than gcc does, leading > to a larger amount of used stack space: > > net/core/ethtool.c:2536:24: error: stack frame size of 1216 bytes in function 'dev_ethtool' [-Werror,-Wframe-larger-than=] > > Marking the sub-functions that require the most stack space as > noinline_for_stack gives us reasonable behavior on all compilers. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > net/core/ethtool.c | 18 ++++++++++-------- > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/net/core/ethtool.c b/net/core/ethtool.c > index d4918ffddda8..fcbed78172a0 100644 > --- a/net/core/ethtool.c > +++ b/net/core/ethtool.c ... > @@ -2533,7 +2535,7 @@ static int ethtool_set_fecparam(struct net_device *dev, void __user *useraddr) > > /* The main entry point in this file. Called from net/core/dev_ioctl.c */ > > -int dev_ethtool(struct net *net, struct ifreq *ifr) > +int noinline_for_stack dev_ethtool(struct net *net, struct ifreq *ifr) > { > struct net_device *dev = __dev_get_by_name(net, ifr->ifr_name); > void __user *useraddr = ifr->ifr_data; Is this part really needed? AFAICS dev_ethtool() is only called from dev_ioctl() which is in a different compilation unit so that dev_ethtool() won't be inlined anyway. Michal Kubecek
On Thu, Mar 7, 2019 at 11:06 AM Michal Kubecek <mkubecek@suse.cz> wrote: > > On Thu, Mar 07, 2019 at 10:33:35AM +0100, Arnd Bergmann wrote: > > @@ -2533,7 +2535,7 @@ static int ethtool_set_fecparam(struct net_device *dev, void __user *useraddr) > > > > /* The main entry point in this file. Called from net/core/dev_ioctl.c */ > > > > -int dev_ethtool(struct net *net, struct ifreq *ifr) > > +int noinline_for_stack dev_ethtool(struct net *net, struct ifreq *ifr) > > { > > struct net_device *dev = __dev_get_by_name(net, ifr->ifr_name); > > void __user *useraddr = ifr->ifr_data; > > Is this part really needed? AFAICS dev_ethtool() is only called from > dev_ioctl() which is in a different compilation unit so that > dev_ethtool() won't be inlined anyway. No, you are right. I had accidentally left this in place from an earlier version. Sending a v2 now. Arnd
diff --git a/net/core/ethtool.c b/net/core/ethtool.c index d4918ffddda8..fcbed78172a0 100644 --- a/net/core/ethtool.c +++ b/net/core/ethtool.c @@ -2319,9 +2319,10 @@ static int ethtool_set_tunable(struct net_device *dev, void __user *useraddr) return ret; } -static int ethtool_get_per_queue_coalesce(struct net_device *dev, - void __user *useraddr, - struct ethtool_per_queue_op *per_queue_opt) +static noinline_for_stack int +ethtool_get_per_queue_coalesce(struct net_device *dev, + void __user *useraddr, + struct ethtool_per_queue_op *per_queue_opt) { u32 bit; int ret; @@ -2349,9 +2350,10 @@ static int ethtool_get_per_queue_coalesce(struct net_device *dev, return 0; } -static int ethtool_set_per_queue_coalesce(struct net_device *dev, - void __user *useraddr, - struct ethtool_per_queue_op *per_queue_opt) +static noinline_for_stack int +ethtool_set_per_queue_coalesce(struct net_device *dev, + void __user *useraddr, + struct ethtool_per_queue_op *per_queue_opt) { u32 bit; int i, ret = 0; @@ -2405,7 +2407,7 @@ static int ethtool_set_per_queue_coalesce(struct net_device *dev, return ret; } -static int ethtool_set_per_queue(struct net_device *dev, +static int noinline_for_stack ethtool_set_per_queue(struct net_device *dev, void __user *useraddr, u32 sub_cmd) { struct ethtool_per_queue_op per_queue_opt; @@ -2533,7 +2535,7 @@ static int ethtool_set_fecparam(struct net_device *dev, void __user *useraddr) /* The main entry point in this file. Called from net/core/dev_ioctl.c */ -int dev_ethtool(struct net *net, struct ifreq *ifr) +int noinline_for_stack dev_ethtool(struct net *net, struct ifreq *ifr) { struct net_device *dev = __dev_get_by_name(net, ifr->ifr_name); void __user *useraddr = ifr->ifr_data;
clang inlines the dev_ethtool() more aggressively than gcc does, leading to a larger amount of used stack space: net/core/ethtool.c:2536:24: error: stack frame size of 1216 bytes in function 'dev_ethtool' [-Werror,-Wframe-larger-than=] Marking the sub-functions that require the most stack space as noinline_for_stack gives us reasonable behavior on all compilers. Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- net/core/ethtool.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)