free tables upon fd_growtable

[sam.houston.peterson_gmail.com]

Summary

This diff implements the freeing of old file descriptor tables after the file descriptor table has been grown, in the case of a single threaded process where the file descriptor table has not been shared with another process. Details can be found in the junior jobs wiki: here. The relevant project details are given below:

Free old file descriptor tables

Technical Contact: mjg@

Difficulty

Easy

Description

File descriptor tables can be shared between multiple processes (see fdshare).

The kernel performs lockless file descriptor lookup, but file descriptor table can be reallocated by fdgrowtable at any time. To deal with this problem old tables are kept around until all processes possibly using them exit.

However, we can safely free the old table if given process has only one thread and the table is not shared.

Implement freeing such tables and submit a test program showing that it works.

Test Plan

To test this, my idea is to create processes that implement the following test cases, in which many files are created in the following contexts:

1. single-thread, no file sharing
2. multi-thread, no file sharing
3. single-thread, file sharing
4. multi-thread, file sharing

By "many files" i mean enough files for fd_growtable to be called twice. The first file descriptor table is left alone by design, and later ones are discarded.

To test whether the file descriptor tables are being freed, I think we can use D-Trace, with the following probes

BEGIN
{
	trace("howdy");
}

fbt::fdgrowtable:entry
/execname == "test_program"/
{
	printf("\ncan_free value: %d\n", arg2);
	printf("\nFiledescriptor table: 0x%p\n",
			curthread->td_proc->p_fd->fd_files);
	printf("\nonfiles: %d\n",curthread->td_proc->p_fd->fd_files->\
			fdt_nfiles);
	printf("\nref_cnt: %d\n",curthread->td_proc->p_fd->fd_refcnt);
	
}

fbt::free:entry
/execname == "test_program" && arg0 != 0/
{

	printf("%s(0x%p)",probefunc, arg0);

}

fbt::fdescfree_fds:entry
/execname == "test_program"/
{
	printf("Whole filedesc structure is being freed");
}

The idea being that if the file descriptor table is supposed to be freed upon call of fd_growtable, we should see the appropriate call to the function free().
Otherwise, the file descriptor table should be freed after fdesc_free is called.

[mjg]

I don't see what's the benefit of having the callers of fdgrowtable decide whether the old table can be freed. If you are worried about the case where a multithreaded process forks, that's covered by the first comment in the function about fd_lastfile == -1. This remark aside, the patch checks for correct things but does it wrong. :)

I suggest embedding necessary checks in fdgrowtable and storing the result in a bool variable.

kern_descrip.c

1553 ↗

(On Diff #30744)

What's the benefit of this comment?

1667 ↗

(On Diff #30744)

Similarly, this comment does not add any value.

1678 ↗

(On Diff #30744)

This routine should return bool.

1686 ↗

(On Diff #30744)

That's a weirdly round-about way of doing the check. There is a significantly simpler method which you can find if you inspect the code creating/destroying threads.

The line violates style by initialising like this in declaration.

The check is buggy. If the process is multithreaded and first thread exits, you can end up deferencing a freed object. It so happens threads are never really freed but only marked as unused, so this would not crash. However, said object can get reallocated for a single-threaded process, in which case this code will incorrectly conclude the caller is single-threaded.

1689 ↗

(On Diff #30744)

This check is fine, but there is no use assigning the result to a variable.

[sam.houston.peterson_gmail.com]

Looking through the proc structure a second time, I see the field p_numthreads, which I now use for the check regarding multi-threadedness.

I also checked for this fields use and found it is used in the thread_link and thread_unlink routines in the file kern_thread.c. Is this the code you were referring to in your clue about finding the simpler manner of determining thread count? I.E:

There is a significantly simpler method which you can find if you inspect the code creating/destroying threads.

The check is now done in the fdgrowtable routine, with the result stored as a boolean.

I removed the unnecessary comments highlighted in your review. Sorry, it is sometimes unclear what degree of explicitness is appropriate for comments.

[sam.houston.peterson_gmail.com]

This comment was removed by sam.houston.peterson_gmail.com.

[mjg]

This looks much better overall.

There is not much point adding the proc parameter to fdgrowtable. If any was to be added, it would have to be struct thread. *. However, here there is no problem just using curproc.

Last time I mentioned multithreaded processes during fdcopy. This variant of the code will not free the table from the filedesc struct if the caller is multithreaded, even though it should. The necessary code to spot the unused table is already present in fdgrowtable with a comment.

sys/kern/kern_descrip.c

1886

this should use curproc

[sam.houston.peterson_gmail.com]

Changed to proc handling to curproc.

Added the condition that can_free is true if fdp->fd_lastfile == -1. Is that what you meant by the following?

Last time I mentioned multithreaded processes during fdcopy. This variant of the code will not free the table from the filedesc struct if the caller is multithreaded, even though it should. The necessary code to spot the unused table is already present in fdgrowtable with a comment.

[mjg]

This does not look like a diff against head, for instance it includes the freeing of the old table.

[sam.houston.peterson_gmail.com]

I am unsure what you mean. In case I made an error with svn, I cleared out the /usr/src directory, checkout out head again with the command:

svn checkout https://svn.FreeBSD.org/base/head /usr/src

Then I remade the changes and obtained the diff with:

svn diff /usr/src/sys/kern/kern_descrip.c

Is that correct?

[sam.houston.peterson_gmail.com]

Alright, now I used the command:

svn diff -r HEAD kern_descrip.c

Hopefully that's right. It didn't appear to be any different from the other diffs though. (except for the line indicating that the diff was respect to revision 321195 (rather than 321008)

[mjg]

There is definitely something messed up. Now there is no diff at all, but just a patched file. I don't know this tooling so can't you with it. Regardless, I can still provide comments.

sys/kern/kern_descrip.c

32

Now that this code got shortened it's clear it can be simplified. In particular there is no need to test can_free twice. Instead, the code should free the map. Then see if onfiles <= NDFILE, in which case it can just return. Now that we know onfiles > NDFILE is true, one can test if the table can be freed (do it if so) and add it to the list if not.

[sam.houston.peterson_gmail.com]

That's a neat optimization, thanks!

Since the check for onfiles > NDFILES is now implicit rather than explicit for the freeing and appending conditionals, I state that check in the comment.

How would you like me to send the diffs if the output on this platform is weird? Would you like me to email them to you? From my end, I don't see the same oddities that you are;
when you are viewing the revision contents, are you specifying in the history option that the diffs should be compared to the Base (my actual diffs are in fact referencing HEAD now, but the history option says BASE)?

[mjg]

sys/kern/kern_descrip.c

1650

Now that the result of the check is only needed once there is no need for the variable. It should be acted up on imemdiately in the code below instead.

1663

there is some weird whitespace damage here. Also the comment adds no value as it straight up repeats the if condition.

1669

this is testing somewhat backwards. also since this is a bool this should have been mere if (!can_free) or if (can_free). also see the previous note about not having the variable in the first place.

1676

this violates style. these braces should come on the same line.

the free case is shorter and should be handled first.

[sam.houston.peterson_gmail.com]

can_free variable assignment removed, checks reversed, and style conformed to.