Re: [PATCH v2 1/2] mm: Export nr_swap_pages

Michal Hocko
2015-12-08T11:22:27Z

On Mon 07-12-15 14:13:46, Johannes Weiner wrote:
> On Mon, Dec 07, 2015 at 06:10:00PM +0000, Dave Gordon wrote:
> > Exporting random uncontrolled variables from the kernel to loaded modules is
> > not really considered best practice. It would be preferable to provide an
> > accessor function - which is just what the declaration says we have; the
> > implementation as a static inline (and/or macro) is what causes the problem
> > here.
>
> No, what causes the problem is thinking we can't trust in-kernel code.

This is not about the trust. It is about a clear API and separation.

> If somebody screws up, we can fix it easily enough. Sure, we shouldn't
> be laying traps and create easy-to-misuse interfaces, but that's not
> what's happening here. There is no reason to add function overhead to
> what should be a single 'mov' instruction.

The mere fact that the current implementation is a simple atomic_long_read
is a detail and not important for the API. The function is not used
in any hot path where a single function call overhead would be a
performance killer. Exporting implementation details to random users
tends to add maintenance burden in future.

I think it is natural to export symbols which are consumed by modules
and that will be get_nr_swap_pages(). I do not even understand the
resistance against that. Anyway I am not going to argue about it more.
I have raised my review comment and leave the decision to Chris/Andrew.
--
Michal Hocko
SUSE Labs