lkml.org 
[lkml]   [2021]   [Jun]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH V4 05/12] trace/hwlat: Support hotplug operations
    From
    Date
    On 6/21/21 7:46 PM, Steven Rostedt wrote:
    > On Mon, 21 Jun 2021 18:14:36 +0200
    > Daniel Bristot de Oliveira <bristot@redhat.com> wrote:
    >
    >>>> Yep! I tried to take the trace_type_lock here, and got the lockdep info about
    >>>> this problem.
    >>>>
    >>>>> The only thing I could think of is to wake up a worker thread to do the
    >>>>> work. That is, this just wakes the worker thread, then the worker grabs
    >>>>> the trace_types_lock, iterates through the cpu mask of expect running
    >>>>> threads, and then starts or kills them depending on the hwlat_busy
    >>>>> value.
    >>>> So, it will not wait for the kworker to run?
    >>> What wont wait?
    >>
    >> For example, at the shutdown, should the hotplug callback wait for the workqueue
    >> to run & kill the thread, or not?
    >
    > Doing that won't help the deadlock situation.

    yep, that is why I asked... :-(

    > CPU 1 CPU 2
    > ----- -----
    > Start shutdown
    > down online_cpus()
    >
    > mutex_lock(trace_types_lock);
    > get_online_cpus()
    > [BLOCK]
    >
    > wake_up_thread;
    > [schedule worker]
    >
    > mutex_lock(trace_types_lock);
    >
    > [ DEADLOCK ]
    >
    >
    > Make all access to save_cpumask and hwlat_per_cpu_data inside the
    > get_online_cpus() protection. (like in move_to_next_cpu(),
    > start_single_thread() expand the get_online_cpus()).
    >
    > Then in the cpu going down case, we can simply kill the thread and
    > update the save_cpumask, as it will be protected by the
    > get_online_cpus() code.
    >
    > That is, don't even check if hwlat_busy is set or not. Just simply do:
    >
    >
    > CPU_DOWN:
    >
    > stop_cpu_kthead(cpu);
    >
    > That will stop the kthread if it is running. But we should update
    > that function to also set per_cpu(hwlat_per_cpu_data).kthread = NULL;
    > Like stop_single_kthread() does.
    >
    > But for CPU_UP, we should do the work via a worker thread.
    >
    > CPU_UP:
    > schedule_work_on(&update_kthreads, cpu);
    >
    > Which in the work function for that update_kthreads work queue:
    >
    > mutex_lock(&trace_types_lock);
    > if (!hwlat_busy || hwlat_data.thread_mode != MODE_PER_CPU)
    > goto out_unlock;
    >
    > get_online_cpus();
    > if (!this_cpu(hwlat_per_cpu_data).kthread)
    > start_per_cpu_kthread(smp_processor_id());
    > put_online_cpus();
    >
    > out_unlock:
    > mutex_unlock(&trace_types_lock);
    >
    > Or something like that.

    It works!

    I will send a v5 with all the fixes requested, including this one.

    Thanks Steven!
    -- Daniel

    \
     
     \ /
      Last update: 2021-06-22 16:06    [W:2.612 / U:0.028 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site