GPLPV: Respecting SG capability

GPLPV: Respecting SG capability

[Russ Blaine]

Hi James,

As you may have heard, the latest GPLPV release doesn't work on 
Opensolaris dom0. Our backend net driver doesn't support scatter/gather, 
but it seems that GPLPV now requires it.

I have a fix for this in the frontend which coalesces all NDIS buffers 
into one ring transaction. With the fix, packets flow again.

Once this is addressed I may go and implement SG in our backend anyway, 
but I wanted to get this fix into the GPLPV source first to enable 
networking on older and current dom0s.

The fix as I have it now is around line 229 of xennet_tx.c (see below). 
I think a further and necessary improvement on this would be to avoid 
the construction of the header_buf() altogether in the no-sg case. There 
is also only one tx_sendbuf per driver instance (it just points to 
tx_hb[0]), and I suppose there should be several and that they should be 
managed in the same way you deal with the tx_hb[] instances.

Actually, on second look, there is a per-driver-instance tx_lock which 
must act to serialize all transmits? In which case we only need a single 
tx_sendbuf anyhow. Would Windows benefit from having a reentrant send 
routine?

Here is the prototype of the fix.

  if (xi->config_sg == 0) {
       int i;
       ULONG len;
       ULONG offset = 0;
       PNDIS_BUFFER buf;

       buf = pi.first_buffer;
       while (buf) {
           PUCHAR src_addr;

           NdisQueryBufferSafe(buf, &src_addr, &len, NormalPagePriority);

           memcpy((PUCHAR)xi->tx_sendbuf.virtual + offset, src_addr, len);
           offset += len;

           NdisGetNextBuffer(buf, &buf);
       }

       tx0->gref = (grant_ref_t)xi->tx_sendbuf.logical.QuadPart >> 
PAGE_SHIFT;
       tx0->offset = (USHORT)xi->tx_sendbuf.logical.LowPart & (PAGE_SIZE 
- 1);
       ASSERT(offset == pi.total_length);
       tx0->size = offset;
       tx0->flags &= ~NETTXF_more_data;
       sg_element = sg->NumberOfElements;
   } else if (header_buf)

Cheers,
- Russ

-----------------------------------------------------
Russ Blaine | Solaris Kernel | russell.blaine@sun.com

RE: GPLPV: Respecting SG capability

[James Harper]

> 
> Hi James,
> 
> As you may have heard, the latest GPLPV release doesn't work on
> Opensolaris dom0. Our backend net driver doesn't support
scatter/gather,
> but it seems that GPLPV now requires it.

Correct. Previously I was doing the coalescing already to work around
the problem that Linux has a limit of 18 SG elements.

> 
> I have a fix for this in the frontend which coalesces all NDIS buffers
> into one ring transaction. With the fix, packets flow again.
> 
> Once this is addressed I may go and implement SG in our backend
anyway,
> but I wanted to get this fix into the GPLPV source first to enable
> networking on older and current dom0s.
> 
> The fix as I have it now is around line 229 of xennet_tx.c (see
below).
> I think a further and necessary improvement on this would be to avoid
> the construction of the header_buf() altogether in the no-sg case.
There
> is also only one tx_sendbuf per driver instance (it just points to
> tx_hb[0]), and I suppose there should be several and that they should
be
> managed in the same way you deal with the tx_hb[] instances.
> 
> Actually, on second look, there is a per-driver-instance tx_lock which
> must act to serialize all transmits? In which case we only need a
single
> tx_sendbuf anyhow. Would Windows benefit from having a reentrant send
> routine?

The send path is definitely serialised.

We may be able to simply tell Windows that we don't support SG and it
may coalesce the buffers itself. I will do some testing of that before I
consider other workarounds.

Thanks

James

Re: RE: GPLPV: Respecting SG capability

[Russ Blaine]

[-- Attachment #1: Type: text/plain, Size: 975 bytes --]

James Harper wrote:
> We may be able to simply tell Windows that we don't support SG and it
> may coalesce the buffers itself. I will do some testing of that before I
> consider other workarounds.

As I understand it, this isn't possible to do - Windows insists on handing us 
several buffers per packet.

Attached is a patch against the current source - it would be helpful to have 
this in the upstream source so that these drivers work out of the box on Solaris 
dom0 (albeit with scatter/gather disabled in the frontend). A future improvement 
on this work will be to avoid constructing header_buf in this case, but this 
gets things working well enough. Another piece of future work will be to have 
the net driver disable sg if the backend doesn't have "feature-sg" set to 1 in 
xenstore.

I can do a push if so desired, just let me know.

Thanks,
- Russ

-- 

-----------------------------------------------------
Russ Blaine | Solaris Kernel | russell.blaine@sun.com

[-- Attachment #2: xennet-nosg.diff --]
[-- Type: text/plain, Size: 1811 bytes --]

diff -r d40c760a4f6b xennet/xennet.h
--- a/xennet/xennet.h	Tue Jun 09 13:42:03 2009 +1000
+++ b/xennet/xennet.h	Thu Jun 18 15:26:27 2009 -0700
@@ -279,6 +279,7 @@
   ULONG tx_hb_free;
   ULONG tx_hb_list[TX_HEADER_BUFFERS];
   shared_buffer_t tx_hbs[TX_HEADER_BUFFERS];
+  shared_buffer_t tx_sendbuf;
   KDPC tx_dpc;
 
   /* rx_related - protected by rx_lock */
diff -r d40c760a4f6b xennet/xennet_tx.c
--- a/xennet/xennet_tx.c	Tue Jun 09 13:42:03 2009 +1000
+++ b/xennet/xennet_tx.c	Thu Jun 18 15:26:27 2009 -0700
@@ -231,7 +231,30 @@
   chunks++;
   xi->tx_ring_free--;
   tx0->id = 0xFFFF;
-  if (header_buf)
+  if (xi->config_sg == 0) {
+      ULONG len;
+      ULONG offset = 0;
+      PNDIS_BUFFER buf;
+
+      buf = pi.first_buffer;
+      while (buf) {
+          PUCHAR src_addr;
+
+          NdisQueryBufferSafe(buf, &src_addr, &len, NormalPagePriority);
+
+          memcpy((PUCHAR)xi->tx_sendbuf.virtual + offset, src_addr, len);
+          offset += len;
+
+          NdisGetNextBuffer(buf, &buf);
+      }
+
+      tx0->gref = (grant_ref_t)xi->tx_sendbuf.logical.QuadPart >> PAGE_SHIFT;
+      tx0->offset = (USHORT)xi->tx_sendbuf.logical.LowPart & (PAGE_SIZE - 1);
+      ASSERT(offset == pi.total_length);
+      tx0->size = (uint16_t)offset;
+      tx0->flags &= ~NETTXF_more_data;
+      sg_element = sg->NumberOfElements; 
+  } else if (header_buf)
   {
     ULONG remaining = pi.header_length;
     ASSERT(pi.header_length < TX_HEADER_BUFFER_SIZE);
@@ -587,6 +610,9 @@
   if (i == 0)
     KdPrint((__DRIVER_NAME "     Unable to allocate any SharedMemory buffers\n"));
 
+  xi->tx_sendbuf.virtual = xi->tx_hbs[0].virtual;
+  xi->tx_sendbuf.logical = xi->tx_hbs[0].logical;
+
   xi->tx_id_free = 0;
   for (i = 0; i < NET_TX_RING_SIZE; i++)
   {

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

RE: RE: GPLPV: Respecting SG capability

[James Harper]

> 
> James Harper wrote:
> > We may be able to simply tell Windows that we don't support SG and
it
> > may coalesce the buffers itself. I will do some testing of that
before I
> > consider other workarounds.
> 
> As I understand it, this isn't possible to do - Windows insists on
handing us
> several buffers per packet.

Yes, you are right. If we don't support SG then windows still gives us a
string of buffers chained together.

> 
> Attached is a patch against the current source - it would be helpful
to have
> this in the upstream source so that these drivers work out of the box
on
> Solaris
> dom0 (albeit with scatter/gather disabled in the frontend). A future
> improvement
> on this work will be to avoid constructing header_buf in this case,
but this
> gets things working well enough. Another piece of future work will be
to have
> the net driver disable sg if the backend doesn't have "feature-sg" set
to 1 in
> xenstore.
> 
> I can do a push if so desired, just let me know.
> 

Each header buffer is only 512 (TX_HEADER_BUFFER_SIZE) bytes long, so I
don't think your solution will work reliably.

I actually implement the DMA_ADAPTER interface that builds the SG list
for devices on the 'xen' bus (in xenpci_pdo.c). I provide the following
hooks via windows 'device extensions':
. need_virtual_address function - return true if we need the VA of the
mapped buffer instead of the physical address (for scsiport)
. get_alignment function - return the required buffer alignment (again,
for scsiport that needs sectors aligned to 512 byte boundaries. I just
allocate a bounce buffer if required)
. max_sg_elements - the maximum number of sg elements allowed. I fail
any attempt to build an SG list with more than this many breaks, and
Windows just goes back and coalesces the buffers itself and resubmits
the single coalesced buffer.

Unfortunately max_sg_elements is set at the driver level (eg way before
we set up NDIS and know if we support SG or not) and so isn't much use
to you at the moment... I didn't have the foresight to make it a
function. DMA_ADAPTER knows nothing about NDIS so we need to be able to
get the NDIS adapter context (xi) from the DEVICE_OBJECT, and I don't
know how to do that or if it's possible yet...

The function would look something like:

static ULONG
max_sg_elements(DEVICE_OBJECT device_object)
{
  struct xennet_info *xi = SOME_MAGIC_FUNCTION(device_object);
  if (xi->config_sg)
    return 19; /* defined in Linux as a maximum SG size */
  else
    return 1; /* as in SG isn't supported */
}

James

RE: RE: GPLPV: Respecting SG capability

[James Harper]

> Another piece of future work will be to have
> the net driver disable sg if the backend doesn't have "feature-sg" set
to 1 in
> xenstore.

I've just pushed a fix for that - config-sg should now be disabled of
feature-sg is 0 in the backend. Let me know how it goes (nothing
actually uses config-sg yet though).

Contrary to my ramblings in the previous email, it may just be best to
allocate a bunch of MTU sized buffers and coalesce to those instead, as
you have done.

So the patch will need to check for !config_sg and:
. Make sure that TSO can only be a maximum of PAGE_SIZE (assuming sun
supports TSO at all under Xen?)
. Don't allocate the header buffers
. Allocate packet buffers - NET_TX_RING_SIZE (256 I think) should be
enough
. Don't make the call to NdisMInitializeScatterGatherDma at all
. Don't get ScatterGatherListPacketInfo at all (it won't exist due to
the absence of the above call)
. Most of the rest of what you did, except not to hbs but to the new
buffers

The reason for not calling NdisMInitializeScatterGatherDma when
!config_sg is that it's expensive if we aren't going to use it.

Does that sound like something you can put together?

Also remember that NdisMAllocateSharedMemory calls my DMA_ADAPTER code
in xenpci_pdo which constructs a bus-centric ('logical') address of
((GREF << PAGE_SHIFT) | page_offset), eg the grant table ref is encoded
into the logical address, and the lower PAGE_SHIFT (12) bits remain the
same as the virtual address. This is why the xennet driver doesn't have
to bother allocating grant references - the bus driver does it all for
it.

James

Re: GPLPV: Respecting SG capability

[David Edmondson]

* james.harper@bendigoit.com.au [2009-06-19 13:39:14]
> (assuming sun supports TSO at all under Xen?)

Not yet.

dme.
-- 
David Edmondson, Sun Microsystems, http://dme.org