Re: [Pv-ops][PATCH] Netback multiple tasklet support

[Jeremy Fitzhardinge <jeremy@goop.org>]

On 12/03/09 18:13, Xu, Dongxiao wrote:
>>      * same with "foreign_page_tracker"
>>            o (the foreign page tracker API should have better names,
>>              but that's not your problem)
>>      * What's cpu_online_nr for?  I don't think it should be necessary
>>        at all, and if it is, then it needs a much more distinct name.
>>      * If they're really per-cpu variables, they should use the percpu
>>        mechanism
>>      
> Actually those tasklets are not per-cpu variables.
> We just defined cpu_online_nr of tasklets, in order to get the best performance
> if each tasklet could run on each cpu. However, they are not binded with cpus.
> Some tasklets may run on the same vcpu of dom0 due to interrupt delivery
> affinity. Therefore these tasklets are not per-cpu variables.
>    

OK, you should name the variable to what it actually means, not what its 
value happens to be.  It seems like a parameter which should be 
adjustable via sysfs or something.

How did you arrive at 3?

>>      * How do you relate the number of online CPUs to the whole group
>>        index/pending index computation?  It isn't obvious how they're
>>        connected, or how it guarantees that the index is enough.
>>      
> Same explaination as above. Whether online cpus number is greater or less than
> tasklet number does not matter in our case. We defined them to the same value
> is only for getting best performance.
>    

Nevertheless, it isn't at all clear how we can be certain the index 
calculations are less guaranteed to be less than the number of 
tasklets.  There is a lot of code scattered around the place; perhaps 
you could condense it into a smaller number of places?

In fact, the overall patch size is very large, and hard to review and 
test.  Could you please give some thought to how you can incrementally 
modify netback to get the result you want.  For example, keep the 
current functional structure, but make the changes to generalize to N 
processing handlers (but keeping N=1), then convert the softirq to a 
tasklet, then make N > 1.

Thanks,
     J