[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel][Pv-ops][PATCH 0/3] Resend: Netback multiple thread support
> >> I'd like to make an update on these patches. The main logic is not > >> changed, and I only did a rebase towards the upstream pv-ops kernel. > >> See attached patch. The original patches are checked in Jeremy's > >> netback-tasklet branch. > > I have a couple of (quite minor) comments on the patches: > > > > 0001-Netback-Generilize-static-global-variables-into-stru.txt: > >> diff --git a/drivers/xen/netback/netback.c > >> b/drivers/xen/netback/netback.c index c24debf..a484b0a 100644 --- > >> a/drivers/xen/netback/netback.c +++ b/drivers/xen/netback/netback.c > >> @@ -49,18 +49,13 @@ > >> > >> /*define NETBE_DEBUG_INTERRUPT*/ > >> > >> +struct xen_netbk *xen_netbk; > > > >> +int group_nr = 1; > >> +struct page_foreign_tracker *foreign_page_tracker; > > I think these two would benefit from more descriptive names, given > > that they're not static. > Oops...I thought I had modified this when Jeremy commented this > last time, maybe there was some mistake and I left it until today... Easily done. > > If I was feeling pedantic, I'd complain that this includes some bits > > of support for multiple struct xen_netbks, rather than just moving all > > of the fields around, which reduces its obviously-correct-ness quite a > > bit. > Actually I was struggling how to split the first patch into small ones, > however I don't have much idea since the patch changes the function > Interface/data structure, so the corresponding caller needs change too, > which generates a 1k line of patch... The approach I would take would be something like this: 1) Gather all the global data together into a struct xen_netbk, and then have a single, global, instance of that structure. Go through and turn every reference to a bit of global data into a reference to a field of that structure. This will be a massive patch, but it's purely mechanical and it's very easy to check that it's safe. 2) Introduce struct ext_page and use it everywhere you use it in the current patch. This should be fairly small. 3) Generalise to multiple struct xen_netbks by changing the single global instance into a struct xen_netbk * and fixing the resulting compile errors. Another big patch, but provided you remember to initialise everything properly the compiler will check almost all of it for you. This is to some extent a bikeshed argument, so if you prefer the current patch structure then that would work as well. > > Even more pedantically, it might be better to pass around a struct > > xen_netbk in a few places, rather than an int group, so that you get > > better compiler type checking. > I will change this in next version of patch. Thanks. > > 0002-Netback-Multiple-tasklets-support.txt: > > Design question: you do your balancing by making each tasklet serve > > roughly equal numbers of remote domains. Would it not have been > > easier to make them serve equal numbers of netfronts? You could then > > get rid of the domain_entry business completely and just have a count > > of serviced netfronts in each xen_netbk, which might be a bit easier > > to deal with. > According to my understanding, one guest with VNIF driver represented > by one netfront. Is this true? Therefore I don't understand the difference > between "number of domains" and "number of netfronts", I used to thought > they were the same. Please correct me my understanding is wrong. I think we might be using slightly different terminology here. When I say ``netfront'', I mean the frontend half of a virtual network interface, rather than the netfront driver, so a single domain can be configured with multiple netfronts in the same way that a single physical host can have multiple ixgbes (say), despite only having one ixgbe driver loaded. So, my original point was that it might be better to balance interfaces such that the number of interfaces in each group is the same, ignoring the frontend domain ID completely. This would mean that if, for instance, a domain had two very busy NICs then they wouldn't be forced to share a tasklet, which might otherwise be a bottleneck. The downside, of course, is that it would allow domains with multiple vNICs to use more dom0 CPU time, potentially aggravating starvation and unfairness problems. On the other hand, a domain with N vNICs wouldn't be able to do any more damage than N domains with 1 vNIC each, so I don't think it's too bad. > Actually in the very early stage of this patch, I use a simple method to > identify which group does a netfront belong to, by calculating > (domid % online_cpu_nr()). The advantage is simple, however it may > cause netfront count unbalanced between different groups. Well, any static scheme will potentially come unbalanced some of the time, if different interfaces experience different levels of traffic. But see the other thread for another discussion of balancing issues. > I will try to remove "domain_entry" related code in next version patch. Thanks. > >> @@ -1570,6 +1570,7 @@ static int __init netback_init(void) > >> /* We can increase reservation by this much in net_rx_action(). */ > >> // balloon_update_driver_allowance(NET_RX_RING_SIZE); > >> > >> + group_nr = num_online_cpus(); > >> xen_netbk = kzalloc(group_nr * sizeof(struct xen_netbk), > >> GFP_KERNEL); if (!xen_netbk) { printk(KERN_ALERT "%s: out of > >> memory\n", __func__); > > What happens if the number of online CPUs changes while netback is > > running? In particular, do we stop trying to send a tasklet/thread to > > a CPU which has been offlined? > The group_nr just defines the max number of tasklets, however it doesn't > decide > which tasklet is handled by which CPU. It is decided by the delivery of > interrupt. Ah, yes, so it is. Thanks for explaining it. > > 0003-Use-Kernel-thread-to-replace-the-tasklet.txt: > >> diff --git a/drivers/xen/netback/netback.c > >> b/drivers/xen/netback/netback.c index 773cd4f..8b55efc 100644 --- > >> a/drivers/xen/netback/netback.c +++ b/drivers/xen/netback/netback.c > >> +static int netbk_action_thread(void *index) > >> +{ > >> + int group = (long)index; > >> + struct xen_netbk *netbk = &xen_netbk[group]; > >> + while (1) { > >> + wait_event_interruptible(netbk->netbk_action_wq, > >> + rx_work_todo(group) + > >> || tx_work_todo(group)); > >> + cond_resched(); > >> + > >> + if (rx_work_todo(group)) > >> + net_rx_action(group); > >> + > >> + if (tx_work_todo(group)) > >> + net_tx_action(group); > >> + } > > Hmm... You use kthread_stop() on this thread, so you should probably > > test kthread_should_stop() every so often. > OK, I will modify it. Thanks. Steven. Attachment:
signature.asc _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxx http://lists.xensource.com/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |