svn commit: r236935 - head/sys/kern

[Mateusz Guzik]

On Wed, Jun 13, 2012 at 12:29:01PM +0200, Pawel Jakub Dawidek wrote:
> On Wed, Jun 13, 2012 at 11:38:07AM +0200, Mateusz Guzik wrote:
> > On Tue, Jun 12, 2012 at 11:44:00PM +0200, Pawel Jakub Dawidek wrote:
> > > On Tue, Jun 12, 2012 at 09:18:28PM +0200, Mateusz Guzik wrote:
> > > > On Tue, Jun 12, 2012 at 06:01:29PM +0200, Pawel Jakub Dawidek wrote:
> > > > > Look good and you can just add 'In co-operation with: pjd'.
> > > > > One minor thing is that fd_first_free() can return 'size' if there are
> > > > > no free slots available. Could you include that in the comment as well?
> > > > > 
> > > > 
> > > > http://people.freebsd.org/~mjg/patches/fdalloc%2bfd_first_free2.patch
> > > 
> > > Ok. Merge racct_set() call into one line, it now fits into 80 chars.
> > > I have no more objections.
> > > 
> > 
> > This patch contains terrible brain-o:
> > 
> > if (fd >= fdp->fd_nfiles) {
> > 	allocfd = 2 * max(fdp->fd_nfiles, fd);
> > 
> > allocfd is always 2 * fd, which I think is ok, just no reason to compare
> > again with fdp->fd_nfiles.
> > 
> > Also the patch can be simplified.
> > 
> > fdp xlock is held for whole duration of fdalloc.
> > 
> > Code:
> > fdgrowtable(fdp, min(allocfd, maxfd));
> > /* Retry... */
> > fd = fd_first_free(fdp, minfd, fdp->fd_nfiles);
> > if (fd >= maxfd)
> > 	return (EMFILE);
> > 
> > fdgrowtable never fails and when we call it we know that new fd will fit
> > in the table. In case of no free entries fd_first_free returns size,
> > which coressponds to first newly allocated fd after growth. So fd after
> > table growth is already the first free descriptor that can be used and
> > would be returned by another call to fd_first_free.
> > 
> > To sum up:
> > http://people.freebsd.org/~mjg/patches/fdalloc%2bfd_first_free4.patch
> > 
> > Any comments?
> 
> All makes sense and the code looks good, but I'd love to see such an
> explanation in the code as well (below or above fdgrowtable() call).
> 

http://people.freebsd.org/~mjg/patches/fdalloc%2bfd_first_free5.patch

David reported that previous version of the patch fixed panic he was
experiencing, so I consider the patch tested.

Thanks,
-- 
Mateusz Guzik <mjguzik gmail.com>