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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
> > Hi James, > > As you may have heard, the latest GPLPV release doesn''t work on > Opensolaris dom0. Our backend net driver doesn''t supportscatter/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 backendanyway,> 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 (seebelow).> 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 shouldbe> 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 asingle> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
> > James Harper wrote: > > We may be able to simply tell Windows that we don''t support SG andit> > may coalesce the buffers itself. I will do some testing of thatbefore I> > consider other workarounds. > > As I understand it, this isn''t possible to do - Windows insists onhanding 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 helpfulto have> this in the upstream source so that these drivers work out of the boxon> 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 beto have> the net driver disable sg if the backend doesn''t have "feature-sg" setto 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
> Another piece of future work will be to have > the net driver disable sg if the backend doesn''t have "feature-sg" setto 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
* 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel