lkml.org 
[lkml]   [2012]   [Dec]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [patch,v2] bdi: add a user-tunable cpu_list for the bdi flusher threads
    On Mon, Dec 03, 2012 at 01:53:39PM -0500, Jeff Moyer wrote:
    > Hi,
    >
    > In realtime environments, it may be desirable to keep the per-bdi
    > flusher threads from running on certain cpus. This patch adds a
    > cpu_list file to /sys/class/bdi/* to enable this. The default is to tie
    > the flusher threads to the same numa node as the backing device (though
    > I could be convinced to make it a mask of all cpus to avoid a change in
    > behaviour).

    The default seems reasonable to me.

    > Comments, as always, are appreciated.
    .....

    > +static ssize_t cpu_list_store(struct device *dev,
    > + struct device_attribute *attr, const char *buf, size_t count)
    > +{
    > + struct backing_dev_info *bdi = dev_get_drvdata(dev);
    > + struct bdi_writeback *wb = &bdi->wb;
    > + cpumask_var_t newmask;
    > + ssize_t ret;
    > + struct task_struct *task;
    > +
    > + if (!alloc_cpumask_var(&newmask, GFP_KERNEL))
    > + return -ENOMEM;
    > +
    > + ret = cpulist_parse(buf, newmask);
    > + if (!ret) {
    > + spin_lock(&bdi->wb_lock);
    > + task = wb->task;
    > + if (task)
    > + get_task_struct(task);
    > + spin_unlock(&bdi->wb_lock);
    > + if (task) {
    > + ret = set_cpus_allowed_ptr(task, newmask);
    > + put_task_struct(task);
    > + }

    Why is this set here outside the bdi->flusher_cpumask_mutex?

    Also, I'd prefer it named "..._lock" as that is the normal
    convention for such variables. You can tell the type of lock from
    the declaration or the use...

    ....

    > @@ -437,6 +488,14 @@ static int bdi_forker_thread(void *ptr)
    > spin_lock_bh(&bdi->wb_lock);
    > bdi->wb.task = task;
    > spin_unlock_bh(&bdi->wb_lock);
    > + mutex_lock(&bdi->flusher_cpumask_mutex);
    > + ret = set_cpus_allowed_ptr(task,
    > + bdi->flusher_cpumask);
    > + mutex_unlock(&bdi->flusher_cpumask_mutex);

    As it is set under the lock here....

    Cheers,

    Dave.
    --
    Dave Chinner
    david@fromorbit.com


    \
     
     \ /
      Last update: 2012-12-04 04:21    [W:3.565 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site