[prev in list] [next in list] [prev in thread] [next in thread] 

List:       freebsd-commits-all
Subject:    Re: svn commit: r326394 - head/sys/fs/devfs
From:       Conrad Meyer <cem () freebsd ! org>
Date:       2017-11-30 18:31:49
Message-ID: CAG6CVpUe5jD6mWp7LQUGiy4Ova6XeVW+34JhvFw-N=X8=j0vDw () mail ! gmail ! com
[Download RAW message or body]

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);
>  }
>
_______________________________________________
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
[prev in list] [next in list] [prev in thread] [next in thread]