On Thu, Mar 17, 2016 at 08:30:15AM +0100, Thomas Huth wrote: > On 17.03.2016 07:23, David Gibson wrote: > > On Wed, Mar 16, 2016 at 01:16:50PM +0100, Thomas Huth wrote: > >> > >> This patch introduces an alternate way of handling the receive > >> buffers of the spapr-vlan device, resulting in much better > >> receive performance for the guest. > [...] > >> Though it seems at a first glance like PAPR says that we should store > >> the receive buffer descriptors in the page that is supplied during > >> the H_REGISTER_LOGICAL_LAN call, chapter 16.4.1.2 in the LoPAPR spec > >> declares that "the contents of these descriptors are architecturally > >> opaque, none of these descriptors are manipulated by code above > >> the architected interfaces". > > > > Aaaahhh! I remember back when I first implemented this, that exposing > > the pool of buffer descriptors via DMA seemed a silly and pointless > > thing to do, but I did so because I thought that's what the spec said. > > > > 16.4.1.2 seems to make it clearer that the page doesn't list actual Rx > > buffers, but rather opaque handles on internal buffer pools. > > > > I don't know if I just misread this back in 2011 (or whenever it was) > > or if the PAPR wording at the time was less clear at the time. > > > > I note that you don't actually put the buffer pool pointers into that > > page in your patch below. I don't think that matters now, but I > > wonder if we'd ever want to implement H_MIGRATE_DMA and if we'd need > > it in that case. > > I also thought about putting the pointers to the pools into that page. > But: If we put buffer pool pointers into that page, where should the > buffer pools be located? Still in the memory of the hypervisor? Then > this sounds like a very baaad design, the guest then could tinker with > pointers to the host memory, causing very bad side effects or crashes. > Or should the buffer pools be located in guest memory? That might be OK, > but how do the pools get allocated in that case? > > So unless you've got a good idea here, I think it's better to keep the > pointer list and the buffer pools both in host memory for now.
Yes, I think you're right. > [...] > >> +/** > >> + * Enqueuing receive buffer by adding it to one of our receive buffer > >> pools > >> + */ > >> +static target_long spapr_vlan_add_rxbuf_to_pool(VIOsPAPRVLANDevice *dev, > >> + target_ulong buf) > >> +{ > >> + int size = VLAN_BD_LEN(buf); > >> + int pool; > >> + > >> + pool = spapr_vlan_get_rx_pool_id(dev, size); > >> + > >> + /* No matching pool found? Try to create a new one */ > >> + if (pool < 0) { > >> + for (pool = RX_MAX_POOLS - 1; pool >= 0 ; pool--) { > > > > I don't think this loop actually accomplishes anything. Either the > > last slot is free, in which case you use it, then sort into place, or > > it's not, in which case you've hit the maximum number of buffer pools. > > Oh, you're right. Well spotted! I'll rework my patch to do it without > that loop. > > Thomas > > -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature