[lkml]   [2011]   [Dec]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCHSET] block, mempool, percpu: implement percpu mempool and fix blkcg percpu alloc deadlock
    On Thu, 22 Dec 2011 20:21:12 -0500 Vivek Goyal <> wrote:

    > On Thu, Dec 22, 2011 at 03:41:38PM -0800, Andrew Morton wrote:
    > >
    > > If the code *does* correctly handle ->stats_cpu == NULL then we have
    > > options.
    > >
    > > a) Give userspace a new procfs/debugfs file to start stats gathering
    > > on a particular cgroup/request_queue pair. Allocate the stats
    > > memory in that.
    > >
    > > b) Or allocate stats_cpu on the first call to blkio_read_stat_cpu()
    > > and return zeroes for this first call.
    > But the purpose of stats is that they are gathered even if somebody
    > has not read them even once?

    That's not a useful way of using stats. The normal usage would be to
    record the stats then start the workload then monitor how the stats
    have changed as work proceeds.

    > So if I create a cgroup and put some
    > task into it which does some IO, I think stat collection should start
    > immediately without user taking any action.

    If you really want to know the stats since cgroup creation then trigger
    the stats initialisation from userspace when creating the blkio_cgroup.

    > Forcing the user to first
    > read a stat before the collection starts is kind of odd to me.

    Well one could add a separate stats_enable knob. Doing it
    automatically from read() would be for approximate-back-compatibility
    with existing behaviour.

    Plus (again) this way we also avoid burdening non-stats-users with the
    overhead of stats.

    > >
    > > c) Or change the low-level code to do
    > > blkio_group.want_stats_cpu=true, then test that at the top level
    > > after we've determined that blkio_group.stats_cpu is NULL.
    > >
    > > d) Or, worse, punt the allocation into a workqueue thread.
    > I implemented a patch to punt the allocation using a worker thread. Tejun
    > did not like it. I personally think that it is less intrusive to fix this
    > specific problem.


    Look, the basic problem here is a highish-level design one. We're
    attempting to do a high-level heavyweight intialization operation in a
    low-level place. How do we fix that *properly*? It has to be by
    performing the heavyweight operation in an appropriate place.

    > >
    > > Note that all these option will permit us to use GFP_KERNEL, which is
    > > better.
    > >
    > > Note that a) and b) means that users get control over whether these
    > > stats are accumulated at all, so many won't incur needless memory and
    > > CPU consumption.
    > >
    > > I think I like b). Fix the code so it doesn't oops when ->stats_cpu is
    > > NULL, then turn on stats gathering the first time someone tries to read
    > > the stats.
    > >
    > > (Someone appears to have misspelled "throttle" as "throtl" for no
    > > apparent reason about 1000 times. Sigh.)
    > That someone would be me. I thought that throtl communicates the meaning
    > and keeps the length of all the strings relatively short. But if it does
    > not look good, I can change it.

    It's unconventional. We do usually avoid the odd abbreviations and
    just spell the whole thing out.

     \ /
      Last update: 2011-12-23 02:37    [W:0.037 / U:9.716 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site