<div dir="ltr">Replying to Kirill's comment here, since I didn't get it in my inbox.<div><br></div><div><div>> This chooses service fd base for main criu process, usernsd process (possibly there are other helpers).</div><div>> I'm not sure, these guys never create many temporary fds before they install a service fd:</div><div>> </div><div>> criu main task:</div><div>> cached_fd1 = open()</div><div>> cached_fd2 = open()</div><div>> ...</div><div>> cached_fdn = open()</div><div>> </div><div>> install_service_fd() -> rewrites cached_fdn.</div><div>> </div><div>> It's need to review criu main task & friends code and check there are no such the situations.</div><div>> Also, if we are going to do this way, we will have to implement some things to detect such</div><div>> the problems in the future code.</div><div>> </div><div>> Really restored tasks (init and its children) do not create many temporary fd by design,</div><div>> and we check their fd leak by tests (in zdtm.py), so there is no such the problem.</div></div><div><br></div><div>Thanks, I wasn't aware of that. So the main CRIU process is effectively relying on a high</div><div>service_fd_base to avoid collisions with temporary fds, right?<br></div><div><br></div><div>In that case, we could still try to select a base by using a large enough pad.</div><div>In install_service_fd, we could then do:</div><div> BUG_ON(close(new_fd) == 0);</div><div>(such a safety check may be generally useful, regardless of my changes).</div><div><br></div><div>What do you think? Even with RLIMIT_NOFILE of few billions, the fork delay is just few hundred ms</div><div>- so if it turns out that my change gets very complex, it might not be worth it.</div></div><br><div class="gmail_quote"><div dir="ltr">On Fri, Aug 3, 2018 at 3:12 AM Andrei Vagin <<a href="mailto:avagin@virtuozzo.com" target="_blank">avagin@virtuozzo.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Kirill, could you review this patch?<br>
<br>
On Thu, Aug 02, 2018 at 02:53:10PM +0200, Radoslaw Burny wrote:<br>
> choose_service_fd_base selects lowest possible numbers for service fds<br>
> to keep fd table of the criu process small, making fork() faster.<br>
> However, this method was only used in child processes, not the root CRIU<br>
> process - thus forks from root were still slow. This fixes the issue.<br>
> <br>
> Sorry for slightly unreadable patch - the code move was necessary due to<br>
> changes in the inter-function dependencies. The key part of the diff is<br>
> this line:<br>
> service_fd_base = choose_service_fd_base(NULL);<br>
> <br>
> Signed-off-by: Radoslaw Burny <<a href="mailto:rburny@google.com" target="_blank">rburny@google.com</a>><br>
> ---<br>
> criu/util.c | 66 ++++++++++++++++++++++++++---------------------------<br>
> 1 file changed, 33 insertions(+), 33 deletions(-)<br>
> <br>
> diff --git a/criu/util.c b/criu/util.c<br>
> index ab4d89aa..114756a6 100644<br>
> --- a/criu/util.c<br>
> +++ b/criu/util.c<br>
> @@ -441,27 +441,6 @@ static int service_fd_base;<br>
> /* Id of current process in shared fdt */<br>
> static int service_fd_id = 0;<br>
> <br>
> -int init_service_fd(void)<br>
> -{<br>
> - struct rlimit64 rlimit;<br>
> -<br>
> - /*<br>
> - * Service FDs are those that most likely won't<br>
> - * conflict with any 'real-life' ones<br>
> - */<br>
> -<br>
> - if (syscall(__NR_prlimit64, getpid(), RLIMIT_NOFILE, NULL, &rlimit)) {<br>
> - pr_perror("Can't get rlimit");<br>
> - return -1;<br>
> - }<br>
> -<br>
> - service_fd_rlim_cur = (int)rlimit.rlim_cur;<br>
> - service_fd_base = service_fd_rlim_cur;<br>
> - BUG_ON(service_fd_base < SERVICE_FD_MAX);<br>
> -<br>
> - return 0;<br>
> -}<br>
> -<br>
> static int __get_service_fd(enum sfd_type type, int service_fd_id)<br>
> {<br>
> return service_fd_base - type - SERVICE_FD_MAX * service_fd_id;<br>
> @@ -560,20 +539,20 @@ static void move_service_fd(struct pstree_item *me, int type, int new_id, int ne<br>
> <br>
> static int choose_service_fd_base(struct pstree_item *me)<br>
> {<br>
> - int nr, real_nr, fdt_nr = 1, id = rsti(me)->service_fd_id;<br>
> + int nr = -1, real_nr, fdt_nr = 1, id = rsti(me)->service_fd_id;<br>
> <br>
> - if (rsti(me)->fdt) {<br>
> - /* The base is set by owner of fdt (id 0) */<br>
> - if (id != 0)<br>
> - return service_fd_base;<br>
> - fdt_nr = rsti(me)->fdt->nr;<br>
> + if (me != NULL) {<br>
> + if (rsti(me)->fdt) {<br>
> + /* The base is set by owner of fdt (id 0) */<br>
> + if (id != 0)<br>
> + return service_fd_base;<br>
> + fdt_nr = rsti(me)->fdt->nr;<br>
> + }<br>
> + /* Now find process's max used fd number */<br>
> + if (!list_empty(&rsti(me)->fds))<br>
> + nr = list_entry(rsti(me)->fds.prev,<br>
> + struct fdinfo_list_entry, ps_list)->fe->fd;<br>
> }<br>
> - /* Now find process's max used fd number */<br>
> - if (!list_empty(&rsti(me)->fds))<br>
> - nr = list_entry(rsti(me)->fds.prev,<br>
> - struct fdinfo_list_entry, ps_list)->fe->fd;<br>
> - else<br>
> - nr = -1;<br>
> <br>
> nr = max(nr, inh_fd_max);<br>
> /*<br>
> @@ -607,6 +586,27 @@ static int choose_service_fd_base(struct pstree_item *me)<br>
> return nr;<br>
> }<br>
> <br>
> +int init_service_fd(void)<br>
> +{<br>
> + struct rlimit64 rlimit;<br>
> +<br>
> + /*<br>
> + * Service FDs are those that most likely won't<br>
> + * conflict with any 'real-life' ones<br>
> + */<br>
> +<br>
> + if (syscall(__NR_prlimit64, getpid(), RLIMIT_NOFILE, NULL, &rlimit)) {<br>
> + pr_perror("Can't get rlimit");<br>
> + return -1;<br>
> + }<br>
> +<br>
> + service_fd_rlim_cur = (int)rlimit.rlim_cur;<br>
> + service_fd_base = choose_service_fd_base(NULL);<br>
> + BUG_ON(service_fd_base < SERVICE_FD_MAX);<br>
> +<br>
> + return 0;<br>
> +}<br>
> +<br>
> int clone_service_fd(struct pstree_item *me)<br>
> {<br>
> int id, new_base, i, ret = -1;<br>
> -- <br>
> 2.18.0.597.ga71716f1ad-goog<br>
> <br>
> _______________________________________________<br>
> CRIU mailing list<br>
> <a href="mailto:CRIU@openvz.org" target="_blank">CRIU@openvz.org</a><br>
> <a href="https://lists.openvz.org/mailman/listinfo/criu" rel="noreferrer" target="_blank">https://lists.openvz.org/mailman/listinfo/criu</a><br>
</blockquote></div>