Re: [Xen-devel] [0/5] [NET]: Add TSO support

[Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>]

On Wed, Jun 28, 2006 at 01:30:10PM +0100, Keir Fraser wrote:
> 
> 1. The GSO patch broke netback, because netback/interface.c accesses 
> dev->xmit_lock. None of your patches fixed this and you can't build the 
> driver unless it's fixed -- did you somehow miss that file from one of 
> the patches you sent?

Sorry, I missed that hunk while generating the patches.

> 2. The new 'wire format' with netif_tx_extra: I placed the GSO fields 
> inside a struct inside a union, so we can extend the union with other 
> extra-info types in future. I hope that's okay and in line with what 
> you intended.

Actually, the idea is to add new fields after the existing fields so
I don't think a union is a good fit here.  The reason is that the new
fields will likely be used in conjunction with GSO.  In particular, I'm
thinking of the checksum offset/header offset.

> 3. Wire format again: we need some extra documentation and info in 
> netif.h for the new GSO fields. Currently they conveniently directly 
> correspond to fields in a Linux skbuff: you read them out in netfront 
> and write them straight back in netback. That's fine for Linux for now, 
> but not so good for other OSes, nor potentially if the Linux GSO 
> internals change later.

Well I think things like TSO (and even more so with GSO) are highly
OS-specific so porting them to other paravirt OSes are always going to
be hard.

The way I see it these are simply add-on features that you can enable
to get that extra bit out of your Xen performance.  So it is not required
for your system to function.  Therefore other OSes that do not have
TSO/GSO can simply elect to not use it.

(I'm curious, which other paravirt OSes do you have in mind that would
use something like TSO/GSO? Do they currently support TSO or something
similar?)

That's how it was designed in general: if the frontend doesn't know about
GSO it simply never sends any GSO packets.  If the backend doesn't know
about GSO it'd never advertise it to the frontend so again no GSO packets
would be sent.

As to how likely the implementation details are to change in Linux I'd
say that it probably won't happen anytime soon but I can't offer any
guarantees because it is an internel interface.

However, the design of the Xen wire format is such that it should be easy
to adapt to any incompatible changes by either

* Provide translation layers in the netfront/netback if the interface
change does not require new information to be exchanged.

* Adding new feature bits/request flags to indicate that new information
is required.  Older guests/hosts would simply not do TSO with incompatible
new hosts/guests.

> In particular the gso_type field is concerning. We should provide 
> defines for the legitimate values of that field in netif.h, with a 
> comment explaining what each one means. Extra comments are also 
> required for the other two fields, to convince us that they aren't 
> Linux-specific in some way. Some brief info about how GSO works in 
> general, including usage of those fields, would help.

No problems.  I've attached a patch that adds more comments for these
fields.  However, I'm really hesitant to add the actual gso_type values
here because I don't think this helps compatibility in any way.

If Linux ever does make an incompatible change (which believe me I will
do my best to prevent), having those values defined here are not really
going to help people notice the change or provide compatibility.

But if you really want these values, I can add them.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@xxxxxxxxxxxxxxxxxxx>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
diff -r 9f4e79081e4a xen/include/public/io/netif.h
--- a/xen/include/public/io/netif.h     Wed Jun 28 13:55:32 2006 +1000
+++ b/xen/include/public/io/netif.h     Wed Jun 28 23:54:47 2006 +1000
@@ -49,9 +49,23 @@ typedef struct netif_tx_request netif_tx
 
 /* This structure needs to fit within netif_tx_request for compatibility. */
 struct netif_tx_extra {
-    uint16_t gso_size;    /* GSO MSS. */
-    uint16_t gso_segs;    /* GSO segment count. */
-    uint16_t gso_type;    /* GSO type. */
+    /*
+     * Maximum payload size of each segment.  For example, for TCP this is
+     * just the path MSS.
+     */
+    uint16_t gso_size;
+
+    /*
+     * Number of GSO segments.  This is the number of segments that have to
+     * be generated for this packet given the MSS.
+     */
+    uint16_t gso_segs;
+
+    /*
+     * GSO type.  This determines the protocol of the packet and any extra
+     * features required to segment the packet properly.
+     */
+    uint16_t gso_type;
 };
 
 struct netif_tx_response {

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel