| Message ID | 852a74b9068b3be7413a65023f6096f142dfd805.1308596963.git.mirq-linux@rere.qmqm.pl |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, 2011-06-20 at 21:14 +0200, Michał Mirosław wrote: [...] > @@ -125,19 +131,26 @@ static int ethtool_set_features(struct net_device *dev, void __user *useraddr) > if (copy_from_user(features, useraddr, sizeof(features))) > return -EFAULT; > > - if (features[0].valid & ~NETIF_F_ETHTOOL_BITS) > + /* I wonder if the compiler will be smart enough to loop-unroll > + * and optimize this... (no worries if not) --mq */ > + for (i = ETHTOOL_DEV_FEATURE_WORDS; i-- > 0; ) { > + valid = (valid << 32)|features[i].valid; > + wanted = (wanted << 32)|features[i].requested; > + } [...] I don't know (or care) about optimisation of this, but I would expect gcc to complain about shifting a 32-bit value by 32 bits. I suggest you write this as: for (i = 0; i < ETHTOOL_DEV_FEATURE_WORDS; ++i) { valid |= (netdev_features_t)features[i].valid << 32 *i; wanted |= (netdev_features_t)features[i].requested << 32 *i; } Ben.
On Mon, Jun 20, 2011 at 2:16 PM, Ben Hutchings <bhutchings@solarflare.com> wrote: > > On Mon, 2011-06-20 at 21:14 +0200, Michał Mirosław wrote: > [...] > > @@ -125,19 +131,26 @@ static int ethtool_set_features(struct net_device *dev, void __user *useraddr) > > if (copy_from_user(features, useraddr, sizeof(features))) > > return -EFAULT; > > > > - if (features[0].valid & ~NETIF_F_ETHTOOL_BITS) > > + /* I wonder if the compiler will be smart enough to loop-unroll > > + * and optimize this... (no worries if not) --mq */ > > + for (i = ETHTOOL_DEV_FEATURE_WORDS; i-- > 0; ) { > > + valid = (valid << 32)|features[i].valid; > > + wanted = (wanted << 32)|features[i].requested; > > + } > [...] > > I don't know (or care) about optimisation of this, but I would expect > gcc to complain about shifting a 32-bit value by 32 bits. I suggest you > write this as: > > for (i = 0; i < ETHTOOL_DEV_FEATURE_WORDS; ++i) { > valid |= (netdev_features_t)features[i].valid << 32 *i; > wanted |= (netdev_features_t)features[i].requested << 32 *i; It's a valid point but this type of typecast or similar usage would imply that netdev_feature_t is an int of XXX bits. That's not opaque and would hinder the way you can abstract the feature type. --mahesh.. > > } > > Ben. > > -- > Ben Hutchings, Senior Software Engineer, Solarflare > Not speaking for my employer; that's the marketing department's job. > They asked us to note that Solarflare product names are trademarked. > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2011-06-23 at 10:50 -0700, Mahesh Bandewar wrote: > On Mon, Jun 20, 2011 at 2:16 PM, Ben Hutchings > <bhutchings@solarflare.com> wrote: > > > > On Mon, 2011-06-20 at 21:14 +0200, Michał Mirosław wrote: > > [...] > > > @@ -125,19 +131,26 @@ static int ethtool_set_features(struct net_device *dev, void __user *useraddr) > > > if (copy_from_user(features, useraddr, sizeof(features))) > > > return -EFAULT; > > > > > > - if (features[0].valid & ~NETIF_F_ETHTOOL_BITS) > > > + /* I wonder if the compiler will be smart enough to loop-unroll > > > + * and optimize this... (no worries if not) --mq */ > > > + for (i = ETHTOOL_DEV_FEATURE_WORDS; i-- > 0; ) { > > > + valid = (valid << 32)|features[i].valid; > > > + wanted = (wanted << 32)|features[i].requested; > > > + } > > [...] > > > > I don't know (or care) about optimisation of this, but I would expect > > gcc to complain about shifting a 32-bit value by 32 bits. I suggest you > > write this as: > > > > for (i = 0; i < ETHTOOL_DEV_FEATURE_WORDS; ++i) { > > valid |= (netdev_features_t)features[i].valid << 32 *i; > > wanted |= (netdev_features_t)features[i].requested << 32 *i; > > It's a valid point but this type of typecast or similar usage would > imply that netdev_feature_t is an int of XXX bits. That's not opaque > and would hinder the way you can abstract the feature type. Yes, ethtool_{get,set}_features() will have to be changed if and when the representation of netdev_features_t is changed significantly. I don't think there's any way of avoiding that and I don't think it really matters. Ben.
On Thu, Jun 23, 2011 at 11:03 AM, Ben Hutchings <bhutchings@solarflare.com> wrote: > On Thu, 2011-06-23 at 10:50 -0700, Mahesh Bandewar wrote: >> On Mon, Jun 20, 2011 at 2:16 PM, Ben Hutchings >> <bhutchings@solarflare.com> wrote: >> > >> > On Mon, 2011-06-20 at 21:14 +0200, Michał Mirosław wrote: >> > [...] >> > > @@ -125,19 +131,26 @@ static int ethtool_set_features(struct net_device *dev, void __user *useraddr) >> > > if (copy_from_user(features, useraddr, sizeof(features))) >> > > return -EFAULT; >> > > >> > > - if (features[0].valid & ~NETIF_F_ETHTOOL_BITS) >> > > + /* I wonder if the compiler will be smart enough to loop-unroll >> > > + * and optimize this... (no worries if not) --mq */ >> > > + for (i = ETHTOOL_DEV_FEATURE_WORDS; i-- > 0; ) { >> > > + valid = (valid << 32)|features[i].valid; >> > > + wanted = (wanted << 32)|features[i].requested; >> > > + } >> > [...] >> > >> > I don't know (or care) about optimisation of this, but I would expect >> > gcc to complain about shifting a 32-bit value by 32 bits. I suggest you >> > write this as: >> > >> > for (i = 0; i < ETHTOOL_DEV_FEATURE_WORDS; ++i) { >> > valid |= (netdev_features_t)features[i].valid << 32 *i; >> > wanted |= (netdev_features_t)features[i].requested << 32 *i; >> >> It's a valid point but this type of typecast or similar usage would >> imply that netdev_feature_t is an int of XXX bits. That's not opaque >> and would hinder the way you can abstract the feature type. > > Yes, ethtool_{get,set}_features() will have to be changed if and when > the representation of netdev_features_t is changed significantly. I > don't think there's any way of avoiding that and I don't think it really > matters. > Well, if you have a conversion routine that converts (whatever the) netdev_type_t type is to the ethtool representation (array of u32 for example). So the changes would have to be done in that conversion routine only and not get/set_features() ethtool methods as such. --mahesh.. > Ben. > > -- > Ben Hutchings, Senior Software Engineer, Solarflare > Not speaking for my employer; that's the marketing department's job. > They asked us to note that Solarflare product names are trademarked. > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2011-06-23 at 11:21 -0700, Mahesh Bandewar wrote: > On Thu, Jun 23, 2011 at 11:03 AM, Ben Hutchings > <bhutchings@solarflare.com> wrote: > > On Thu, 2011-06-23 at 10:50 -0700, Mahesh Bandewar wrote: > >> On Mon, Jun 20, 2011 at 2:16 PM, Ben Hutchings > >> <bhutchings@solarflare.com> wrote: > >> > > >> > On Mon, 2011-06-20 at 21:14 +0200, Michał Mirosław wrote: > >> > [...] > >> > > @@ -125,19 +131,26 @@ static int ethtool_set_features(struct net_device *dev, void __user *useraddr) > >> > > if (copy_from_user(features, useraddr, sizeof(features))) > >> > > return -EFAULT; > >> > > > >> > > - if (features[0].valid & ~NETIF_F_ETHTOOL_BITS) > >> > > + /* I wonder if the compiler will be smart enough to loop-unroll > >> > > + * and optimize this... (no worries if not) --mq */ > >> > > + for (i = ETHTOOL_DEV_FEATURE_WORDS; i-- > 0; ) { > >> > > + valid = (valid << 32)|features[i].valid; > >> > > + wanted = (wanted << 32)|features[i].requested; > >> > > + } > >> > [...] > >> > > >> > I don't know (or care) about optimisation of this, but I would expect > >> > gcc to complain about shifting a 32-bit value by 32 bits. I suggest you > >> > write this as: > >> > > >> > for (i = 0; i < ETHTOOL_DEV_FEATURE_WORDS; ++i) { > >> > valid |= (netdev_features_t)features[i].valid << 32 *i; > >> > wanted |= (netdev_features_t)features[i].requested << 32 *i; > >> > >> It's a valid point but this type of typecast or similar usage would > >> imply that netdev_feature_t is an int of XXX bits. That's not opaque > >> and would hinder the way you can abstract the feature type. > > > > Yes, ethtool_{get,set}_features() will have to be changed if and when > > the representation of netdev_features_t is changed significantly. I > > don't think there's any way of avoiding that and I don't think it really > > matters. > > > Well, if you have a conversion routine that converts (whatever the) > netdev_type_t type is to the ethtool representation (array of u32 for > example). So the changes would have to be done in that conversion > routine only and not get/set_features() ethtool methods as such. These are precisely those conversion routines, because there is no other place that needs to deal with the ethtool representation... Ben.
On Thu, Jun 23, 2011 at 11:55 AM, Ben Hutchings <bhutchings@solarflare.com> wrote: > On Thu, 2011-06-23 at 11:21 -0700, Mahesh Bandewar wrote: >> On Thu, Jun 23, 2011 at 11:03 AM, Ben Hutchings >> <bhutchings@solarflare.com> wrote: >> > On Thu, 2011-06-23 at 10:50 -0700, Mahesh Bandewar wrote: >> >> On Mon, Jun 20, 2011 at 2:16 PM, Ben Hutchings >> >> <bhutchings@solarflare.com> wrote: >> >> > >> >> > On Mon, 2011-06-20 at 21:14 +0200, Michał Mirosław wrote: >> >> > [...] >> >> > > @@ -125,19 +131,26 @@ static int ethtool_set_features(struct net_device *dev, void __user *useraddr) >> >> > > if (copy_from_user(features, useraddr, sizeof(features))) >> >> > > return -EFAULT; >> >> > > >> >> > > - if (features[0].valid & ~NETIF_F_ETHTOOL_BITS) >> >> > > + /* I wonder if the compiler will be smart enough to loop-unroll >> >> > > + * and optimize this... (no worries if not) --mq */ >> >> > > + for (i = ETHTOOL_DEV_FEATURE_WORDS; i-- > 0; ) { >> >> > > + valid = (valid << 32)|features[i].valid; >> >> > > + wanted = (wanted << 32)|features[i].requested; >> >> > > + } >> >> > [...] >> >> > >> >> > I don't know (or care) about optimisation of this, but I would expect >> >> > gcc to complain about shifting a 32-bit value by 32 bits. I suggest you >> >> > write this as: >> >> > >> >> > for (i = 0; i < ETHTOOL_DEV_FEATURE_WORDS; ++i) { >> >> > valid |= (netdev_features_t)features[i].valid << 32 *i; >> >> > wanted |= (netdev_features_t)features[i].requested << 32 *i; >> >> >> >> It's a valid point but this type of typecast or similar usage would >> >> imply that netdev_feature_t is an int of XXX bits. That's not opaque >> >> and would hinder the way you can abstract the feature type. >> > >> > Yes, ethtool_{get,set}_features() will have to be changed if and when >> > the representation of netdev_features_t is changed significantly. I >> > don't think there's any way of avoiding that and I don't think it really >> > matters. >> > >> Well, if you have a conversion routine that converts (whatever the) >> netdev_type_t type is to the ethtool representation (array of u32 for >> example). So the changes would have to be done in that conversion >> routine only and not get/set_features() ethtool methods as such. > > These are precisely those conversion routines, because there is no other > place that needs to deal with the ethtool representation... > Oh, I was considering get/set_features ethtool methods as APIs to manipulate certain bits from user-space into kernel space. I was disassociating features bit representation as a separate aspect and (of course) the conversion. So depending on the type selection you may or may not need these conversion routines. --mahesh.. > Ben. > > -- > Ben Hutchings, Senior Software Engineer, Solarflare > Not speaking for my employer; that's the marketing department's job. > They asked us to note that Solarflare product names are trademarked. > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/core/ethtool.c b/net/core/ethtool.c index 97bbf25..3a93932 100644 --- a/net/core/ethtool.c +++ b/net/core/ethtool.c @@ -38,7 +38,7 @@ EXPORT_SYMBOL(ethtool_op_get_link); /* Handlers for each ethtool command */ -#define ETHTOOL_DEV_FEATURE_WORDS 1 +#define ETHTOOL_DEV_FEATURE_WORDS ((NETDEV_FEATURE_COUNT + 31) / 32) static const char netdev_features_strings[NETDEV_FEATURE_COUNT][ETH_GSTRING_LEN] = { [NETIF_F_SG_BIT] = "tx-scatter-gather", @@ -82,16 +82,21 @@ static int ethtool_get_features(struct net_device *dev, void __user *useraddr) .cmd = ETHTOOL_GFEATURES, .size = ETHTOOL_DEV_FEATURE_WORDS, }; - struct ethtool_get_features_block features[ETHTOOL_DEV_FEATURE_WORDS] = { - { - .available = dev->hw_features, - .requested = dev->wanted_features, - .active = dev->features, - .never_changed = NETIF_F_NEVER_CHANGE, - }, - }; + struct ethtool_get_features_block features[ETHTOOL_DEV_FEATURE_WORDS]; u32 __user *sizeaddr; u32 copy_size; + int i; + + /* in case feature bits run out again */ + BUILD_BUG_ON(ETHTOOL_DEV_FEATURE_WORDS*sizeof(u32) > sizeof(netdev_features_t)); + + /* this should get loop-unrolled easily by compiler */ + for (i = 0; i < ETHTOOL_DEV_FEATURE_WORDS; ++i) { + features[i].available = (u32)(dev->hw_features >> (32*i)); + features[i].requested = (u32)(dev->wanted_features >> (32*i)); + features[i].active = (u32)(dev->features >> (32*i)); + features[i].never_changed = (u32)(NETIF_F_NEVER_CHANGE >> (32*i)); + } sizeaddr = useraddr + offsetof(struct ethtool_gfeatures, size); if (get_user(copy_size, sizeaddr)) @@ -113,7 +118,8 @@ static int ethtool_set_features(struct net_device *dev, void __user *useraddr) { struct ethtool_sfeatures cmd; struct ethtool_set_features_block features[ETHTOOL_DEV_FEATURE_WORDS]; - int ret = 0; + netdev_features_t wanted = 0, valid = 0; + int i, ret = 0; if (copy_from_user(&cmd, useraddr, sizeof(cmd))) return -EFAULT; @@ -125,19 +131,26 @@ static int ethtool_set_features(struct net_device *dev, void __user *useraddr) if (copy_from_user(features, useraddr, sizeof(features))) return -EFAULT; - if (features[0].valid & ~NETIF_F_ETHTOOL_BITS) + /* I wonder if the compiler will be smart enough to loop-unroll + * and optimize this... (no worries if not) --mq */ + for (i = ETHTOOL_DEV_FEATURE_WORDS; i-- > 0; ) { + valid = (valid << 32)|features[i].valid; + wanted = (wanted << 32)|features[i].requested; + } + + if (valid & ~NETIF_F_ETHTOOL_BITS) return -EINVAL; - if (features[0].valid & ~dev->hw_features) { - features[0].valid &= dev->hw_features; + if (valid & ~dev->hw_features) { + valid &= dev->hw_features; ret |= ETHTOOL_F_UNSUPPORTED; } - dev->wanted_features &= ~features[0].valid; - dev->wanted_features |= features[0].valid & features[0].requested; + dev->wanted_features &= ~valid; + dev->wanted_features |= wanted & valid; __netdev_update_features(dev); - if ((dev->wanted_features ^ dev->features) & features[0].valid) + if ((dev->wanted_features ^ dev->features) & valid) ret |= ETHTOOL_F_WISH; return ret;
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> --- net/core/ethtool.c | 45 +++++++++++++++++++++++++++++---------------- 1 files changed, 29 insertions(+), 16 deletions(-)