Re: [Minios-devel] [UNIKRAFT PATCH 1/3] lib/vfscore: Use a scalable bitmap for open file descriptors

[Sharan Santhanam <sharan.santhanam@xxxxxxxxx>]

Hello Costin,

Please find the comment inline.

Thanks & Regards

Sharan

On 9/2/19 3:22 PM, Costin Lupu wrote:
> Until now, the number of open file descriptors was limited to 64, which was the
> number of bits of the underlying bitmap. We fix that by replacing with a
> configurable bitmap. We set the default value of maximum open files to 1024.
>
> Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
> ---
>   lib/vfscore/Config.uk |  8 +++++++-
>   lib/vfscore/fd.c      | 16 ++++++++--------
>   2 files changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/lib/vfscore/Config.uk b/lib/vfscore/Config.uk
> index 6cf6c63a..07319099 100644
> --- a/lib/vfscore/Config.uk
> +++ b/lib/vfscore/Config.uk
> @@ -1,6 +1,12 @@
> -config LIBVFSCORE
> +menuconfig LIBVFSCORE
>         bool "vfscore: VFS Core Interface"
>         default n
>         select LIBNOLIBC if !HAVE_LIBC
>         select LIBUKDEBUG
>         select LIBUKLOCK
> +
> +if LIBVFSCORE
> +config LIBVFSCORE_MAX_FILES
> +       int "Maximum number of open files"
> +       default 1024
> +endif
Do we want to define max file descriptor as configuration in Config.uk? 
From a user perspective it might not always be possible to determine 
this value before hand. We might add support for ulimit instead of keep 
is compile time configuration. I would rather we pick a default value 
for now and if a need arise we make it configurable then.
> diff --git a/lib/vfscore/fd.c b/lib/vfscore/fd.c
> index 01025a46..e09dd54c 100644
> --- a/lib/vfscore/fd.c
> +++ b/lib/vfscore/fd.c
> @@ -35,19 +35,19 @@
>   
>   #include <string.h>
>   #include <uk/essentials.h>
> -#include <uk/bitops.h>
> +#include <uk/bitmap.h>
>   #include <uk/assert.h>
>   #include <vfscore/file.h>
>   #include <uk/plat/lcpu.h>
>   #include <errno.h>
>   #include <uk/ctors.h>
>   
> -#define FDTABLE_MAX_FILES (sizeof(uint64_t) * 8)
> +#define FDTABLE_MAX_FILES 1024
Shouldn't it this be CONFIG_LIBVFSCORE_MAX_FILES?
>   
>   void init_stdio(void);
>   
>   struct fdtable {
> -       uint64_t bitmap;
> +       unsigned long bitmap[UK_BITS_TO_LONGS(FDTABLE_MAX_FILES)];
>         uint32_t fd_start;
>         struct vfscore_file *files[FDTABLE_MAX_FILES];
>   };
> @@ -59,14 +59,14 @@ int vfscore_alloc_fd(void)
>         int ret;
>   
>   	flags = ukplat_lcpu_save_irqf();
> -       ret = ukarch_ffsl(~fdtable.bitmap);
> +       ret = uk_find_next_zero_bit(fdtable.bitmap, FDTABLE_MAX_FILES, 0);
>   
>   	if (!ret) {
>                 ret = -ENFILE;
>                 goto exit;
>         }
>   
> -	fdtable.bitmap |= (uint64_t) 1 << ret;
> +       uk_bitmap_set(fdtable.bitmap, ret, 1);
>   
>   exit:
>         ukplat_lcpu_restore_irqf(flags);
> @@ -83,7 +83,7 @@ void vfscore_put_fd(int fd)
>         UK_ASSERT(fd > 2);
>   
>   	flags = ukplat_lcpu_save_irqf();
> -       __uk_clear_bit(fd, &fdtable.bitmap);\
> +       uk_bitmap_clear(fdtable.bitmap, fd, 1);
>         fp = fdtable.files[fd];
>         fdtable.files[fd] = NULL;
>         ukplat_lcpu_restore_irqf(flags);
> @@ -129,7 +129,7 @@ struct vfscore_file *vfscore_get_file(int fd)
>         UK_ASSERT(fd < (int) FDTABLE_MAX_FILES);
>   
>   	flags = ukplat_lcpu_save_irqf();
> -       if (!(fdtable.bitmap & ((uint64_t) 1 << fd)))
> +       if (!uk_test_bit(fd, fdtable.bitmap))
>                 goto exit;
>         ret = fdtable.files[fd];
>         fhold(ret);
> @@ -186,7 +186,7 @@ static void fdtable_init(void)
>         memset(&fdtable, 0, sizeof(fdtable));
>   
>   	/* reserve stdin, stdout and stderr */
> -       fdtable.bitmap = 7;
> +       uk_bitmap_set(fdtable.bitmap, 0, 3);
>         init_stdio();
>   }
>   
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel