On 05/10/2014 08:36 AM, Tejun Heo wrote:
> On Wed, May 07, 2014 at 03:31:51AM +0530, Srivatsa S. Bhat wrote:
>> diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c
>> index 01fbae5..7abb361 100644
>> --- a/kernel/stop_machine.c
>> +++ b/kernel/stop_machine.c
>> @@ -165,12 +165,13 @@ static void ack_state(struct multi_stop_data *msdata)
>> set_state(msdata, msdata->state + 1);
>> }
>>
>> +
>
> Why add a new line here?
Argh, a stray newline.. will remove it.
>
>> /* This is the cpu_stop function which stops the CPU. */
>> static int multi_cpu_stop(void *data)
>> {
>> struct multi_stop_data *msdata = data;
>> enum multi_stop_state curstate = MULTI_STOP_NONE;
>> - int cpu = smp_processor_id(), err = 0;
>> + int cpu = smp_processor_id(), num_active_cpus, err = 0;
>
> TYPE var0 = INIT0, var1, var2 = INIT2;
>
> looks kinda weird. Maybe collect initialized ones to one side or
> separate out uninitialized one to a separate declaration?
>
Yeah, now that you point out, it does look very odd. I don't
remember why I wrote it that way in the first place! :-(
I'll fix this in the next version. Thanks!
> Also, isn't nr_active_cpus more common way of naming it?
>
Sure, will use this convention.
>> unsigned long flags;
>> bool is_active;
>>
>> @@ -180,15 +181,38 @@ static int multi_cpu_stop(void *data)
>> */
>> local_save_flags(flags);
>>
>> - if (!msdata->active_cpus)
>> + if (!msdata->active_cpus) {
>> is_active = cpu == cpumask_first(cpu_online_mask);
>> - else
>> + num_active_cpus = 1;
>> + } else {
>> is_active = cpumask_test_cpu(cpu, msdata->active_cpus);
>> + num_active_cpus = cpumask_weight(msdata->active_cpus);
>> + }
>>
>> /* Simple state machine */
>> do {
>> /* Chill out and ensure we re-read multi_stop_state. */
>> cpu_relax();
>> +
>> + /*
>> + * In the case of CPU offline, we don't want the other CPUs to
>> + * send IPIs to the active_cpu (the one going offline) after it
>> + * has entered the _DISABLE_IRQ state (because, then it will
>> + * notice the IPIs only after it goes offline). So ensure that
>> + * the active_cpu always follows the others while entering
>> + * each subsequent state in this state-machine.
>> + *
>> + * msdata->thread_ack tracks the number of CPUs that are yet to
>> + * move to the next state, during each transition. So make the
>> + * active_cpu(s) wait until ->thread_ack indicates that the
>> + * active_cpus are the only ones left to complete the
>> transition.
>> + */
>> + if (is_active) {
>> + /* Wait until all the non-active threads ack the state
>> */
>> + while (atomic_read(&msdata->thread_ack) >
>> num_active_cpus)
>> + cpu_relax();
>> + }
>
> Wouldn't it be cleaner to separate this out to a separate stage so
> that there are two separate DISABLE_IRQ stages - sth like
> MULTI_STOP_DISABLE_IRQ_INACTIVE and MULTI_STOP_DISABLE_IRQ_ACTIVE?
> The above adds an ad-hoc mechanism on top of the existing mechanism
> which is built to sequence similar things anyway.
>
Indeed, that looks like a much more elegant method! Thanks a lot for the
suggestion Tejun, I'll use that in the next version of the patchset.
Thank you!
Regards,
Srivatsa S. Bhat
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
