Re: [PATCH v1 2/2] binderfs: reserve devices for initial mount
On Thu, Jan 3, 2019 at 2:08 PM Christian Brauner wrote: > > On Thu, Jan 03, 2019 at 01:47:13PM -0800, Todd Kjos wrote: > > On Thu, Jan 3, 2019 at 12:34 PM Christian Brauner > > wrote: > > > > > > On Thu, Jan 03, 2019 at 12:25:24PM -0800, Todd Kjos wrote: > > > > On Sun, Dec 23, 2018 at 6:36 AM Christian Brauner > > > > wrote: > > > > > > > > > > The binderfs instance in the initial ipc namespace will always have a > > > > > reserve of 4 binder devices unless explicitly capped by specifying a > > > > > lower > > > > > value via the "max" mount option. > > > > > This ensures when binder devices are removed (on accident or on > > > > > purpose) > > > > > they can always be recreated without risking that all minor numbers > > > > > have > > > > > already been used up. > > > > > > > > > > Cc: Todd Kjos > > > > > Cc: Greg Kroah-Hartman > > > > > Signed-off-by: Christian Brauner > > > > > --- > > > > > v1: > > > > > - patch introduced > > > > > v0: > > > > > - patch not present > > > > > --- > > > > > drivers/android/binderfs.c | 7 ++- > > > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c > > > > > index 873adda064ac..aa635c7ea727 100644 > > > > > --- a/drivers/android/binderfs.c > > > > > +++ b/drivers/android/binderfs.c > > > > > @@ -40,6 +40,8 @@ > > > > > #define INODE_OFFSET 3 > > > > > #define INTSTRLEN 21 > > > > > #define BINDERFS_MAX_MINOR (1U << MINORBITS) > > > > > +/* Ensure that the initial ipc namespace always has a devices > > > > > available. */ > > > > > +#define BINDERFS_MAX_MINOR_CAPPED (BINDERFS_MAX_MINOR - 4) > > > > > > > > Why do you ever need more minors per instance than the number of > > > > devices listed in CONFIG_ANDROID_BINDER_DEVICES? > > > > > > Are you asking asking why this is 4 and not 3? In that case we should > > > probably parse CONFIG_ANDROID_BINDER_DEVICES once at init time and then > > > reserve that many binder devices. Thoughts? > > > > I'm asking why CONFIG_ANDROID_BINDER_DEVICES isn't the source of truth > > for the number of devices (it normally specifies 3 devices, but could > > be different). I can't think of a reason why you'd want > > CONFIG_MAX_MINOR_CAPPED to be different than the number of devices > > indicated by CONFIG_ANDROID_BINDER_DEVICES and having it specified in > > two places seems error prone. > > I'm not following. The CONFIG_MAX_MINOR_CAPPED and > CONFIG_ANDROID_BINDER_DEVICES do not have a relation to each other just > like binder devices requested in CONFIG_ANDROID_BINDER_DEVICES do not > have a relation to binderfs binder devices as was requested for this > patchset. > I also don't know what you mean "appear in two places". > > The fact is, binderfs binder devices are independent of binderfs binder > devices. So it is perfectly reasonable for someone to say > CONFIG_ANDROID_BINDER_DEVICES="" and *only* rely on binderfs itself. > Which absolutely need to be possible. > What I want in such scenarios is that people always have a number of > binderfs binder devices guaranteed to be available in the binderfs mount > in the initial ipc namespace no matter how many devices are allocated in > non-initial ipc namespace binderfs mounts. That's why allo non-initial > ipc namespace binderfs mounts will use the CONFIG_MAX_MINOR_CAPPED macro > which guarantees that there's number of devices reserved for the > binderfs instance in the initial ipc namespace. Yes, you are right. Cobwebs from vacation -- I confused this with the previous non-binderfs implementation that was posted that did use CONFIG_ANDROID_BINDER_DEVICES to instantiate the devices in all containers. In binderfs, that is only used for the initial (default) devices.
Re: [PATCH v1 2/2] binderfs: reserve devices for initial mount
On Thu, Jan 03, 2019 at 01:47:13PM -0800, Todd Kjos wrote: > On Thu, Jan 3, 2019 at 12:34 PM Christian Brauner > wrote: > > > > On Thu, Jan 03, 2019 at 12:25:24PM -0800, Todd Kjos wrote: > > > On Sun, Dec 23, 2018 at 6:36 AM Christian Brauner > > > wrote: > > > > > > > > The binderfs instance in the initial ipc namespace will always have a > > > > reserve of 4 binder devices unless explicitly capped by specifying a > > > > lower > > > > value via the "max" mount option. > > > > This ensures when binder devices are removed (on accident or on purpose) > > > > they can always be recreated without risking that all minor numbers have > > > > already been used up. > > > > > > > > Cc: Todd Kjos > > > > Cc: Greg Kroah-Hartman > > > > Signed-off-by: Christian Brauner > > > > --- > > > > v1: > > > > - patch introduced > > > > v0: > > > > - patch not present > > > > --- > > > > drivers/android/binderfs.c | 7 ++- > > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c > > > > index 873adda064ac..aa635c7ea727 100644 > > > > --- a/drivers/android/binderfs.c > > > > +++ b/drivers/android/binderfs.c > > > > @@ -40,6 +40,8 @@ > > > > #define INODE_OFFSET 3 > > > > #define INTSTRLEN 21 > > > > #define BINDERFS_MAX_MINOR (1U << MINORBITS) > > > > +/* Ensure that the initial ipc namespace always has a devices > > > > available. */ > > > > +#define BINDERFS_MAX_MINOR_CAPPED (BINDERFS_MAX_MINOR - 4) > > > > > > Why do you ever need more minors per instance than the number of > > > devices listed in CONFIG_ANDROID_BINDER_DEVICES? > > > > Are you asking asking why this is 4 and not 3? In that case we should > > probably parse CONFIG_ANDROID_BINDER_DEVICES once at init time and then > > reserve that many binder devices. Thoughts? > > I'm asking why CONFIG_ANDROID_BINDER_DEVICES isn't the source of truth > for the number of devices (it normally specifies 3 devices, but could > be different). I can't think of a reason why you'd want > CONFIG_MAX_MINOR_CAPPED to be different than the number of devices > indicated by CONFIG_ANDROID_BINDER_DEVICES and having it specified in > two places seems error prone. I'm not following. The CONFIG_MAX_MINOR_CAPPED and CONFIG_ANDROID_BINDER_DEVICES do not have a relation to each other just like binder devices requested in CONFIG_ANDROID_BINDER_DEVICES do not have a relation to binderfs binder devices as was requested for this patchset. I also don't know what you mean "appear in two places". The fact is, binderfs binder devices are independent of binderfs binder devices. So it is perfectly reasonable for someone to say CONFIG_ANDROID_BINDER_DEVICES="" and *only* rely on binderfs itself. Which absolutely need to be possible. What I want in such scenarios is that people always have a number of binderfs binder devices guaranteed to be available in the binderfs mount in the initial ipc namespace no matter how many devices are allocated in non-initial ipc namespace binderfs mounts. That's why allo non-initial ipc namespace binderfs mounts will use the CONFIG_MAX_MINOR_CAPPED macro which guarantees that there's number of devices reserved for the binderfs instance in the initial ipc namespace.
Re: [PATCH v1 2/2] binderfs: reserve devices for initial mount
On Thu, Jan 3, 2019 at 12:34 PM Christian Brauner wrote: > > On Thu, Jan 03, 2019 at 12:25:24PM -0800, Todd Kjos wrote: > > On Sun, Dec 23, 2018 at 6:36 AM Christian Brauner > > wrote: > > > > > > The binderfs instance in the initial ipc namespace will always have a > > > reserve of 4 binder devices unless explicitly capped by specifying a lower > > > value via the "max" mount option. > > > This ensures when binder devices are removed (on accident or on purpose) > > > they can always be recreated without risking that all minor numbers have > > > already been used up. > > > > > > Cc: Todd Kjos > > > Cc: Greg Kroah-Hartman > > > Signed-off-by: Christian Brauner > > > --- > > > v1: > > > - patch introduced > > > v0: > > > - patch not present > > > --- > > > drivers/android/binderfs.c | 7 ++- > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c > > > index 873adda064ac..aa635c7ea727 100644 > > > --- a/drivers/android/binderfs.c > > > +++ b/drivers/android/binderfs.c > > > @@ -40,6 +40,8 @@ > > > #define INODE_OFFSET 3 > > > #define INTSTRLEN 21 > > > #define BINDERFS_MAX_MINOR (1U << MINORBITS) > > > +/* Ensure that the initial ipc namespace always has a devices available. > > > */ > > > +#define BINDERFS_MAX_MINOR_CAPPED (BINDERFS_MAX_MINOR - 4) > > > > Why do you ever need more minors per instance than the number of > > devices listed in CONFIG_ANDROID_BINDER_DEVICES? > > Are you asking asking why this is 4 and not 3? In that case we should > probably parse CONFIG_ANDROID_BINDER_DEVICES once at init time and then > reserve that many binder devices. Thoughts? I'm asking why CONFIG_ANDROID_BINDER_DEVICES isn't the source of truth for the number of devices (it normally specifies 3 devices, but could be different). I can't think of a reason why you'd want CONFIG_MAX_MINOR_CAPPED to be different than the number of devices indicated by CONFIG_ANDROID_BINDER_DEVICES and having it specified in two places seems error prone. > > Christian
Re: [PATCH v1 2/2] binderfs: reserve devices for initial mount
On Thu, Jan 03, 2019 at 12:25:24PM -0800, Todd Kjos wrote: > On Sun, Dec 23, 2018 at 6:36 AM Christian Brauner > wrote: > > > > The binderfs instance in the initial ipc namespace will always have a > > reserve of 4 binder devices unless explicitly capped by specifying a lower > > value via the "max" mount option. > > This ensures when binder devices are removed (on accident or on purpose) > > they can always be recreated without risking that all minor numbers have > > already been used up. > > > > Cc: Todd Kjos > > Cc: Greg Kroah-Hartman > > Signed-off-by: Christian Brauner > > --- > > v1: > > - patch introduced > > v0: > > - patch not present > > --- > > drivers/android/binderfs.c | 7 ++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c > > index 873adda064ac..aa635c7ea727 100644 > > --- a/drivers/android/binderfs.c > > +++ b/drivers/android/binderfs.c > > @@ -40,6 +40,8 @@ > > #define INODE_OFFSET 3 > > #define INTSTRLEN 21 > > #define BINDERFS_MAX_MINOR (1U << MINORBITS) > > +/* Ensure that the initial ipc namespace always has a devices available. */ > > +#define BINDERFS_MAX_MINOR_CAPPED (BINDERFS_MAX_MINOR - 4) > > Why do you ever need more minors per instance than the number of > devices listed in CONFIG_ANDROID_BINDER_DEVICES? Are you asking asking why this is 4 and not 3? In that case we should probably parse CONFIG_ANDROID_BINDER_DEVICES once at init time and then reserve that many binder devices. Thoughts? Christian
Re: [PATCH v1 2/2] binderfs: reserve devices for initial mount
On Sun, Dec 23, 2018 at 6:36 AM Christian Brauner wrote: > > The binderfs instance in the initial ipc namespace will always have a > reserve of 4 binder devices unless explicitly capped by specifying a lower > value via the "max" mount option. > This ensures when binder devices are removed (on accident or on purpose) > they can always be recreated without risking that all minor numbers have > already been used up. > > Cc: Todd Kjos > Cc: Greg Kroah-Hartman > Signed-off-by: Christian Brauner > --- > v1: > - patch introduced > v0: > - patch not present > --- > drivers/android/binderfs.c | 7 ++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c > index 873adda064ac..aa635c7ea727 100644 > --- a/drivers/android/binderfs.c > +++ b/drivers/android/binderfs.c > @@ -40,6 +40,8 @@ > #define INODE_OFFSET 3 > #define INTSTRLEN 21 > #define BINDERFS_MAX_MINOR (1U << MINORBITS) > +/* Ensure that the initial ipc namespace always has a devices available. */ > +#define BINDERFS_MAX_MINOR_CAPPED (BINDERFS_MAX_MINOR - 4) Why do you ever need more minors per instance than the number of devices listed in CONFIG_ANDROID_BINDER_DEVICES? > > static struct vfsmount *binderfs_mnt; > > @@ -129,11 +131,14 @@ static int binderfs_binder_device_create(struct inode > *ref_inode, > struct inode *inode = NULL; > struct super_block *sb = ref_inode->i_sb; > struct binderfs_info *info = sb->s_fs_info; > + bool use_reserve = (info->ipc_ns == _ipc_ns); > > /* Reserve new minor number for the new device. */ > mutex_lock(_minors_mutex); > if (++info->device_count <= info->mount_opts.max) > - minor = ida_alloc_max(_minors, BINDERFS_MAX_MINOR, > + minor = ida_alloc_max(_minors, > + use_reserve ? BINDERFS_MAX_MINOR : > + BINDERFS_MAX_MINOR_CAPPED, > GFP_KERNEL); > else > minor = -ENOSPC; > -- > 2.19.1 >
[PATCH v1 2/2] binderfs: reserve devices for initial mount
The binderfs instance in the initial ipc namespace will always have a reserve of 4 binder devices unless explicitly capped by specifying a lower value via the "max" mount option. This ensures when binder devices are removed (on accident or on purpose) they can always be recreated without risking that all minor numbers have already been used up. Cc: Todd Kjos Cc: Greg Kroah-Hartman Signed-off-by: Christian Brauner --- v1: - patch introduced v0: - patch not present --- drivers/android/binderfs.c | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c index 873adda064ac..aa635c7ea727 100644 --- a/drivers/android/binderfs.c +++ b/drivers/android/binderfs.c @@ -40,6 +40,8 @@ #define INODE_OFFSET 3 #define INTSTRLEN 21 #define BINDERFS_MAX_MINOR (1U << MINORBITS) +/* Ensure that the initial ipc namespace always has a devices available. */ +#define BINDERFS_MAX_MINOR_CAPPED (BINDERFS_MAX_MINOR - 4) static struct vfsmount *binderfs_mnt; @@ -129,11 +131,14 @@ static int binderfs_binder_device_create(struct inode *ref_inode, struct inode *inode = NULL; struct super_block *sb = ref_inode->i_sb; struct binderfs_info *info = sb->s_fs_info; + bool use_reserve = (info->ipc_ns == _ipc_ns); /* Reserve new minor number for the new device. */ mutex_lock(_minors_mutex); if (++info->device_count <= info->mount_opts.max) - minor = ida_alloc_max(_minors, BINDERFS_MAX_MINOR, + minor = ida_alloc_max(_minors, + use_reserve ? BINDERFS_MAX_MINOR : + BINDERFS_MAX_MINOR_CAPPED, GFP_KERNEL); else minor = -ENOSPC; -- 2.19.1
