Re: [PATCH] select appropriate skb size in tcp_sendmsg when TSO is used

Re: [PATCH] select appropriate skb size in tcp_sendmsg when TSO is used

[Herbert Xu]

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

Re: [PATCH] select appropriate skb size in tcp_sendmsg when TSO is used

[Herbert Xu]

[-- 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);
 }

Re: [PATCH] select appropriate skb size in tcp_sendmsg when TSO is used

[Jeff Garzik]

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

Re: [PATCH] select appropriate skb size in tcp_sendmsg when TSO is used

[Herbert Xu]

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

Re: [PATCH] select appropriate skb size in tcp_sendmsg when TSO is used

[David S. Miller]

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.

Re: [PATCH] select appropriate skb size in tcp_sendmsg when TSO is used

[Herbert Xu]

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

Re: [PATCH] select appropriate skb size in tcp_sendmsg when TSO is used

[Jeff Garzik]

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

Re: [PATCH] select appropriate skb size in tcp_sendmsg when TSO is used

[Herbert Xu]

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

Re: [PATCH] select appropriate skb size in tcp_sendmsg when TSO is used

[David S. Miller]

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?

Re: [PATCH] select appropriate skb size in tcp_sendmsg when TSO is used

[Jeff Garzik]

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