Re: [PATCH V2] Switch from select() to poll() in xenconsoled's IO loop.

[Mats Petersson <mats.petersson@citrix.com>]

On 04/01/13 16:38, Wei Liu wrote:
> On Fri, 2013-01-04 at 16:08 +0000, Ian Campbell wrote:
>>> +static int initialize_pollfd_arrays(void)
>>> +{
>>> +       fds = (struct pollfd *)
>>> +               malloc(sizeof(struct pollfd) * DEFAULT_ARRAY_SIZE);
>>> +       if (!fds)
>>> +               goto fail;
>>> +       fd_to_pollfd = (struct pollfd **)
>>> +               malloc(sizeof(struct pollfd *) * DEFAULT_ARRAY_SIZE);
I believe it's considered "bad" to cast the result of malloc... Unless 
you are using a C++ compiler to compile C - in which case it's 
necessary. But then it should really be converted to "new" and "delete" 
where relevant. [Although that does cause problems with realloc!]

I do notice you are not casting realloc, so I expect it's safe to remove 
the casts here too.

Of course, if you remove these calls to malloc and just use realloc in 
the first place, you can ignore this comment! ;)
>>> +       if (!fd_to_pollfd)
>>> +               goto fail;
>>> +       memset(fds, 0, sizeof(struct pollfd) * DEFAULT_ARRAY_SIZE);
>>> +       memset(fd_to_pollfd, 0, sizeof(struct pollfd *) * DEFAULT_ARRAY_SIZE);
>>> +       current_array_size = DEFAULT_ARRAY_SIZE;
>>> +       return 0;
>>> +fail:
>>> +       free(fds);
>>> +       free(fd_to_pollfd);
>>> +       return -ENOMEM;
>>> +}
>>> +
>>> +static void destroy_pollfd_arrays(void)
>>> +{
>>> +       free(fds);
>>> +       free(fd_to_pollfd);
>>> +       current_array_size = 0;
>>> +}
>>> +
>>> +static void set_fds(int fd, short events)
>>> +{
>>> +       if (current_array_size < fd+1) {
>>> +               struct pollfd  *p1 = NULL;
>>> +               struct pollfd **p2 = NULL;
>>> +               unsigned int newsize = current_array_size;
>>> +
>>> +               do { newsize += GROWTH_LENGTH; } while (newsize < fd+1);
>> Steal #define ROUNDUP from tools/libxc/xc_linux_osdep.c.
>>> +
>>> +               p1 = realloc(fds, sizeof(struct pollfd)*newsize);
>>> +               if (!p1)
>>> +                       goto fail;
>>> +               fds = p1;
>>> +
>>> +               p2 = realloc(fd_to_pollfd, sizeof(struct pollfd *)*newsize);
>> realloc(NULL, ...) is the same as malloc() so I think you can initialise
>> current_array_size to 0 and the various pointers to NULL and avoid the
>> need for initialize_pollfd_arrays -- i.e. it will just grow from 0 on
>> the first use here.
>>
> Huh, good idea.
>
>>>          for (;;) {
>>>                  struct domain *d, *n;
>>> -               int max_fd = -1;
>>> -               struct timeval timeout;
>>> +               int poll_timeout; /* timeout in milliseconds */
>>>                  struct timespec ts;
>>>                  long long now, next_timeout = 0;
>>>
>>> -               FD_ZERO(&readfds);
>>> -               FD_ZERO(&writefds);
>>> +               reset_fds();
>>>
>>> -               FD_SET(xs_fileno(xs), &readfds);
>>> -               max_fd = MAX(xs_fileno(xs), max_fd);
>>> +               set_fds(xs_fileno(xs), POLLIN);
>> Do you know, or can you track, the maximum fd over time?
>> If you can then you could likely make use of automatic stack allocations
>> (struct pollfd fds[max_fd]) and therefore avoid the pain of manual
>> memory management.
> Stack size is subject to system setting. Typically it is limited to 8MB
> in Linux as shown by `ulimit -s`. This is actually very small compared
> to heap space.
Not to mention that if you run out of heapspace, you can "do something 
about it" (even if it's just print an error message and exit, it's 
better than "Stack overflow crash with no message at all").

I prefer allocations for that reason.

On the other hand, the number of fds we can fit in 8MB (or 4MB) is 
probably more than we'd ever get from running many guests with 
consoles.... And it wouldn't be hard to add a "set stack size to 16MB" 
or something like that as part of "start xenconsoled".

--
Mats
>
> Of course we can make xenconsoled special among all the processes... But
> that's not ideal.
>
>
> Wei.
>
>> Not sure what the semantics of those are inside a for loop where max_fd
>> can change but worst case you could put the content of the loop into a
>> function.
>>
>> Ian.
>>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
>