Date: Thu, 30 Nov 2017 10:31:49 -0800 From: Conrad Meyer <cem@freebsd.org> To: Emmanuel Vadot <manu@freebsd.org> Cc: src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r326394 - head/sys/fs/devfs Message-ID: <CAG6CVpUe5jD6mWp7LQUGiy4Ova6XeVW%2B34JhvFw-N=X8=j0vDw@mail.gmail.com> In-Reply-To: <201711301238.vAUCcgrC091396@repo.freebsd.org> References: <201711301238.vAUCcgrC091396@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
This moves the M_WAITOK (sleepable) memory allocation under
dirlist_mtx, which is an ordinary mutex. If you run this under
WITNESS, it will (rightfully) complain. Please revert this change.
One possible way to accomplish the goal you have in mind is a fallback
path if the devfs_dir_findent_locked misses. E.g.:
=====================================8<==================================
--- a/sys/fs/devfs/devfs_dir.c
+++ b/sys/fs/devfs/devfs_dir.c
@@ -96,19 +96,27 @@ devfs_dir_ref(const char *dir)
if (*dir == '\0')
return;
- dle_new = malloc(sizeof(*dle), M_DEVFS4, M_WAITOK);
- dle_new->dir = strdup(dir, M_DEVFS4);
- dle_new->refcnt = 1;
+ dle_new = NULL;
+again:
mtx_lock(&dirlist_mtx);
dle = devfs_dir_findent_locked(dir);
if (dle != NULL) {
dle->refcnt++;
mtx_unlock(&dirlist_mtx);
- free(dle_new->dir, M_DEVFS4);
- free(dle_new, M_DEVFS4);
+ if (dle_new != NULL) {
+ free(dle_new->dir, M_DEVFS4);
+ free(dle_new, M_DEVFS4);
+ }
return;
}
+ if (dle_new == NULL) {
+ mtx_unlock(&dirlist_mtx);
+ dle_new = malloc(sizeof(*dle), M_DEVFS4, M_WAITOK);
+ dle_new->dir = strdup(dir, M_DEVFS4);
+ dle_new->refcnt = 1;
+ goto again;
+ }
LIST_INSERT_HEAD(&devfs_dirlist, dle_new, link);
mtx_unlock(&dirlist_mtx);
}
=====================================8<==================================
The downside of this method is we potentially have to take the dirlist
mutex twice. An even more "optimized" approach would be to M_NOWAIT
the allocations inside the lock and fallback if those fail. But that
introduces even more code complexity.
Best,
Conrad
On Thu, Nov 30, 2017 at 4:38 AM, Emmanuel Vadot <manu@freebsd.org> wrote:
> Author: manu
> Date: Thu Nov 30 12:38:42 2017
> New Revision: 326394
> URL: https://svnweb.freebsd.org/changeset/base/326394
>
> Log:
> devfs: Avoid a malloc/free if we just need to increment the refcount
>
> MFC after: 1 week
> Sponsored by: Gandi.net
>
> Modified:
> head/sys/fs/devfs/devfs_dir.c
>
> Modified: head/sys/fs/devfs/devfs_dir.c
> ==============================================================================
> --- head/sys/fs/devfs/devfs_dir.c Thu Nov 30 12:22:15 2017 (r326393)
> +++ head/sys/fs/devfs/devfs_dir.c Thu Nov 30 12:38:42 2017 (r326394)
> @@ -98,19 +98,18 @@ devfs_dir_ref(const char *dir)
> if (*dir == '\0')
> return;
>
> - dle_new = malloc(sizeof(*dle), M_DEVFS4, M_WAITOK);
> - dle_new->dir = strdup(dir, M_DEVFS4);
> - dle_new->refcnt = 1;
> -
> mtx_lock(&dirlist_mtx);
> dle = devfs_dir_findent_locked(dir);
> if (dle != NULL) {
> dle->refcnt++;
> mtx_unlock(&dirlist_mtx);
> - free(dle_new->dir, M_DEVFS4);
> - free(dle_new, M_DEVFS4);
> return;
> }
> +
> + dle_new = malloc(sizeof(*dle), M_DEVFS4, M_WAITOK);
> + dle_new->dir = strdup(dir, M_DEVFS4);
> + dle_new->refcnt = 1;
> +
> LIST_INSERT_HEAD(&devfs_dirlist, dle_new, link);
> mtx_unlock(&dirlist_mtx);
> }
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAG6CVpUe5jD6mWp7LQUGiy4Ova6XeVW%2B34JhvFw-N=X8=j0vDw>