* Re: [PATCH] select appropriate skb size in tcp_sendmsg when TSO is used [not found] ` <20041020163510.6d13e9c7.davem@davemloft.net> @ 2004-10-26 11:19 ` Herbert Xu 2004-10-26 23:51 ` Herbert Xu 0 siblings, 1 reply; 10+ messages in thread From: Herbert Xu @ 2004-10-26 11:19 UTC (permalink / raw) To: David S. Miller; +Cc: thomas.spatzier, netdev, Jeff Garzik On Wed, Oct 20, 2004 at 04:35:10PM -0700, David S. Miller wrote: > > BTW, we allow mucking of all of these SG, TSO, CSUM settings > via ethtool yet the "X needs Y" rules are not enforced. I > can't think of an easy way to do this without touching a lot > of drivers. Perhaps something like: Well since they're all being invoked through ethtool, you can just enforce the policy there. So let's take set_tx_csum as the example again, you'd do this in ethtool_set_tx_csum: if (!dev->ethtool_ops->set_tx_csum) return -EOPNOTSUPP; if (copy_from_user(&edata, useraddr, sizeof(edata))) return -EFAULT; if (!edata.data && (dev->features & NETIF_F_SG)) __ethtool_set_sg(dev, 0); return dev->ethtool_ops->set_tx_csum(dev, edata.data); Where __ethtool_set_sg would be ethtool_set_sg with the second argument turned into a simple int. It can then do a similar check to turn off TSO. > It just occured to me that instead of manually clearing > dev->feature bits we should invoke the appropriate ethtool > op to accomplish that just in case the device needs to do > something implementation specific when disabling said features. > This implies that ethtool_feature_change() should be invoked > without any device locks set to prevent deadlocks. Absolutely. That sounds exactly like what I've just described :) -- Visit Openswan at http://www.openswan.org/ Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] select appropriate skb size in tcp_sendmsg when TSO is used 2004-10-26 11:19 ` [PATCH] select appropriate skb size in tcp_sendmsg when TSO is used Herbert Xu @ 2004-10-26 23:51 ` Herbert Xu 2004-10-26 23:54 ` Jeff Garzik 0 siblings, 1 reply; 10+ messages in thread From: Herbert Xu @ 2004-10-26 23:51 UTC (permalink / raw) To: David S. Miller; +Cc: thomas.spatzier, netdev, Jeff Garzik [-- Attachment #1: Type: text/plain, Size: 812 bytes --] On Tue, Oct 26, 2004 at 09:19:12PM +1000, herbert wrote: > On Wed, Oct 20, 2004 at 04:35:10PM -0700, David S. Miller wrote: > > > > BTW, we allow mucking of all of these SG, TSO, CSUM settings > > via ethtool yet the "X needs Y" rules are not enforced. I > > can't think of an easy way to do this without touching a lot > > of drivers. Perhaps something like: > > Well since they're all being invoked through ethtool, you can > just enforce the policy there. So let's take set_tx_csum Alright, here is a patch to do exactly that. Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> Cheers, -- Visit Openswan at http://www.openswan.org/ Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt [-- Attachment #2: p --] [-- Type: text/plain, Size: 1554 bytes --] ===== net/core/ethtool.c 1.15 vs edited ===== --- 1.15/net/core/ethtool.c 2004-06-14 05:59:15 +10:00 +++ edited/net/core/ethtool.c 2004-10-27 09:39:09 +10:00 @@ -452,9 +452,23 @@ return 0; } +static int __ethtool_set_sg(struct net_device *dev, u32 data) +{ + int err; + + if (!data && dev->ethtool_ops->set_tso) { + err = dev->ethtool_ops->set_tso(dev, 0); + if (err) + return err; + } + + return dev->ethtool_ops->set_sg(dev, data); +} + static int ethtool_set_tx_csum(struct net_device *dev, char __user *useraddr) { struct ethtool_value edata; + int err; if (!dev->ethtool_ops->set_tx_csum) return -EOPNOTSUPP; @@ -462,6 +476,12 @@ if (copy_from_user(&edata, useraddr, sizeof(edata))) return -EFAULT; + if (!edata.data && dev->ethtool_ops->set_sg) { + err = __ethtool_set_sg(dev, 0); + if (err) + return err; + } + return dev->ethtool_ops->set_tx_csum(dev, edata.data); } @@ -489,7 +509,13 @@ if (copy_from_user(&edata, useraddr, sizeof(edata))) return -EFAULT; - return dev->ethtool_ops->set_sg(dev, edata.data); + if (edata.data && + !(dev->features & (NETIF_F_IP_CSUM | + NETIF_F_NO_CSUM | + NETIF_F_HW_CSUM))) + return -EINVAL; + + return __ethtool_set_sg(dev, edata.data); } static int ethtool_get_tso(struct net_device *dev, char __user *useraddr) @@ -515,6 +541,9 @@ if (copy_from_user(&edata, useraddr, sizeof(edata))) return -EFAULT; + + if (edata.data && !(dev->features & NETIF_F_SG)) + return -EINVAL; return dev->ethtool_ops->set_tso(dev, edata.data); } ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] select appropriate skb size in tcp_sendmsg when TSO is used 2004-10-26 23:51 ` Herbert Xu @ 2004-10-26 23:54 ` Jeff Garzik 2004-10-27 0:07 ` Herbert Xu 0 siblings, 1 reply; 10+ messages in thread From: Jeff Garzik @ 2004-10-26 23:54 UTC (permalink / raw) To: Herbert Xu; +Cc: David S. Miller, thomas.spatzier, netdev Herbert Xu wrote: > +static int __ethtool_set_sg(struct net_device *dev, u32 data) > +{ > + int err; > + > + if (!data && dev->ethtool_ops->set_tso) { > + err = dev->ethtool_ops->set_tso(dev, 0); > + if (err) > + return err; > + } > + > + return dev->ethtool_ops->set_sg(dev, data); > +} you want to disable tx-csum also ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] select appropriate skb size in tcp_sendmsg when TSO is used 2004-10-26 23:54 ` Jeff Garzik @ 2004-10-27 0:07 ` Herbert Xu 2004-10-27 0:03 ` David S. Miller 2004-10-27 0:15 ` Jeff Garzik 0 siblings, 2 replies; 10+ messages in thread From: Herbert Xu @ 2004-10-27 0:07 UTC (permalink / raw) To: Jeff Garzik; +Cc: David S. Miller, thomas.spatzier, netdev On Tue, Oct 26, 2004 at 07:54:35PM -0400, Jeff Garzik wrote: > Herbert Xu wrote: > >+static int __ethtool_set_sg(struct net_device *dev, u32 data) > >+{ > >+ int err; > >+ > >+ if (!data && dev->ethtool_ops->set_tso) { > >+ err = dev->ethtool_ops->set_tso(dev, 0); > >+ if (err) > >+ return err; > >+ } > >+ > >+ return dev->ethtool_ops->set_sg(dev, data); > >+} > > you want to disable tx-csum also IMHO it is valid to disable SG without disabling checksums, no? -- Visit Openswan at http://www.openswan.org/ Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] select appropriate skb size in tcp_sendmsg when TSO is used 2004-10-27 0:07 ` Herbert Xu @ 2004-10-27 0:03 ` David S. Miller 2004-10-27 1:41 ` Herbert Xu 2004-10-27 0:15 ` Jeff Garzik 1 sibling, 1 reply; 10+ messages in thread From: David S. Miller @ 2004-10-27 0:03 UTC (permalink / raw) To: Herbert Xu; +Cc: jgarzik, thomas.spatzier, netdev On Wed, 27 Oct 2004 10:07:24 +1000 Herbert Xu <herbert@gondor.apana.org.au> wrote: > On Tue, Oct 26, 2004 at 07:54:35PM -0400, Jeff Garzik wrote: > > Herbert Xu wrote: > > >+static int __ethtool_set_sg(struct net_device *dev, u32 data) > > >+{ > > >+ int err; > > >+ > > >+ if (!data && dev->ethtool_ops->set_tso) { > > >+ err = dev->ethtool_ops->set_tso(dev, 0); > > >+ if (err) > > >+ return err; > > >+ } > > >+ > > >+ return dev->ethtool_ops->set_sg(dev, data); > > >+} > > > > you want to disable tx-csum also > > IMHO it is valid to disable SG without disabling checksums, no? Yes and no. SG support requires TX csum support. See the tests we make at register_netdev() time. So as long as you enforce that rule, which I believe you are, things are fine. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] select appropriate skb size in tcp_sendmsg when TSO is used 2004-10-27 0:03 ` David S. Miller @ 2004-10-27 1:41 ` Herbert Xu 0 siblings, 0 replies; 10+ messages in thread From: Herbert Xu @ 2004-10-27 1:41 UTC (permalink / raw) To: David S. Miller; +Cc: jgarzik, thomas.spatzier, netdev On Tue, Oct 26, 2004 at 05:03:44PM -0700, David S. Miller wrote: > > Yes and no. SG support requires TX csum support. See the tests we > make at register_netdev() time. > > So as long as you enforce that rule, which I believe you are, > things are fine. That's right, the patch will turn off SG whenever TX csum is turned off. And if you try to turn SG on while TX csum is off, it'll return EINVAL. Cheers, -- Visit Openswan at http://www.openswan.org/ Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] select appropriate skb size in tcp_sendmsg when TSO is used 2004-10-27 0:07 ` Herbert Xu 2004-10-27 0:03 ` David S. Miller @ 2004-10-27 0:15 ` Jeff Garzik 2004-10-27 0:22 ` Herbert Xu 1 sibling, 1 reply; 10+ messages in thread From: Jeff Garzik @ 2004-10-27 0:15 UTC (permalink / raw) To: Herbert Xu; +Cc: David S. Miller, thomas.spatzier, netdev On Wed, Oct 27, 2004 at 10:07:24AM +1000, Herbert Xu wrote: > On Tue, Oct 26, 2004 at 07:54:35PM -0400, Jeff Garzik wrote: > > Herbert Xu wrote: > > >+static int __ethtool_set_sg(struct net_device *dev, u32 data) > > >+{ > > >+ int err; > > >+ > > >+ if (!data && dev->ethtool_ops->set_tso) { > > >+ err = dev->ethtool_ops->set_tso(dev, 0); > > >+ if (err) > > >+ return err; > > >+ } > > >+ > > >+ return dev->ethtool_ops->set_sg(dev, data); > > >+} > > > > you want to disable tx-csum also > > IMHO it is valid to disable SG without disabling checksums, no? It's useless: The packet header is always in a separate memory location from the packet data, when using zerocopy sendfile(2). When not using zerocopy sendfile, you are copying the data _anyway_. Jeff ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] select appropriate skb size in tcp_sendmsg when TSO is used 2004-10-27 0:15 ` Jeff Garzik @ 2004-10-27 0:22 ` Herbert Xu 2004-10-27 0:55 ` David S. Miller 0 siblings, 1 reply; 10+ messages in thread From: Herbert Xu @ 2004-10-27 0:22 UTC (permalink / raw) To: Jeff Garzik; +Cc: David S. Miller, thomas.spatzier, netdev On Tue, Oct 26, 2004 at 08:15:31PM -0400, Jeff Garzik wrote: > > > IMHO it is valid to disable SG without disabling checksums, no? > > It's useless: The packet header is always in a separate memory > location from the packet data, when using zerocopy sendfile(2). > > When not using zerocopy sendfile, you are copying the data _anyway_. I'm fine with adding this check. However I think that belongs in another patch since we don't check that in register_netdev currently. Dave, what do you think? -- Visit Openswan at http://www.openswan.org/ Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] select appropriate skb size in tcp_sendmsg when TSO is used 2004-10-27 0:22 ` Herbert Xu @ 2004-10-27 0:55 ` David S. Miller 2004-10-28 4:16 ` Jeff Garzik 0 siblings, 1 reply; 10+ messages in thread From: David S. Miller @ 2004-10-27 0:55 UTC (permalink / raw) To: Herbert Xu; +Cc: jgarzik, thomas.spatzier, netdev On Wed, 27 Oct 2004 10:22:09 +1000 Herbert Xu <herbert@gondor.apana.org.au> wrote: > On Tue, Oct 26, 2004 at 08:15:31PM -0400, Jeff Garzik wrote: > > > > > IMHO it is valid to disable SG without disabling checksums, no? > > > > It's useless: The packet header is always in a separate memory > > location from the packet data, when using zerocopy sendfile(2). > > > > When not using zerocopy sendfile, you are copying the data _anyway_. > > I'm fine with adding this check. However I think that belongs in > another patch since we don't check that in register_netdev currently. > > Dave, what do you think? I believe that allowing TX csum support without SG _is_ useful even though it is not _effective_. It is quite desirable for a driver author to be able to test out his TX csum offload support first, then add SG support next. Similarly, if a driver author suspects some issues with either SG or TX csum support, he can better isolate the problem if we allow this. Jeff do you agree? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] select appropriate skb size in tcp_sendmsg when TSO is used 2004-10-27 0:55 ` David S. Miller @ 2004-10-28 4:16 ` Jeff Garzik 0 siblings, 0 replies; 10+ messages in thread From: Jeff Garzik @ 2004-10-28 4:16 UTC (permalink / raw) To: David S. Miller; +Cc: Herbert Xu, thomas.spatzier, netdev David S. Miller wrote: > On Wed, 27 Oct 2004 10:22:09 +1000 > Herbert Xu <herbert@gondor.apana.org.au> wrote: > > >>On Tue, Oct 26, 2004 at 08:15:31PM -0400, Jeff Garzik wrote: >> >>>>IMHO it is valid to disable SG without disabling checksums, no? >>> >>>It's useless: The packet header is always in a separate memory >>>location from the packet data, when using zerocopy sendfile(2). >>> >>>When not using zerocopy sendfile, you are copying the data _anyway_. >> >>I'm fine with adding this check. However I think that belongs in >>another patch since we don't check that in register_netdev currently. >> >>Dave, what do you think? > > > I believe that allowing TX csum support without SG _is_ > useful even though it is not _effective_. > > It is quite desirable for a driver author to be able to > test out his TX csum offload support first, then add > SG support next. Similarly, if a driver author suspects > some issues with either SG or TX csum support, he can > better isolate the problem if we allow this. > > Jeff do you agree? <shrug> it's never used that way in practice AFAIK, only used by confused sysadmins :) I won't object if you preserve the behavior, but I still don't see much value in allowing it. Jeff ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2004-10-28 4:16 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <OF96546AB5.ACE12043-ONC1256F33.0027BC6B-C1256F33.002D4A6E@de.ibm.com>
[not found] ` <E1CKE5P-0005SP-00@gondolin.me.apana.org.au>
[not found] ` <20041020163510.6d13e9c7.davem@davemloft.net>
2004-10-26 11:19 ` [PATCH] select appropriate skb size in tcp_sendmsg when TSO is used Herbert Xu
2004-10-26 23:51 ` Herbert Xu
2004-10-26 23:54 ` Jeff Garzik
2004-10-27 0:07 ` Herbert Xu
2004-10-27 0:03 ` David S. Miller
2004-10-27 1:41 ` Herbert Xu
2004-10-27 0:15 ` Jeff Garzik
2004-10-27 0:22 ` Herbert Xu
2004-10-27 0:55 ` David S. Miller
2004-10-28 4:16 ` Jeff Garzik
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).