[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 1/4] xen/netback: implements persistent grant with one page pool.





On 2012-11-15 17:10, Ian Campbell wrote:
On Thu, 2012-11-15 at 07:04 +0000, Annie Li wrote:
This patch implements persistent grant in netback driver. Tx and rx
share the same page pool, this pool will be split into two parts
in next patch.

Signed-off-by: Annie Li<annie.li@xxxxxxxxxx>
---
  drivers/net/xen-netback/common.h    |   18 +++-
  drivers/net/xen-netback/interface.c |   22 ++++
  drivers/net/xen-netback/netback.c   |  212 +++++++++++++++++++++++++++++++----
  drivers/net/xen-netback/xenbus.c    |   14 ++-
  4 files changed, 239 insertions(+), 27 deletions(-)

diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
index 94b79c3..a85cac6 100644
--- a/drivers/net/xen-netback/common.h
+++ b/drivers/net/xen-netback/common.h
@@ -45,8 +45,19 @@
  #include<xen/grant_table.h>
  #include<xen/xenbus.h>

+#define XEN_NETIF_TX_RING_SIZE __CONST_RING_SIZE(xen_netif_tx, PAGE_SIZE)
+#define XEN_NETIF_RX_RING_SIZE __CONST_RING_SIZE(xen_netif_rx, PAGE_SIZE)
+#define MAXIMUM_OUTSTANDING_BLOCK_REQS \
BLOCK?

Oh, an error when splitting the patch, will fix it, thanks.


+                       (XEN_NETIF_TX_RING_SIZE + XEN_NETIF_RX_RING_SIZE)
+
  struct xen_netbk;

+struct persistent_entry {
+       grant_ref_t forgranted;
+       struct page *fpage;
+       struct gnttab_map_grant_ref map;
+};
Isn't this duplicating a bunch of infrastructure which is also in
blkback? Can we put it into some common helpers please?

Yes,

"struct gnttab_map_grant_ref map" can be changed to handle like blkback to keep 
same as blkback, and share them in common helpers.



+
  struct xenvif {
         /* Unique identifier for this interface. */
         domid_t          domid;
@@ -75,6 +86,7 @@ struct xenvif {

         /* Internal feature information. */
         u8 can_queue:1;     /* can queue packets for receiver? */
+       u8 persistent_grant:1;

         /*
          * Allow xenvif_start_xmit() to peek ahead in the rx request
@@ -98,6 +110,9 @@ struct xenvif {
         struct net_device *dev;

         wait_queue_head_t waiting_to_free;
+
+       struct persistent_entry *persistent_gnt[MAXIMUM_OUTSTANDING_BLOCK_REQS];
What is the per-vif memory overhead of this array?
In this patch,
The maximum of memory overhead is about

(XEN_NETIF_TX_RING_SIZE+XEN_NETIF_RX_RING_SIZE)*PAGE_SIZE  (plus size of 
grant_ref_t and handle)
which is about 512 PAGE_SIZE. Normally, without heavy network offload, this 
maximum can not be reached.

In next patch of splitting tx/rx pool, the maximum is about (256+512)PAGE_SIZE.


+static struct persistent_entry*
+get_per_gnt(struct persistent_entry **pers_entry,
+           unsigned int count, grant_ref_t gref)
+{
+       int i;
+
+       for (i = 0; i<  count; i++)
+               if (gref == pers_entry[i]->forgranted)
+                       return pers_entry[i];
Isn't this linear scan rather expensive? I think Roger implemented some
sort of hash lookup for blkback which I think is required here too (and
should be easy if you make that code common).

Agree, thanks.


+
+       return NULL;
+}
+
@@ -1338,7 +1497,11 @@ static unsigned xen_netbk_tx_build_gops(struct xen_netbk 
*netbk)
                 gop->source.domid = vif->domid;
                 gop->source.offset = txreq.offset;

-               gop->dest.u.gmfn = virt_to_mfn(page_address(page));
+               if (!vif->persistent_grant)
+                       gop->dest.u.gmfn = virt_to_mfn(page_address(page));
+               else
+                       gop->dest.u.gmfn = (unsigned long)page_address(page);
page_address doesn't return any sort of frame number, does it? This is
rather confusing...

Yes. I only use dest.u.gmfn element to save the page_address here for future memcpy, and it does not mean to use frame number actually. To avoid confusion, here I can use

gop->dest.u.gmfn = virt_to_mfn(page_address(page));

and then call mfn_to_virt when doing memcpy.



@@ -453,7 +460,12 @@ static int connect_rings(struct backend_info *be)
                 val = 0;
         vif->csum = !val;

-       /* Map the shared frame, irq etc. */
+       if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-persistent-grants",
+                        "%u",&val)<  0)
+               val = 0;
+       vif->persistent_grant = !!val;
+
+/* Map the shared frame, irq etc. */
Please run the patches through checkpatch.pl

Yes, I run checkpatch.pl before posting them. The only warning exists in initial code netfront.c, it is a printk code in xennet_tx_buf_gc, I did not fix that.

Thanks
Annie

         err = xenvif_connect(vif, tx_ring_ref, rx_ring_ref, evtchn);
         if (err) {
                 xenbus_dev_fatal(dev, err,
--
1.7.3.4



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.