* A field in files_struct has been used without initialization
@ 2022-10-06 10:44 Abd-Alrhman Masalkhi
2022-10-06 10:57 ` Abd-Alrhman Masalkhi
0 siblings, 1 reply; 5+ messages in thread
From: Abd-Alrhman Masalkhi @ 2022-10-06 10:44 UTC (permalink / raw)
To: linux-kernel; +Cc: linux-fsdevel
Hello Linux community,
I have came acrose the following code in dup_fd()
1 newf = kmem_cache_alloc(files_cachep, GFP_KERNEL);
2 if (!newf)
3 goto out;
4
5 atomic_set(&newf->count, 1);
6
7 spin_lock_init(&newf->file_lock);
8 newf->resize_in_progress = false;
9 init_waitqueue_head(&newf->resize_wait);
10 newf->next_fd = 0;
11 new_fdt = &newf->fdtab;
12 new_fdt->max_fds = NR_OPEN_DEFAULT;
13 new_fdt->close_on_exec = newf->close_on_exec_init;
On line 13 new_fdt->close_on_exec has given the value of
newf->close_on_exec_init, but new_fdt->close_on_exec itself has not
been initialized, is it intended to be like this.
Thanky you very much!
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: A field in files_struct has been used without initialization
2022-10-06 10:44 A field in files_struct has been used without initialization Abd-Alrhman Masalkhi
@ 2022-10-06 10:57 ` Abd-Alrhman Masalkhi
2022-10-07 19:16 ` Al Viro
0 siblings, 1 reply; 5+ messages in thread
From: Abd-Alrhman Masalkhi @ 2022-10-06 10:57 UTC (permalink / raw)
To: linux-kernel; +Cc: linux-fsdevel
> new_fdt->close_on_exec itself has not been initialized, is it intended
> to be like this.
I meant:
newf->close_on_exec_init itself has not been initialized ...
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: A field in files_struct has been used without initialization
2022-10-06 10:57 ` Abd-Alrhman Masalkhi
@ 2022-10-07 19:16 ` Al Viro
2022-10-09 9:33 ` Abd-Alrhman Masalkhi
2022-10-09 18:21 ` Al Viro
0 siblings, 2 replies; 5+ messages in thread
From: Al Viro @ 2022-10-07 19:16 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi; +Cc: linux-kernel, linux-fsdevel
On Thu, Oct 06, 2022 at 12:57:28PM +0200, Abd-Alrhman Masalkhi wrote:
> > new_fdt->close_on_exec itself has not been initialized, is it intended
> > to be like this.
>
> I meant:
>
> newf->close_on_exec_init itself has not been initialized ...
Huh? close_on_exec_init is an array, and this assignment stores the address
of its first (and only) element into newf->fdtab.close_on_exec. So it's
basically
newf->fdtab.close_on_exec = &newf->close_on_exec_init[0];
->fdtab and ->close_on_exec_init are to be used only if we need no more than
BITS_PER_LONG descriptors. It's common enough to make avoiding a separate
allocation (and separate cacheline on following the pointer chain) worth
the trouble.
Note that we do not use newf->fdtab directly - we use newf->fdt.
What happens here is
new_fdt = &newf->fdtab;
...
set newf->fdtab contents for the case we need few descriptors
if we need more
allocate a separate struct fdtable, bitmaps, etc.
set the contents of that separate fdtable
new_fdt = that new fdtable
copy bitmaps into whatever new_fdt->close_on_exec and
new_fdt->open_fds are pointing at.
copy file pointers into whatever new_fdt->fd[] points at.
set newf->fdt to new_fdt.
The value of newf->close_on_exec_init is simply newf + known constant.
It needs no initialization at all. The *contents* of the array
it points to is used only if new_fdt remains pointing to newf->fdtab;
in that case it's initialized by
copy_fd_bitmaps(new_fdt, old_fdt, open_files);
IOW, for few-descriptors case we end up with
newf:
fdt points to newf->fdtab
fdtab.close_on_exec points to newf->close_on_exec_init[0]
fdtab.full_fd_bits points to newf->full_fd_bits_init[0]
fdtab.open_fds points to newf->open_fds_init[0]
fdtab.fd points to newf->fd_array[0]
fdtab.max_fds max capacity; will be BITS_PER_LONG
close_on_exec_init[0] hosts the close_on_exec bitmap
full_fd_bits_init[0] hosts the full_fd_bits bitmap
open_fds_init[0] hosts the open_fds bitmap
fd_array[] contains file pointers (BITS_PER_LONG of them)
For more-than-a-few-descriptors case we have
array of pointers (from first kvmalloc() in alloc_fdtable()): contains file pointers
array of unsigned long (from the second kmvalloc() there): hosts the bitmaps
fdtable (allocated in alloc_fdtable()):
open_fds points to the beginning of bitmap-hosting array
close_on_exec points to the middle third of the same array
full_fd_bits points to the last third of the same array
fd points to array of struct file pointers.
max_fds max capacity, matches the sizes of arrays.
newf:
fdt points to fdtable
fdtab, ..._init and fd_array unused
Might be useful to take a piece of paper and draw the picture,
I really don't want to bother with ASCII graphics for that...
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: A field in files_struct has been used without initialization
2022-10-07 19:16 ` Al Viro
@ 2022-10-09 9:33 ` Abd-Alrhman Masalkhi
2022-10-09 18:21 ` Al Viro
1 sibling, 0 replies; 5+ messages in thread
From: Abd-Alrhman Masalkhi @ 2022-10-09 9:33 UTC (permalink / raw)
To: viro; +Cc: linux-kernel, linux-fsdevel
Hello Al Viro
> close_on_exec_init is an array, and this assignment stores the address
> of its first (and only) element into newf->fdtab.close_on_exec. So it's
> basically
> newf->fdtab.close_on_exec = &newf->close_on_exec_init[0];
>
> ->fdtab and ->close_on_exec_init are to be used only if we need no more than
> BITS_PER_LONG descriptors. It's common enough to make avoiding a separate
> allocation (and separate cacheline on following the pointer chain) worth
> the trouble
> ...
> ...
Fascinating, thank you for this very informative response. I have learned a
lot from it.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: A field in files_struct has been used without initialization
2022-10-07 19:16 ` Al Viro
2022-10-09 9:33 ` Abd-Alrhman Masalkhi
@ 2022-10-09 18:21 ` Al Viro
1 sibling, 0 replies; 5+ messages in thread
From: Al Viro @ 2022-10-09 18:21 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi; +Cc: linux-kernel, linux-fsdevel
On Fri, Oct 07, 2022 at 08:16:11PM +0100, Al Viro wrote:
> array of pointers (from first kvmalloc() in alloc_fdtable()): contains file pointers
> array of unsigned long (from the second kmvalloc() there): hosts the bitmaps
> fdtable (allocated in alloc_fdtable()):
> open_fds points to the beginning of bitmap-hosting array
> close_on_exec points to the middle third of the same array
> full_fd_bits points to the last third of the same array
Rereading that... a bit of clarification: the wording would seem to imply
that these 3 bitmaps are of equal size; they are not - the third one is
smaller.
If fdt->max_fds (table capacity) is equal to N, we have
* N struct file pointers in fdt->fd[]
* N bits in fdt->open_fds[] (i.e. N/BITS_PER_LONG unsigned long)
* N bits in fdt->close_on_exec[] (ditto)
* N/BITS_PER_LONG bits in fdt->full_fd_bits[]
The meaning of bitmaps:
bit k set in open_fds[] - descriptor k is in use
bit k set in close_on_exec[] - descriptor k is to be closed on exec()
bit k set in full_fd_bits[] - all descriptors covered by open_fds[k]
(i.e. BITS_PER_LONG of them,, starting from k * BITS_PER_LONG) are in use.
In other words, "don't even bother looking for clear bits in open_fds[k];
there won't be any".
->full_fd_bits[] is there purely as a way to speed finding unused descriptors,
along with ->next_fd. Some processes have an obscene amount of opened
descriptors and linear search through the mostly full bitmap can get painful.
E.g. 4 millions of descriptors would mean half megabyte worth of bitmap
(in ->open_fds[]). Cheaper to search through 8Kb of ->full_fd_bits, then
check one 64bit word in ->open_fds[]; kinder on the cache as well...
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-10-09 18:21 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-06 10:44 A field in files_struct has been used without initialization Abd-Alrhman Masalkhi
2022-10-06 10:57 ` Abd-Alrhman Masalkhi
2022-10-07 19:16 ` Al Viro
2022-10-09 9:33 ` Abd-Alrhman Masalkhi
2022-10-09 18:21 ` Al Viro
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).