Messages in this thread |  | | Date | Sun, 15 Jan 2023 00:52:26 +0800 | Subject | Re: [External] Re: [PATCH 0/3] mm: replace atomic_t with percpu_ref in mempolicy. | From | Zhongkun He <> |
| |
> On Mon 05-12-22 00:14:29, Zhongkun He wrote: > [...] >> +/* Obtain a reference on the specified mpol */ >> static inline void mpol_get(struct mempolicy *pol) >> { >> if (pol) > > Shouldn't this be mpol_needs_cond_ref? > >> - atomic_inc(&pol->refcnt); >> + percpu_ref_get(&pol->refcnt); >> +} >> + >> +static inline bool mpol_tryget(struct mempolicy *pol) >> +{ >> + return pol && percpu_ref_tryget(&pol->refcnt); >> } >> >> +/* >> + * This function initiates destruction of mempolicy. > > This is not a useful comment. It would be much more helpful to say when > this should be called. > >> + */ >> +static inline void mpol_kill(struct mempolicy *pol) >> +{ >> + if (pol) >> + percpu_ref_kill(&pol->refcnt); >> +} >> + >> + >> extern bool __mpol_equal(struct mempolicy *a, struct mempolicy *b); >> static inline bool mpol_equal(struct mempolicy *a, struct mempolicy *b) >> { >> @@ -197,11 +210,15 @@ static inline void mpol_put(struct mempolicy *p) >> { >> } >> >> -static inline void mpol_cond_put(struct mempolicy *pol) >> +static inline void mpol_get(struct mempolicy *pol) >> { >> } >> >> -static inline void mpol_get(struct mempolicy *pol) >> +static inline bool mpol_tryget(struct mempolicy *pol) >> +{ >> +} > > This should return false, right? > > [...] >> +/* Obtain a reference on the specified task mempolicy */ > > Again, this is pretty much clear from the name. It would be more useful > to explain how the pointer can be used - e.g. needs to call mpol_put > or mpol_kill depending on the calling context. > >> +static mempolicy *get_task_mpol(struct task_struct *p) >> +{ >> + struct mempolicy *pol; >> + >> + rcu_read_lock(); >> + pol = rcu_dereference(p->mempolicy); >> + >> + if (!pol || mpol_tryget(pol)) > > Shouldn't be !mpol_tryget? > >> + pol = NULL; >> + rcu_read_unlock(); >> + >> + return pol; >> +} >> + > > I do not see any rcu_assign_pointer for the newly created policy so this > seems incomplete. Ditto no mpol_kill calls. I am unlikely to get into > follow up patches now. Please split up the work so that it is reviewable > more easily and then I can have a further look. > > Thanks!
Thanks for your review, some changes may be in other patch,i will reorganize the patches according to the suggestions to make things clear.
Thanks.
|  |