On 01/08/2012 06:13 PM, Gilad Ben-Yossef wrote: > On Mon, Jan 2, 2012 at 3:30 PM, Avi Kivity <avi@xxxxxxxxxx> wrote: > > On 01/02/2012 01:59 PM, Gilad Ben-Yossef wrote: > >> On Sun, Jan 1, 2012 at 6:50 PM, Avi Kivity <avi@xxxxxxxxxx> wrote: > >> > On 01/01/2012 06:12 PM, Gilad Ben-Yossef wrote: > >> >> > > >> >> > Since this seems to be a common pattern, how about: > >> >> > > >> >> > zalloc_cpumask_var_or_all_online_cpus(&cpus, GFTP_ATOMIC); > >> >> > ... > >> >> > free_cpumask_var(cpus); > >> >> > > >> >> > The long-named function at the top of the block either returns a newly > >> >> > allocated zeroed cpumask, or a static cpumask with all online cpus set. > >> >> > The code in the middle is only allowed to set bits in the cpumask > >> >> > (should be the common usage). free_cpumask_var() needs to check whether > >> >> > the freed object is the static variable. > >> >> > >> >> Thanks for the feedback and advice! I totally agree the repeating > >> >> pattern needs abstracting. > >> >> > >> >> I ended up chosing to try a different abstraction though - basically a wrapper > >> >> on_each_cpu_cond that gets a predicate function to run per CPU to > >> >> build the mask > >> >> to send the IPI to. It seems cleaner to me not having to mess with > >> >> free_cpumask_var > >> >> and it abstracts more of the general pattern. > >> >> > >> > > >> > This converts the algorithm to O(NR_CPUS) from a potentially lower > >> > complexity algorithm. Also, the existing algorithm may not like to be > >> > driven by cpu number. Both are true for kvm. > >> > > >> > >> Right, I was only thinking on my own uses, which are O(NR_CPUS) by nature. > >> > >> I wonder if it would be better to create a safe_cpumask_var type with > >> its own alloc function > >> free and and sset_cpu function but no clear_cpu function so that the > >> compiler will catch > >> cases of trying to clear bits off of such a cpumask? > >> > >> It seems safer and also makes handling the free function easier. > >> > >> Does that makes sense or am I over engineering it? :-) > > > > It makes sense. Depends on the number of call sites, really. If there > > are several, consolidation helps, also makes it easier to further refactor. > > As Andrew pointed out code that usually calls just the CPU you wanted but > sometime (under memory pressure) might call other CPUs is a problem > waiting to happen, so I took his advice and re factored to use > smp_call_function_single to IPI each CPU individually in case of alloc failure. > > I don't know if that applied to the KVM use case. I'm guessing it > probably doesn't > from what you wrote here. No, it doesn't. But note it isn't the case you're covering anyway. kvm already only IPIs relevant cpus, it just suffers from that ugly allocation failure case. -- error compiling committee.c: too many arguments to function -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/ Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>