Re: [Xen-devel] [PATCH 2/3] xen-netback: switch to per-cpu scratch space

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, May 28, 2013 at 05:47:25PM +0800, annie li wrote:
[...]
> >-    struct gnttab_copy grant_copy_op[2*XEN_NETIF_RX_RING_SIZE];
> >-    struct netbk_rx_meta meta[2*XEN_NETIF_RX_RING_SIZE];
> >  };
> >  static struct xen_netbk *xen_netbk;
> >@@ -608,12 +613,31 @@ static void xen_netbk_rx_action(struct xen_netbk 
> >*netbk)
> >     int count;
> >     unsigned long offset;
> >     struct skb_cb_overlay *sco;
> >+    struct gnttab_copy *gco = get_cpu_var(grant_copy_op);
> >+    struct netbk_rx_meta *m = get_cpu_var(meta);
> 
> Change m to a friendly name?

Changed to "meta".

> 
> >+    static int unusable_count;
> >     struct netrx_pending_operations npo = {
> >-            .copy  = netbk->grant_copy_op,
> >-            .meta  = netbk->meta,
> >+            .copy = gco,
> >+            .meta = m,
> >     };
> >+    if (gco == NULL || m == NULL) {
> >+            put_cpu_var(grant_copy_op);
> >+            put_cpu_var(meta);
> >+            if (unusable_count == 1000) {
> 
> It is better to use a macro to replace this number here.
> BTW, can you explain why using 1000 here?
> 

This is just a random number chosen to avoid flooding dmesg log. :-)

Re macro, this is the only place that use this test, so no need for a
macro. Following test in TX path look similar, but the put_cpu_var() in
that branch is different, I doubt we can get much from defining a macro
here.

> >+                    printk(KERN_ALERT
> >+                           "xen-netback: "
> >+                           "CPU %d scratch space is not available,"
> >+                           " not doing any TX work for netback/%d\n",
> >+                           smp_processor_id(),
> >+                           (int)(netbk - xen_netbk));
> 
> unusable_count is not a value based on netbk here. I assume you use
> unusable_count to judge whether scratch space is available for
> specific netbk, if so, then unusable_count needs to be counter for
> specific netbk, not for all netbk.
> 

No, it is not based on netbk. It is for a particular CPU. Per-cpu
scratch is for CPUs not netbks. Netback thread can always be scheduled
on other CPU in 1:1 model, so in practice it will almost never print out
this warning if unusable_count is for netbk.

[...]
> >+
> >+            if (unusable_count == 1000) {
> 
> Same as above
> 
[...]
> >+    return 0;
> >+}
> >+
> >+static void __free_percpu_scratch_space(unsigned int cpu)
> >+{
> >+    void *tmp;
> >+
> >+    tmp = per_cpu(tx_copy_ops, cpu);
> 
> It is better to verify whether tmp is available before freeing it,
> for example: if (tmp)
> 

No need to do this, as it is legit to free() a NULL pointer.

> 
[...]
> >+
> >+static struct notifier_block netback_notifier_block = {
> >+    .notifier_call = __netback_percpu_callback,
> >+};
> 
> Moving this to the top of this file?
> 

It is sort of convention to put this sort of things in the back rather
in the front.

> >+

[...]



Wei.

> 
> Thanks
> Annie
> 

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