Messages in this thread | | | Date | Tue, 27 Mar 2012 13:47:34 +0100 | From | Mel Gorman <> | Subject | Re: [PATCH] cpuset: mm: Reduce large amounts of memory barrier related damage v3 |
| |
On Mon, Mar 26, 2012 at 06:20:52PM +0200, Peter Zijlstra wrote: > > <SNIP> > > > > I think such a change would be better but should also rename the API. > > If developers see a get_foo type call, they will expect to see a put_foo > > call or assume it's a bug even though the implementation happens to be ok > > with that. Any suggestion on what a good new name would be? > > > > How about read_mems_allowed_begin() and read_mems_allowed_retry()? > > > > read_mems_allowed_begin would be a rename of get_mems_allowed(). In an > > error path, read_mems_allowed_retry() would documented to be *optionally* > > called when deciding whether to retry the operation or not. In this scheme, > > !put_mems_allowed would become read_mems_allowed_retry() which might be > > a bit easier to read overall. > > One: > > git grep -l "\(get\|put\)_mems_allowed" | while read file; do sed -i -e > 's/\<get_mems_allowed\>/read_mems_allowed_begin/g' -e > 's/\<put_mems_allowed\>/read_mems_allowed_retry/g' $file; done > > and a few edits later.. > > --- > include/linux/cpuset.h | 18 +++++++++--------- > kernel/cpuset.c | 2 +- > mm/filemap.c | 4 ++-- > mm/hugetlb.c | 4 ++-- > mm/mempolicy.c | 14 +++++++------- > mm/page_alloc.c | 8 ++++---- > mm/slab.c | 4 ++-- > mm/slub.c | 16 +++------------- > 8 files changed, 30 insertions(+), 40 deletions(-) > > diff --git a/include/linux/cpuset.h b/include/linux/cpuset.h > index 7a7e5fd..d008b03 100644 > --- a/include/linux/cpuset.h > +++ b/include/linux/cpuset.h > @@ -89,25 +89,25 @@ extern void rebuild_sched_domains(void); > extern void cpuset_print_task_mems_allowed(struct task_struct *p); > > /* > - * get_mems_allowed is required when making decisions involving mems_allowed > + * read_mems_allowed_begin is required when making decisions involving mems_allowed > * such as during page allocation. mems_allowed can be updated in parallel > * and depending on the new value an operation can fail potentially causing > - * process failure. A retry loop with get_mems_allowed and put_mems_allowed > + * process failure. A retry loop with read_mems_allowed_begin and read_mems_allowed_retry > * prevents these artificial failures. > */
Going over 80 columns there. This happens in other places in the patch but the alternative in those cases is less readable.
> -static inline unsigned int get_mems_allowed(void) > +static inline unsigned int read_mems_allowed_begin(void) > { > return read_seqcount_begin(¤t->mems_allowed_seq); > } > > /* > - * If this returns false, the operation that took place after get_mems_allowed > + * If this returns false, the operation that took place after read_mems_allowed_begin > * may have failed. It is up to the caller to retry the operation if > * appropriate. > */
80 cols again and it should be "returns true". Something like this?
/* * If this returns true, the operation that took place after * read_mems_allowed_begin may have failed artifically due to a paralle * update of mems_allowed. It is up to the caller to retry the operation * if appropriate. */
Other than that, the changes looked good and I agree that it is better overall.
Acked-by: Mel Gorman <mgorman@suse.de>
-- Mel Gorman SUSE Labs
| |