[CRIU] [PATCH 1/1] Use choose_service_fd_base in the root process.

[Kirill Tkhai]

Hi, Radoslaw,

On 02.08.2018 15:53, Radoslaw Burny wrote:
> choose_service_fd_base selects lowest possible numbers for service fds
> to keep fd table of the criu process small, making fork() faster.
> However, this method was only used in child processes, not the root CRIU
> process - thus forks from root were still slow. This fixes the issue.
> 
> Sorry for slightly unreadable patch - the code move was necessary due to
> changes in the inter-function dependencies. The key part of the diff is
> this line:
>   service_fd_base = choose_service_fd_base(NULL);
> 
> Signed-off-by: Radoslaw Burny <rburny at google.com>
> ---
>  criu/util.c | 66 ++++++++++++++++++++++++++---------------------------
>  1 file changed, 33 insertions(+), 33 deletions(-)
> 
> diff --git a/criu/util.c b/criu/util.c
> index ab4d89aa..114756a6 100644
> --- a/criu/util.c
> +++ b/criu/util.c
> @@ -441,27 +441,6 @@ static int service_fd_base;
>  /* Id of current process in shared fdt */
>  static int service_fd_id = 0;
>  
> -int init_service_fd(void)
> -{
> -	struct rlimit64 rlimit;
> -
> -	/*
> -	 * Service FDs are those that most likely won't
> -	 * conflict with any 'real-life' ones
> -	 */
> -
> -	if (syscall(__NR_prlimit64, getpid(), RLIMIT_NOFILE, NULL, &rlimit)) {
> -		pr_perror("Can't get rlimit");
> -		return -1;
> -	}
> -
> -	service_fd_rlim_cur = (int)rlimit.rlim_cur;
> -	service_fd_base = service_fd_rlim_cur;
> -	BUG_ON(service_fd_base < SERVICE_FD_MAX);
> -
> -	return 0;
> -}
> -
>  static int __get_service_fd(enum sfd_type type, int service_fd_id)
>  {
>  	return service_fd_base - type - SERVICE_FD_MAX * service_fd_id;
> @@ -560,20 +539,20 @@ static void move_service_fd(struct pstree_item *me, int type, int new_id, int ne
>  
>  static int choose_service_fd_base(struct pstree_item *me)
>  {
> -	int nr, real_nr, fdt_nr = 1, id = rsti(me)->service_fd_id;
> +	int nr = -1, real_nr, fdt_nr = 1, id = rsti(me)->service_fd_id;
>  
> -	if (rsti(me)->fdt) {
> -		/* The base is set by owner of fdt (id 0) */
> -		if (id != 0)
> -			return service_fd_base;
> -		fdt_nr = rsti(me)->fdt->nr;
> +	if (me != NULL) {
> +		if (rsti(me)->fdt) {
> +			/* The base is set by owner of fdt (id 0) */
> +			if (id != 0)
> +				return service_fd_base;
> +			fdt_nr = rsti(me)->fdt->nr;
> +		}
> +		/* Now find process's max used fd number */
> +		if (!list_empty(&rsti(me)->fds))
> +			nr = list_entry(rsti(me)->fds.prev,
> +					struct fdinfo_list_entry, ps_list)->fe->fd;
>  	}
> -	/* Now find process's max used fd number */
> -	if (!list_empty(&rsti(me)->fds))
> -		nr = list_entry(rsti(me)->fds.prev,
> -				struct fdinfo_list_entry, ps_list)->fe->fd;
> -	else
> -		nr = -1;
>  
>  	nr = max(nr, inh_fd_max);
>  	/*
> @@ -607,6 +586,27 @@ static int choose_service_fd_base(struct pstree_item *me)
>  	return nr;
>  }
>  
> +int init_service_fd(void)
> +{
> +	struct rlimit64 rlimit;
> +
> +	/*
> +	 * Service FDs are those that most likely won't
> +	 * conflict with any 'real-life' ones
> +	 */
> +
> +	if (syscall(__NR_prlimit64, getpid(), RLIMIT_NOFILE, NULL, &rlimit)) {
> +		pr_perror("Can't get rlimit");
> +		return -1;
> +	}
> +
> +	service_fd_rlim_cur = (int)rlimit.rlim_cur;
> +	service_fd_base = choose_service_fd_base(NULL);

This chooses service fd base for main criu process, usernsd process (possibly there are other helpers).
I'm not sure, these guys never create many temporary fds before they install a service fd:

criu main task:
   cached_fd1 = open()
   cached_fd2 = open()
   ...
   cached_fdn = open()

   install_service_fd() -> rewrites cached_fdn.

It's need to review criu main task & friends code and check there are no such the situations.
Also, if we are going to do this way, we will have to implement some things to detect such
the problems in the future code.

Really restored tasks (init and its children) do not create many temporary fd by design,
and we check their fd leak by tests (in zdtm.py), so there is no such the problem.

> +	BUG_ON(service_fd_base < SERVICE_FD_MAX);
> +
> +	return 0;
> +}
> +
>  int clone_service_fd(struct pstree_item *me)
>  {
>  	int id, new_base, i, ret = -1;

Kirill