Re: [PATCH 1/6] cpuset write dirty map

[Satyam Sharma]

On 9/15/07, Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Tue, 11 Sep 2007 18:36:34 -0700
> Ethan Solomita <solo@xxxxxxxxxx> wrote:

> > The dirty map may be stored either directly in the mapping (for NUMA
> > systems with less then BITS_PER_LONG nodes) or separately allocated
> > for systems with a large number of nodes (f.e. IA64 with 1024 nodes).

> > --- 0/include/linux/fs.h      2007-09-11 14:35:58.000000000 -0700
> > +++ 1/include/linux/fs.h      2007-09-11 14:36:24.000000000 -0700
> > @@ -516,6 +516,13 @@ struct address_space {
> >       spinlock_t              private_lock;   /* for use by the address_space */
> >       struct list_head        private_list;   /* ditto */
> >       struct address_space    *assoc_mapping; /* ditto */
> > +#ifdef CONFIG_CPUSETS
> > +#if MAX_NUMNODES <= BITS_PER_LONG
> > +     nodemask_t              dirty_nodes;    /* nodes with dirty pages */
> > +#else
> > +     nodemask_t              *dirty_nodes;   /* pointer to map if dirty */
> > +#endif
> > +#endif
>
> afacit there is no code comment and no changelog text which explains the
> above design decision?  There should be, please.

> > +/*
> > + * Special functions for NUMA systems with a large number of nodes.
> > + * The nodemask is pointed to from the address space structures.
> > + * The attachment of the dirty_node mask is protected by the
> > + * tree_lock. The nodemask is freed only when the inode is cleared
> > + * (and therefore unused, thus no locking necessary).
> > + */
>
> hmm, OK, there's a hint as to wghat's going on.
>
> It's unobvious why the break point is at MAX_NUMNODES = BITS_PER_LONG and
> we might want to tweak that in the future.  Yet another argument for
> centralising this comparison.

Looks like just an optimization to me ... Ethan wants to economize and not bloat
struct address_space too much.

So, if sizeof(nodemask_t) == sizeof(long), i.e. when:
MAX_NUMNODES <= BITS_PER_LONG, then we'll be adding only sizeof(long)
extra bytes to the struct (by plonking the object itself into it).

But even when MAX_NUMNODES > BITS_PER_LONG, because we're storing
a pointer, and because sizeof(void *) == sizeof(long), so again the maximum
bloat addition to struct address_space would only be sizeof(long) bytes.

I didn't see the original mail, but if the #ifdeffery for this
conditional is too much
as a result of this optimization, Ethan should probably just do away
with all of it
entirely, and simply put a full nodemask_t object (irrespective of MAX_NUMNODES)
into the struct. After all, struct task_struct does the same unconditionally ...
but admittedly, there are several times more address_space struct's resident in
memory at any given time than there are task_struct's, so this optimization does
make sense too ...


> > +             if (!nodes)
> > +                     return;
> > +
> > +             *nodes = NODE_MASK_NONE;
> > +             mapping->dirty_nodes = nodes;
> > +     }
> > +
> > +     if (!node_isset(node, *nodes))
> > +             node_set(node, *nodes);
> > +}
> > +
> > +void cpuset_clear_dirty_nodes(struct address_space *mapping)
> > +{
> > +     nodemask_t *nodes = mapping->dirty_nodes;
> > +
> > +     if (nodes) {
> > +             mapping->dirty_nodes = NULL;
> > +             kfree(nodes);
> > +     }
> > +}
>
> Can this race with cpuset_update_dirty_nodes()?  And with itself?  If not,
> a comment which describes the locking requirements would be good.
>
> > +/*
> > + * Called without the tree_lock. The nodemask is only freed when the inode
> > + * is cleared and therefore this is safe.
> > + */
> > +int cpuset_intersects_dirty_nodes(struct address_space *mapping,
> > +                     nodemask_t *mask)
> > +{
> > +     nodemask_t *dirty_nodes = mapping->dirty_nodes;
> > +
> > +     if (!mask)
> > +             return 1;
> > +
> > +     if (!dirty_nodes)
> > +             return 0;
> > +
> > +     return nodes_intersects(*dirty_nodes, *mask);
> > +}
> > +#endif
> > +
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/