Hi,
A simple experiment:
a) insert a code like this in the open() routine of some driver:
struct files_struct *files = current->files;
extern int count_open_files(struct files_struct *, int);
printk(KERN_ERR "%s has %d files open\n",
current->comm, count_open_files(files, files->max_fdset));
b) write a program that opens that device and sleeps indefinitely. Run it,
you will see:
op has 32 files open
c) # lsof -p 659 | nl
1 COMMAND PID USER FD TYPE DEVICE SIZE NODE NAME
2 op 659 tigran cwd DIR 3,6 4096 225730
/home/tigran/C/files
3 op 659 tigran rtd DIR 3,6 4096 2 /
4 op 659 tigran txt REG 3,6 14142 225732
/home/tigran/C/files/op
5 op 659 tigran mem REG 3,6 434945 295227
/lib/ld-2.1.92.so
6 op 659 tigran mem REG 3,6 4776568 295234
/lib/libc-2.1.92.so
7 op 659 tigran 0u CHR 4,9 348027 /dev/tty9
8 op 659 tigran 1u CHR 4,9 348027 /dev/tty9
9 op 659 tigran 2u CHR 4,9 348027 /dev/tty9
10 op 659 tigran 3r CHR 10,184 98313
/dev/cpu/microcode
so, we see that the process has only 9 files open and yet
count_open_files() claims there are 32. Is this a bug of
count_open_files() or is this a minimal granularity (because there are 32
builtin descriptors in files_struct->fd_array[])?
I know that there is no problem due to the way it is called in
copy_files() -- it would only be above 32. But for what I want to use it,
I need the _correct_ number of open file descriptors and not some "rounded
up to 32" one.
(kernel assumed test12-pre2)
Regards,
Tigran
Date: Tue, 28 Nov 2000 15:13:44 +0000 (GMT)
From: Tigran Aivazian <[email protected]>
I know that there is no problem due to the way it is called in
copy_files() -- it would only be above 32. But for what I want to
use it, I need the _correct_ number of open file descriptors and
not some "rounded up to 32" one.
That function is static for a reason :-) It is not meant
for external use. What it gives you is the smallest multiple
of (8 * sizeof(long)) which is larger than the largest file
descriptor the task has open.
What you want is something like:
static int num_open_files(struct files_struct *files, int size)
{
total = 0;
for (i = size / (8 * sizeof(long)); i > 0; )
total += count_bits(files->open_fds->fds_bits[--i]);
return total;
}
Later,
David S. Miller
[email protected]
On Tue, 28 Nov 2000, David S. Miller wrote:
> What you want is something like:
>
> static int num_open_files(struct files_struct *files, int size)
> {
> total = 0;
> for (i = size / (8 * sizeof(long)); i > 0; )
> total += count_bits(files->open_fds->fds_bits[--i]);
>
> return total;
> }
Ok, since we have to walk the sets and test the bits anyway, I propose to
make close_files() to return 'nr_open_fds' and accept an extra argument
'doclose=0 or 1' which will specify whether we want to close the 'fd' or
whether we just want to count them.
Regards,
Tigran
On Tue, 28 Nov 2000, Tigran Aivazian wrote:
> Hi,
>
> A simple experiment:
>
> a) insert a code like this in the open() routine of some driver:
>
> struct files_struct *files = current->files;
> extern int count_open_files(struct files_struct *, int);
>
> printk(KERN_ERR "%s has %d files open\n",
> current->comm, count_open_files(files, files->max_fdset));
>
> b) write a program that opens that device and sleeps indefinitely. Run it,
> you will see:
>
> op has 32 files open
>
> c) # lsof -p 659 | nl
> 1 COMMAND PID USER FD TYPE DEVICE SIZE NODE NAME
> 2 op 659 tigran cwd DIR 3,6 4096 225730
> /home/tigran/C/files
> 3 op 659 tigran rtd DIR 3,6 4096 2 /
> 4 op 659 tigran txt REG 3,6 14142 225732
> /home/tigran/C/files/op
> 5 op 659 tigran mem REG 3,6 434945 295227
> /lib/ld-2.1.92.so
> 6 op 659 tigran mem REG 3,6 4776568 295234
> /lib/libc-2.1.92.so
> 7 op 659 tigran 0u CHR 4,9 348027 /dev/tty9
> 8 op 659 tigran 1u CHR 4,9 348027 /dev/tty9
> 9 op 659 tigran 2u CHR 4,9 348027 /dev/tty9
> 10 op 659 tigran 3r CHR 10,184 98313
> /dev/cpu/microcode
>
> so, we see that the process has only 9 files open and yet
> count_open_files() claims there are 32. Is this a bug of
> count_open_files() or is this a minimal granularity (because there are 32
> builtin descriptors in files_struct->fd_array[])?
>
> I know that there is no problem due to the way it is called in
> copy_files() -- it would only be above 32. But for what I want to use it,
> I need the _correct_ number of open file descriptors and not some "rounded
> up to 32" one.
>
> (kernel assumed test12-pre2)
>
> Regards,
> Tigran
>
Yes. This is probably related to the previously-reported MOD_INC_USE_COUNT
macro in the drivers. The open count is now handled in the call to
the driver's open(). The MOD_INC_USE_COUNT and the MOD_DEC_USE_COUNT
should now be defined to do nothing for kernels that already handle
the counts in the calls.
Cheers,
Dick Johnson
Penguin : Linux version 2.4.0 on an i686 machine (799.54 BogoMips).
"Memory is like gasoline. You use it up when you are running. Of
course you get it all back when you reboot..."; Actual explanation
obtained from the Micro$oft help desk.
On Tue, 28 Nov 2000, David S. Miller wrote:
> Date: Tue, 28 Nov 2000 15:13:44 +0000 (GMT)
> From: Tigran Aivazian <[email protected]>
>
> I know that there is no problem due to the way it is called in
> copy_files() -- it would only be above 32. But for what I want to
> use it, I need the _correct_ number of open file descriptors and
> not some "rounded up to 32" one.
>
> That function is static for a reason :-) It is not meant
> for external use. What it gives you is the smallest multiple
> of (8 * sizeof(long)) which is larger than the largest file
> descriptor the task has open.
>
> What you want is something like:
>
> static int num_open_files(struct files_struct *files, int size)
> {
> total = 0;
> for (i = size / (8 * sizeof(long)); i > 0; )
> total += count_bits(files->open_fds->fds_bits[--i]);
>
> return total;
> }
And you don't want even that - as soon as we release files->file_lock
the value becomes meaningless. So I'm rather suspicious about the
correctness of intended use.
On Tue, 28 Nov 2000, Alexander Viro wrote:
>
> And you don't want even that - as soon as we release files->file_lock
> the value becomes meaningless. So I'm rather suspicious about the
> correctness of intended use.
>
Hi Alexander,
I am well aware of the need to take the file_lock. So, I am putting a code
like this into set_user():
/* switch the open fds from old_user to new_user */
read_lock(&files->file_lock);
nr_open = close_files(files, 0); /* 0 means don't close them */
atomic_sub(nr_open, &old_user->files);
atomic_add(nr_open, &new_user->files);
read_unlock(&files->file_lock);
(haven't tested yet -- just an idea -- kernel is still compiling as we
speak). This assumes the close_files() semantics I described in the
previous email. I also put the corresponding code into
get/__put_unused_fd() and also into put_files_struct(). Btw, the latter
happens to call close_files() anyway so as a bonus it gets the nr_open_fd
return and atomic_subs the p->user->files accordingly. If all this works
and I didn't miss some descriptor accounting somewhere then we'll have a
valid p->user->files support which is what I wanted.
Regards,
Tigran
On Tue, 28 Nov 2000, Tigran Aivazian wrote:
> On Tue, 28 Nov 2000, David S. Miller wrote:
> > What you want is something like:
> >
> > static int num_open_files(struct files_struct *files, int size)
> > {
> > total = 0;
> > for (i = size / (8 * sizeof(long)); i > 0; )
> > total += count_bits(files->open_fds->fds_bits[--i]);
> >
> > return total;
> > }
>
> Ok, since we have to walk the sets and test the bits anyway, I propose to
> make close_files() to return 'nr_open_fds' and accept an extra argument
> 'doclose=0 or 1' which will specify whether we want to close the 'fd' or
> whether we just want to count them.
What for? I smell a bunch of races here - as soon as you release ->files_lock
the value of the function (it can be called only with that lock held) becomes
meaningless.
Besides, locking rules like that (you must hold the files->files_lock if
doclose is 0 and you must NOT hold it is doclose is 1) are sick. We could
make the function itself grab the spinlock, but then the return value
becomes junk before the thing returns it.
On Tue, 28 Nov 2000, Tigran Aivazian wrote:
> /* switch the open fds from old_user to new_user */
> read_lock(&files->file_lock);
> nr_open = close_files(files, 0); /* 0 means don't close them */
> atomic_sub(nr_open, &old_user->files);
> atomic_add(nr_open, &new_user->files);
> read_unlock(&files->file_lock);
That makes no sense - how do you count the descriptors in shared ->files?
And how on the Earth do you count SCM_RIGHTS packets? Because they make
a great way to fool any use of that stuff for resource-limit type of
applications (stash the descriptors into SCM_RIGHTS cookie, send them to
yourself and close them).
Basically, I don't see what are you counting.
On Tue, 28 Nov 2000, Alexander Viro wrote:
> Besides, locking rules like that (you must hold the files->files_lock if
> doclose is 0 and you must NOT hold it is doclose is 1) are sick. We could
> make the function itself grab the spinlock, but then the return value
> becomes junk before the thing returns it.
Ok, you are right (about such policy being sick) -- I will think a bit
more. Thank you!
Regards,
Tigran
Please just finally tell us what you are trying to do with
all this instead of just pointing out side-effect details.
What are you trying to do that requires counting the number
of open files?
It all smells very fishy, as Alexander stated.
Later,
David S. Miller
[email protected]
Hi Alexander,
Thank you for the useful comments.
On Tue, 28 Nov 2000, Alexander Viro wrote:
> On Tue, 28 Nov 2000, Tigran Aivazian wrote:
>
> > /* switch the open fds from old_user to new_user */
> > read_lock(&files->file_lock);
> > nr_open = close_files(files, 0); /* 0 means don't close them */
> > atomic_sub(nr_open, &old_user->files);
> > atomic_add(nr_open, &new_user->files);
> > read_unlock(&files->file_lock);
>
> That makes no sense - how do you count the descriptors in shared ->files?
> And how on the Earth do you count SCM_RIGHTS packets? Because they make
> a great way to fool any use of that stuff for resource-limit type of
> applications (stash the descriptors into SCM_RIGHTS cookie, send them to
> yourself and close them).
Yes, both the shared file struct and the SCM_RIGHTS are not so easy to
account. I never said this is ready -- just work in progress. If you have
already done this, please let me know -- there is plenty of other things
to do and I don't wish to step on your toes.
>
> Basically, I don't see what are you counting.
?????????
it is not basic at all. The problems you point out are extremely complex
(at least the fd in transit issue, definitely is).
So, yes it requires a bit more thought. I will come back when the issues
you pointed out are dealt with. Someone has added the 'files' field to the
'struct user_struct' so someone must have meant to put support for this
field to be something other than the meaningless 0 it currently is.
Regards,
Tigran
On Tue, 28 Nov 2000, Richard B. Johnson wrote:
> Yes. This is probably related to the previously-reported MOD_INC_USE_COUNT
> macro in the drivers. The open count is now handled in the call to
> the driver's open(). The MOD_INC_USE_COUNT and the MOD_DEC_USE_COUNT
> should now be defined to do nothing for kernels that already handle
> the counts in the calls.
Hi Richard,
Although your comment had nothing to do with my question -- I still
would like to point out that:
a) the driver in question (/dev/cpu/micocode) deals with module accounting
correctly by using THIS_MODULE in the fops, it doesn't use
MOD_INC/DEC_USE_COUNT macros
b) No, those macros should NOT be defined to do nothing because the Linux
kernel is a lot more than just set of drivers. There are subsystems that
still need them.
Regards,
Tigran
On Tue, 28 Nov 2000, Tigran Aivazian wrote:
> it is not basic at all. The problems you point out are extremely complex
> (at least the fd in transit issue, definitely is).
>
> So, yes it requires a bit more thought. I will come back when the issues
> you pointed out are dealt with. Someone has added the 'files' field to the
> 'struct user_struct' so someone must have meant to put support for this
> field to be something other than the meaningless 0 it currently is.
You know, in such cases usual course of actions is to remove the bloody
thing. It's not used, it's not set to anything useful, semantics is
fundamentally non-obvious, so Occam's Razor applies. Until somebody
comes up with a reasonable use _and_ clear semantics... Trying to invent
one simply because the field is there looks, erm, odd.
--
POSIX should be renamed to FPOSIX - it is U*IX-like, all right, but POS is
too mild to describe its quality.
On Tue, 28 Nov 2000, Alexander Viro wrote:
> You know, in such cases usual course of actions is to remove the bloody
> thing. It's not used, it's not set to anything useful, semantics is
> fundamentally non-obvious, so Occam's Razor applies. Until somebody
> comes up with a reasonable use _and_ clear semantics... Trying to invent
> one simply because the field is there looks, erm, odd.
Yes, I agree and, like I said, there are other things to do still. It just
looked like "the field was added recently but no support for it so it may
be a 'must-have' item for 2.4.0" which is why I rushed to try and give it
some meaning.
At least one useful thing came out of this exercise -- I understand the fd
allocation (fs/file.c) routines now.
Regards,
Tigran