On Sat, Aug 03, 2024 at 04:51:44PM -0700, Linus Torvalds wrote:
> I think the code could (and should) do just
>
> cpy = count / BITS_PER_LONG;
> max = nfdt->max_fds / BITS_PER_LONG;
>
> bitmap_copy(nfdt->full_fds_bits, ofdt->full_fds_bits, cpy);
> bitmap_clear(nfdt->full_fds_bits, count, max - cpy);
>
> or something like that.
>
> Doesn't that seem more straightforward?
Considered that; unfortunately, bitmap_clear() is... suboptimal,
to put it mildly. And on the expand_fdtable() pathway (as well
as normal fork()) we might have to clear quite a bit.
> Of course, it's also entirely possible that I have just confused
> myself, and the above is buggy garbage. I tried to think this through,
> but I really tried to think more along the lines of "how can we make
> this legible so that the code is actually understandable". Because I
> think your patch almost certainly fixes the bug, but it sure isn't
> easy to understand.
Bitmap handling in there certainly needs to be cleaned up; no arguments
here. If anything, we might want something like
bitmap_copy_and_extend(unsigned long *to, const unsigned long *from,
unsigned int count, unsigned int size)
{
unsigned int copy = BITS_TO_LONGS(count);
memcpy(to, from, copy * sizeof(long));
if (count % BITS_PER_LONG)
to[copy - 1] &= BITMAP_LAST_WORD_MASK(count);
memset(to + copy, 0, bitmap_size(size) - copy * sizeof(long));
}
and use it for all of three bitmaps. Compiler should be able to spot
CSEs here - let it decide if keeping them is cheaper than reevaluating...
Would you hate
static void copy_fd_bitmaps(struct fdtable *nfdt, struct fdtable *ofdt,
unsigned int count, unsigned int size)
{
bitmap_copy_and_extend(nfdt->open_fds, ofdt->open_fds,
count, size);
bitmap_copy_and_extend(nfdt->close_on_exec, ofdt->close_on_exec,
count, size);
bitmap_copy_and_extend(nfdt->full_fds_bits, ofdt->full_fds_bits,
count / BITS_PER_LONG, size / BITS_PER_LONG);
}
with callers passing nfdt->size as the 4th argument?
Said that, the posted variant might be better wrt backporting - it has
no overhead on the common codepaths; something like the variant above
(and I think that use of bitmap_clear() will be worse) would need
profiling. So it might make sense to use that for backports, with
cleanups done on top of it. Up to you...
FWIW, I'm crawling through fs/file.c at the moment (in part - to replace
Documentation/filesystems/files.rst with something that wouldn't be
years out of date), and there's a growing patch series around that
area. This thing got caught while trying to document copy_fd_bitmaps()
and convince myself that it works correctly...
