Re: [PATCH v3] mm, memcg: fix error return value of mem_cgroup_css_alloc()

[Michal Hocko <mhocko@kernel.org>]

On Tue 07-04-20 17:31:33, Yafang Shao wrote:
> On Tue, Apr 7, 2020 at 2:43 PM Michal Hocko <mhocko@kernel.org> wrote:
> >
> > [I have only now noticed there was another version posted. Please try to
> > wait a bit longer for other feedback before reposting a newer version]
> >
> 
> Sure I will. sorry about that.
> But after we send a patch, we always don't know in which state this
> patch is, e.g. Changes Requested, Not Applicable, Rejected, Needs
> Review / ACK and etc, as listed in netdev list[1]. That could give us
> an explicit instruction on what to do next.

Yes I can see how that can be unclear or consuming at times. But a
general rule of thumb I would suggest is to simply wait few days before
reposting a new version. If you need to discuss an incremental change
based on the review feedback you can always do that in the original
email thread and repost the new version after the review feedback
settles.

> [1]. https://patchwork.ozlabs.org/project/netdev/list/?state=*&page=2&param=3
> 
> > On Tue 07-04-20 11:11:13, Yafang Shao wrote:
[...]
> > See my comment about EBUSY in the previous version
> > http://lkml.kernel.org/r/20200407063621.GA18914@dhcp22.suse.cz
> >
> 
> >From the perspective of mkdir(2), it is better to use ENOSPC here. But
> I'm not sure whether it is better to update the man page of mkdir(2)
> or not.

Well, I do not know whether EBUSY missing in the man page is an omission
or an intention. But adding it only for this single operation failure
would sound like an overkill to me.

> I checked the explaination about ENOSPC in 73f576c04b94 ("mm:
> memcontrol: fix cgroup creation failure after many small jobs")
> carefully, but I don't a clear idea which one is better now.

The changelog simply mentioned that without the additional id tracking
the ENOSPC would have been returned from elsewhere. I haven't checked
but I suspect it would be from the cgroup core. This patch just didn't
propagate the idr failure specifically and hid it under the ENOMEM
failure path. I can only speculate why that was the case but I suspect
that Johannes simply didn't consider the distinction important enough.
All I am saying is that preserving the failure mode would be preferable.
Especially unless there a strong reason to use EBUSY which then should
be carefully explained in the changelog.

-- 
Michal Hocko
SUSE Labs