Re: [RFC PATCH v2 2/5] mm: Add policy_name to identify OOM policies

[Chuyi Zhou]

Hello,

在 2023/8/15 04:51, Jonathan Corbet 写道:
> Chuyi Zhou <zhouchuyi@xxxxxxxxxxxxx> writes:
>
> > This patch adds a new metadata policy_name in oom_control and report it
> > in dump_header(), so we can know what has been the selection policy. In
> > BPF program, we can call kfunc set_oom_policy_name to set the current
> > user-defined policy name. The in-kernel policy_name is "default".
> >
> > Signed-off-by: Chuyi Zhou <zhouchuyi@xxxxxxxxxxxxx>
> > ---
> >   include/linux/oom.h |  7 +++++++
> >   mm/oom_kill.c       | 42 +++++++++++++++++++++++++++++++++++++++---
> >   2 files changed, 46 insertions(+), 3 deletions(-)
>
> So I have a possibly dumb question here...
>
> > diff --git a/include/linux/oom.h b/include/linux/oom.h
> > index 7d0c9c48a0c5..69d0f2ec6ea6 100644
> > --- a/include/linux/oom.h
> > +++ b/include/linux/oom.h
> > @@ -22,6 +22,10 @@ enum oom_constraint {
> >   	CONSTRAINT_MEMCG,
> >   };
> >   
> > +enum {
> > +	POLICY_NAME_LEN = 16,
> > +};
>
> We've defined our name length, fine...
>
> >   /*
> >    * Details of the page allocation that triggered the oom killer that are used to
> >    * determine what should be killed.
> > @@ -52,6 +56,9 @@ struct oom_control {
> >   
> >   	/* Used to print the constraint info. */
> >   	enum oom_constraint constraint;
> > +
> > +	/* Used to report the policy info. */
> > +	char policy_name[POLICY_NAME_LEN];
> >   };
>
> ...that is the length of the array, appended to the structure...
>
> >   extern struct mutex oom_lock;
> > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > index 255c9ef1d808..3239dcdba4d7 100644
> > --- a/mm/oom_kill.c
> > +++ b/mm/oom_kill.c
> > @@ -443,6 +443,35 @@ static int dump_task(struct task_struct *p, void *arg)
> >   	return 0;
> >   }
> >   
> > +__bpf_kfunc void set_oom_policy_name(struct oom_control *oc, const char *src, size_t sz)
> > +{
> > +	memset(oc->policy_name, 0, sizeof(oc->policy_name));
> > +
> > +	if (sz > POLICY_NAME_LEN)
> > +		sz = POLICY_NAME_LEN;
> > +
> > +	memcpy(oc->policy_name, src, sz);
> > +}
>
> This truncates the name, possibly leaving it without a terminating NUL
> character, right?

Yes, indeed. We should guarantee that the policy_name is terminated with 
a NULL character to avoid some undefined behavior.

Thanks for your helpful review.

------
Chuyi Zhou