OpenVZ Forum - RDF feed
type: channel
items: [PATCH 2.6.24-rc8-mm1 00/15] IPC: code rewrite + new functionalities
items: [PATCH 2.6.24-rc8-mm1 01/15] IPC/semaphores: code factorisation
items: [PATCH 2.6.24-rc8-mm1 02/15] IPC/shared memory: introduce shmctl_down
items: [PATCH 2.6.24-rc8-mm1 03/15] IPC/message queues: introduce msgctl_down
items: [PATCH 2.6.24-rc8-mm1 04/15] IPC/semaphores: move the rwmutex handling inside semctl_down
items: [PATCH 2.6.24-rc8-mm1 05/15] IPC/semaphores: remove one unused parameter from semctl_down()
items: [PATCH 2.6.24-rc8-mm1 06/15] IPC: get rid of the use *_setbuf structure.
items: [PATCH 2.6.24-rc8-mm1 07/15] IPC: introduce ipc_update_perm()
items: [PATCH 2.6.24-rc8-mm1 08/15] IPC: consolidate all xxxctl_down() functions
items: [PATCH 2.6.24-rc8-mm1 09/15] (RFC) IPC: new kernel API to change an ID
items: [PATCH 2.6.24-rc8-mm1 10/15] (RFC) IPC: new IPC_SETID command to modify an ID
items: [PATCH 2.6.24-rc8-mm1 11/15] (RFC) IPC: new IPC_SETALL command to modify all settings
items: [PATCH 2.6.24-rc8-mm1 12/15] (RFC) IPC/semaphores: make use of RCU to free the sem_undo_list
items: [PATCH 2.6.24-rc8-mm1 13/15] (RFC) IPC/semaphores: per <pid> semundo file in procfs
items: [PATCH 2.6.24-rc8-mm1 14/15] (RFC) IPC/semaphores: prepare semundo code to work on another task than
items: [PATCH 2.6.24-rc8-mm1 15/15] (RFC) IPC/semaphores: add write() operation to semundo file in procfs
items: Re: [PATCH 2.6.24-rc8-mm1 09/15] (RFC) IPC: new kernel API to change an ID
items: Re: [PATCH 2.6.24-rc8-mm1 12/15] (RFC) IPC/semaphores: make use of RCU to free the sem_undo_list
items: Re: [PATCH 2.6.24-rc8-mm1 14/15] (RFC) IPC/semaphores: prepare semundo code to work on another task
items: Re: [PATCH 2.6.24-rc8-mm1 05/15] IPC/semaphores: remove one unused parameter from semctl_down()
items: Re: [PATCH 2.6.24-rc8-mm1 00/15] IPC: code rewrite + new functionalities
items: Re: Re: [PATCH 2.6.24-rc8-mm1 09/15] (RFC) IPC: new kernel API to change an ID

[PATCH 2.6.24-rc8-mm1 00/15] IPC: code rewrite + new functionalities
type: item
description: Hi, Here is a patchset about the IPC, which proposes to consolidate some part of the existing code and to add some functionalities. * Patches 1 to 8 don't change the existing behavior, but propose to rewrite some parts of the existing code. In fact, the three kinds of IPC (semaphores, message queues and shared memory) have some common commands (IPC_SET, IPC_RMID, etc...) but they are mainly handled in three different ways. These patches propose to consolidate this, by handling these commands the same way and try to use, as much as possible, some common code. This should increase readability and maintainability of the code, making them probably good candidate for the -mm tree, I think. * Patches 9 to 15 propose to add some functionalities, and thus are submitted here for RFC, about both the interest and their implementation. These functionalities are: - Two new control-commands: . IPC_SETID: to change an IPC's id. . IPC_SETALL: behaves as IPC_SET, except that it also sets all time and pid values) - add a /proc/<pid>/semundo file to read and write the undo values of some semaphores for a given process. As the namespaces and the "containers" are being integrated in the kernel, these functionalities may be a first step to implement the checkpoint/restart of an application: in fact the existing API does not allow to specify or to change an ID when creating an IPC, when restarting an application, and the times/pids values of each IPCs are also altered. May be someone may find another interest about this ? So again, comments are welcome. Thanks. -- Pierre _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Pierre Peiffer
date: 2008-01-29T16:02:29-00:00

[PATCH 2.6.24-rc8-mm1 01/15] IPC/semaphores: code factorisation
type: item
description: From: Pierre Peiffer <pierre.peiffer@bull.net> Trivial patch which adds some small locking functions and makes use of them to factorize some part of code and makes it cleaner. Signed-off-by: Pierre Peiffer <pierre.peiffer@bull.net> Acked-by: Serge Hallyn <serue@us.ibm.com> --- ipc/sem.c | 61 +++++++++++++++++++++++++++++++------------------------------ 1 file changed, 31 insertions(+), 30 deletions(-) Index: b/ipc/sem.c =================================================================== --- a/ipc/sem.c +++ b/ipc/sem.c @@ -181,6 +181,25 @@ static inline struct sem_array *sem_lock return container_of(ipcp, struct sem_array, sem_perm); } +static inline void sem_lock_and_putref(struct sem_array *sma) +{ + ipc_lock_by_ptr(&sma->sem_perm); + ipc_rcu_putref(sma); +} + +static inline void sem_getref_and_unlock(struct sem_array *sma) +{ + ipc_rcu_getref(sma); + ipc_unlock(&(sma)->sem_perm); +} + +static inline void sem_putref(struct sem_array *sma) +{ + ipc_lock_by_ptr(&sma->sem_perm); + ipc_rcu_putref(sma); + ipc_unlock(&(sma)->sem_perm); +} + static inline void sem_rmid(struct ipc_namespace *ns, struct sem_array *s) { ipc_rmid(&sem_ids(ns), &s->sem_perm); @@ -700,19 +719,15 @@ static int semctl_main(struct ipc_namesp int i; if(nsems > SEMMSL_FAST) { - ipc_rcu_getref(sma); - sem_unlock(sma); + sem_getref_and_unlock(sma); sem_io = ipc_alloc(sizeof(ushort)*nsems); if(sem_io == NULL) { - ipc_lock_by_ptr(&sma->sem_perm); - ipc_rcu_putref(sma); - sem_unlock(sma); + sem_putref(sma); return -ENOMEM; } - ipc_lock_by_ptr(&sma->sem_perm); - ipc_rcu_putref(sma); + sem_lock_and_putref(sma); if (sma->sem_perm.deleted) { sem_unlock(sma); err = -EIDRM; @@ -733,38 +748,30 @@ static int semctl_main(struct ipc_namesp int i; struct sem_undo *un; - ipc_rcu_getref(sma); - sem_unlock(sma); + sem_getref_and_unlock(sma); if(nsems > SEMMSL_FAST) { sem_io = ipc_alloc(sizeof(ushort)*nsems); if(sem_io == NULL) { - ipc_lock_by_ptr(&sma->sem_perm); - ipc_rcu_putref(sma); - sem_unlock(sma); + sem_putref(…
creator: Pierre Peiffer
date: 2008-01-29T16:02:30-00:00

[PATCH 2.6.24-rc8-mm1 02/15] IPC/shared memory: introduce shmctl_down
type: item
description: From: Pierre Peiffer <pierre.peiffer@bull.net> Currently, the way the different commands are handled in sys_shmctl introduces some duplicated code. This patch introduces the shmctl_down function to handle all the commands requiring the rwmutex to be taken in write mode (ie IPC_SET and IPC_RMID for now). It is the equivalent function of semctl_down for shared memory. This removes some duplicated code for handling these both commands and harmonizes the way they are handled among all IPCs. Signed-off-by: Pierre Peiffer <pierre.peiffer@bull.net> Acked-by: Serge Hallyn <serue@us.ibm.com> --- ipc/shm.c | 160 +++++++++++++++++++++++++++----------------------------------- 1 file changed, 72 insertions(+), 88 deletions(-) Index: b/ipc/shm.c =================================================================== --- a/ipc/shm.c +++ b/ipc/shm.c @@ -625,10 +625,78 @@ static void shm_get_stat(struct ipc_name } } -asmlinkage long sys_shmctl (int shmid, int cmd, struct shmid_ds __user *buf) +/* + * This function handles some shmctl commands which require the rw_mutex + * to be held in write mode. + * NOTE: no locks must be held, the rw_mutex is taken inside this function. + */ +static int shmctl_down(struct ipc_namespace *ns, int shmid, int cmd, + struct shmid_ds __user *buf, int version) { + struct kern_ipc_perm *ipcp; struct shm_setbuf setbuf; struct shmid_kernel *shp; + int err; + + if (cmd == IPC_SET) { + if (copy_shmid_from_user(&setbuf, buf, version)) + return -EFAULT; + } + + down_write(&shm_ids(ns).rw_mutex); + shp = shm_lock_check_down(ns, shmid); + if (IS_ERR(shp)) { + err = PTR_ERR(shp); + goto out_up; + } + + ipcp = &shp->shm_perm; + + err = audit_ipc_obj(ipcp); + if (err) + goto out_unlock; + + if (cmd == IPC_SET) { + err = audit_ipc_set_perm(0, setbuf.uid, + setbuf.gid, setbuf.mode); + if (err) + goto out_unlock; + } + + if (current->euid != ipcp->uid && + current->euid != ipcp->cuid && + !capable(CAP_SYS_ADMIN)) { + err = -EPERM; + goto out_unlock; + } + + err = securit…
creator: Pierre Peiffer
date: 2008-01-29T16:02:31-00:00

[PATCH 2.6.24-rc8-mm1 03/15] IPC/message queues: introduce msgctl_down
type: item
description: From: Pierre Peiffer <pierre.peiffer@bull.net> Currently, sys_msgctl is not easy to read. This patch tries to improve that by introducing the msgctl_down function to handle all commands requiring the rwmutex to be taken in write mode (ie IPC_SET and IPC_RMID for now). It is the equivalent function of semctl_down for message queues. This greatly changes the readability of sys_msgctl and also harmonizes the way these commands are handled among all IPCs. Signed-off-by: Pierre Peiffer <pierre.peiffer@bull.net> Acked-by: Serge Hallyn <serue@us.ibm.com> --- ipc/msg.c | 162 ++++++++++++++++++++++++++++++++++---------------------------- 1 file changed, 89 insertions(+), 73 deletions(-) Index: b/ipc/msg.c =================================================================== --- a/ipc/msg.c +++ b/ipc/msg.c @@ -399,10 +399,95 @@ copy_msqid_from_user(struct msq_setbuf * } } -asmlinkage long sys_msgctl(int msqid, int cmd, struct msqid_ds __user *buf) +/* + * This function handles some msgctl commands which require the rw_mutex + * to be held in write mode. + * NOTE: no locks must be held, the rw_mutex is taken inside this function. + */ +static int msgctl_down(struct ipc_namespace *ns, int msqid, int cmd, + struct msqid_ds __user *buf, int version) { struct kern_ipc_perm *ipcp; - struct msq_setbuf uninitialized_var(setbuf); + struct msq_setbuf setbuf; + struct msg_queue *msq; + int err; + + if (cmd == IPC_SET) { + if (copy_msqid_from_user(&setbuf, buf, version)) + return -EFAULT; + } + + down_write(&msg_ids(ns).rw_mutex); + msq = msg_lock_check_down(ns, msqid); + if (IS_ERR(msq)) { + err = PTR_ERR(msq); + goto out_up; + } + + ipcp = &msq->q_perm; + + err = audit_ipc_obj(ipcp); + if (err) + goto out_unlock; + + if (cmd == IPC_SET) { + err = audit_ipc_set_perm(setbuf.qbytes, setbuf.uid, setbuf.gid, + setbuf.mode); + if (err) + goto out_unlock; + } + + if (current->euid != ipcp->cuid && + current->euid != ipcp->uid && + !capable(CAP_SYS_ADMIN)) { + /* We _could_ check for CAP_CHOWN a…
creator: Pierre Peiffer
date: 2008-01-29T16:02:32-00:00

[PATCH 2.6.24-rc8-mm1 04/15] IPC/semaphores: move the rwmutex handling inside semctl_down
type: item
description: From: Pierre Peiffer <pierre.peiffer@bull.net> semctl_down is called with the rwmutex (the one which protects the list of ipcs) taken in write mode. This patch moves this rwmutex taken in write-mode inside semctl_down. This has the advantages of reducing a little bit the window during which this rwmutex is taken, clarifying sys_semctl, and finally of having a coherent behaviour with [shm|msg]ctl_down Signed-off-by: Pierre Peiffer <pierre.peiffer@bull.net> Acked-by: Serge Hallyn <serue@us.ibm.com> --- ipc/sem.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) Index: b/ipc/sem.c =================================================================== --- a/ipc/sem.c +++ b/ipc/sem.c @@ -877,6 +877,11 @@ static inline unsigned long copy_semid_f } } +/* + * This function handles some semctl commands which require the rw_mutex + * to be held in write mode. + * NOTE: no locks must be held, the rw_mutex is taken inside this function. + */ static int semctl_down(struct ipc_namespace *ns, int semid, int semnum, int cmd, int version, union semun arg) { @@ -889,9 +894,12 @@ static int semctl_down(struct ipc_namesp if(copy_semid_from_user (&setbuf, arg.buf, version)) return -EFAULT; } + down_write(&sem_ids(ns).rw_mutex); sma = sem_lock_check_down(ns, semid); - if (IS_ERR(sma)) - return PTR_ERR(sma); + if (IS_ERR(sma)) { + err = PTR_ERR(sma); + goto out_up; + } ipcp = &sma->sem_perm; @@ -917,26 +925,22 @@ static int semctl_down(struct ipc_namesp switch(cmd){ case IPC_RMID: freeary(ns, ipcp); - err = 0; - break; + goto out_up; case IPC_SET: ipcp->uid = setbuf.uid; ipcp->gid = setbuf.gid; ipcp->mode = (ipcp->mode & ~S_IRWXUGO) | (setbuf.mode & S_IRWXUGO); sma->sem_ctime = get_seconds(); - sem_unlock(sma); - err = 0; break; default: - sem_unlock(sma); err = -EINVAL; - break; } - return err; out_unlock: sem_unlock(sma); +out_up: + up_write(&sem_ids(ns).rw_mutex); return err; } @@ -970,9 +974,7 @@ asmlinkage long sys_semctl (int semid, i return err; case IPC_…
creator: Pierre Peiffer
date: 2008-01-29T16:02:33-00:00

[PATCH 2.6.24-rc8-mm1 05/15] IPC/semaphores: remove one unused parameter from semctl_down()
type: item
description: From: Pierre Peiffer <pierre.peiffer@bull.net> semctl_down() takes one unused parameter: semnum. This patch proposes to get rid of it. Signed-off-by: Pierre Peiffer <pierre.peiffer@bull.net> Acked-by: Serge Hallyn <serue@us.ibm.com> --- ipc/sem.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) Index: b/ipc/sem.c =================================================================== --- a/ipc/sem.c +++ b/ipc/sem.c @@ -882,8 +882,8 @@ static inline unsigned long copy_semid_f * to be held in write mode. * NOTE: no locks must be held, the rw_mutex is taken inside this function. */ -static int semctl_down(struct ipc_namespace *ns, int semid, int semnum, - int cmd, int version, union semun arg) +static int semctl_down(struct ipc_namespace *ns, int semid, + int cmd, int version, union semun arg) { struct sem_array *sma; int err; @@ -974,7 +974,7 @@ asmlinkage long sys_semctl (int semid, i return err; case IPC_RMID: case IPC_SET: - err = semctl_down(ns,semid,semnum,cmd,version,arg); + err = semctl_down(ns, semid, cmd, version, arg); return err; default: return -EINVAL; -- Pierre Peiffer _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Pierre Peiffer
date: 2008-01-29T16:02:34-00:00

[PATCH 2.6.24-rc8-mm1 06/15] IPC: get rid of the use *_setbuf structure.
type: item
description: From: Pierre Peiffer <pierre.peiffer@bull.net> All IPCs make use of an intermetiate *_setbuf structure to handle the IPC_SET command. This is not really needed and, moreover, it complicate a little bit the code. This patch get rid of the use of it and uses directly the semid64_ds/ msgid64_ds/shmid64_ds structure. In addition of removing one struture declaration, it also simplifies and improves a little bit the common 64-bits path. Moreover, this will simplify the code for handling the IPC_SETALL command provided in the next patch. Signed-off-by: Pierre Peiffer <pierre.peiffer@bull.net> Acked-by: Serge Hallyn <serue@us.ibm.com> --- ipc/msg.c | 51 ++++++++++++++++++--------------------------------- ipc/sem.c | 40 ++++++++++++++-------------------------- ipc/shm.c | 41 ++++++++++++++--------------------------- 3 files changed, 46 insertions(+), 86 deletions(-) Index: b/ipc/msg.c =================================================================== --- a/ipc/msg.c +++ b/ipc/msg.c @@ -351,31 +351,14 @@ copy_msqid_to_user(void __user *buf, str } } -struct msq_setbuf { - unsigned long qbytes; - uid_t uid; - gid_t gid; - mode_t mode; -}; - static inline unsigned long -copy_msqid_from_user(struct msq_setbuf *out, void __user *buf, int version) +copy_msqid_from_user(struct msqid64_ds *out, void __user *buf, int version) { switch(version) { case IPC_64: - { - struct msqid64_ds tbuf; - - if (copy_from_user(&tbuf, buf, sizeof(tbuf))) + if (copy_from_user(out, buf, sizeof(*out))) return -EFAULT; - - out->qbytes = tbuf.msg_qbytes; - out->uid = tbuf.msg_perm.uid; - out->gid = tbuf.msg_perm.gid; - out->mode = tbuf.msg_perm.mode; - return 0; - } case IPC_OLD: { struct msqid_ds tbuf_old; @@ -383,14 +366,14 @@ copy_msqid_from_user(struct msq_setbuf * if (copy_from_user(&tbuf_old, buf, sizeof(tbuf_old))) return -EFAULT; - out->uid = tbuf_old.msg_perm.uid; - out->gid = tbuf_old.msg_perm.gid; - out->mode = tbuf_old.msg_perm.mode; + out->msg_perm.uid = tbuf_old.msg_perm.uid; + out->msg_perm.…
creator: Pierre Peiffer
date: 2008-01-29T16:02:35-00:00

[PATCH 2.6.24-rc8-mm1 07/15] IPC: introduce ipc_update_perm()
type: item
description: From: Pierre Peiffer <pierre.peiffer@bull.net> The IPC_SET command performs the same permission setting for all IPCs. This patch introduces a common ipc_update_perm() function to update these permissions and makes use of it for all IPCs. Signed-off-by: Pierre Peiffer <pierre.peiffer@bull.net> Acked-by: Serge Hallyn <serue@us.ibm.com> --- ipc/msg.c | 5 +---- ipc/sem.c | 5 +---- ipc/shm.c | 5 +---- ipc/util.c | 13 +++++++++++++ ipc/util.h | 1 + 5 files changed, 17 insertions(+), 12 deletions(-) Index: b/ipc/msg.c =================================================================== --- a/ipc/msg.c +++ b/ipc/msg.c @@ -447,10 +447,7 @@ static int msgctl_down(struct ipc_namesp msq->q_qbytes = msqid64.msg_qbytes; - ipcp->uid = msqid64.msg_perm.uid; - ipcp->gid = msqid64.msg_perm.gid; - ipcp->mode = (ipcp->mode & ~S_IRWXUGO) | - (S_IRWXUGO & msqid64.msg_perm.mode); + ipc_update_perm(&msqid64.msg_perm, ipcp); msq->q_ctime = get_seconds(); /* sleeping receivers might be excluded by * stricter permissions. Index: b/ipc/sem.c =================================================================== --- a/ipc/sem.c +++ b/ipc/sem.c @@ -915,10 +915,7 @@ static int semctl_down(struct ipc_namesp freeary(ns, ipcp); goto out_up; case IPC_SET: - ipcp->uid = semid64.sem_perm.uid; - ipcp->gid = semid64.sem_perm.gid; - ipcp->mode = (ipcp->mode & ~S_IRWXUGO) - | (semid64.sem_perm.mode & S_IRWXUGO); + ipc_update_perm(&semid64.sem_perm, ipcp); sma->sem_ctime = get_seconds(); break; default: Index: b/ipc/shm.c =================================================================== --- a/ipc/shm.c +++ b/ipc/shm.c @@ -665,10 +665,7 @@ static int shmctl_down(struct ipc_namesp do_shm_rmid(ns, ipcp); goto out_up; case IPC_SET: - ipcp->uid = shmid64.shm_perm.uid; - ipcp->gid = shmid64.shm_perm.gid; - ipcp->mode = (ipcp->mode & ~S_IRWXUGO) - | (shmid64.shm_perm.mode & S_IRWXUGO); + ipc_update_perm(&shmid64.shm_perm, ipcp); shp->shm_ctim = get_seconds(); break; default: Index: b/ipc/util.c =====================…
creator: Pierre Peiffer
date: 2008-01-29T16:02:36-00:00

[PATCH 2.6.24-rc8-mm1 08/15] IPC: consolidate all xxxctl_down() functions
type: item
description: semctl_down(), msgctl_down() and shmctl_down() are used to handle the same set of commands for each kind of IPC. They all start to do the same job (they retrieve the ipc and do some permission checks) before handling the commands on their own. This patch proposes to consolidate this by moving these same pieces of code into one common function called ipcctl_pre_down(). It simplifies a little these xxxctl_down() functions and increases a little the maintainability. Signed-off-by: Pierre Peiffer <pierre.peiffer@bull.net> Acked-by: Serge Hallyn <serue@us.ibm.com> --- ipc/msg.c | 48 +++++------------------------------------------- ipc/sem.c | 42 ++++-------------------------------------- ipc/shm.c | 42 ++++-------------------------------------- ipc/util.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ ipc/util.h | 2 ++ 5 files changed, 66 insertions(+), 119 deletions(-) Index: b/ipc/sem.c =================================================================== --- a/ipc/sem.c +++ b/ipc/sem.c @@ -142,21 +142,6 @@ void __init sem_init (void) } /* - * This routine is called in the paths where the rw_mutex is held to protect - * access to the idr tree. - */ -static inline struct sem_array *sem_lock_check_down(struct ipc_namespace *ns, - int id) -{ - struct kern_ipc_perm *ipcp = ipc_lock_check_down(&sem_ids(ns), id); - - if (IS_ERR(ipcp)) - return (struct sem_array *)ipcp; - - return container_of(ipcp, struct sem_array, sem_perm); -} - -/* * sem_lock_(check_) routines are called in the paths where the rw_mutex * is not held. */ @@ -880,31 +865,12 @@ static int semctl_down(struct ipc_namesp if (copy_semid_from_user(&semid64, arg.buf, version)) return -EFAULT; } - down_write(&sem_ids(ns).rw_mutex); - sma = sem_lock_check_down(ns, semid); - if (IS_ERR(sma)) { - err = PTR_ERR(sma); - goto out_up; - } - - ipcp = &sma->sem_perm; - err = audit_ipc_obj(ipcp); - if (err) - goto out_unlock; + ipcp = ipcctl_pre_down(&sem_ids(ns), semid, cmd, &semid64.sem_perm, 0); + if (IS_ERR(ipcp…
creator: Pierre Peiffer
date: 2008-01-29T16:02:37-00:00

[PATCH 2.6.24-rc8-mm1 09/15] (RFC) IPC: new kernel API to change an ID
type: item
description: From: Pierre Peiffer <pierre.peiffer@bull.net> This patch provides three new API to change the ID of an existing System V IPCs. These APIs are: long msg_chid(struct ipc_namespace *ns, int id, int newid); long sem_chid(struct ipc_namespace *ns, int id, int newid); long shm_chid(struct ipc_namespace *ns, int id, int newid); They return 0 or an error code in case of failure. They may be useful for setting a specific ID for an IPC when preparing a restart operation. To be successful, the following rules must be respected: - the IPC exists (of course...) - the new ID must satisfy the ID computation rule. - the entry in the idr corresponding to the new ID must be free. Signed-off-by: Pierre Peiffer <pierre.peiffer@bull.net> Acked-by: Serge Hallyn <serue@us.ibm.com> --- include/linux/msg.h | 2 ++ include/linux/sem.h | 2 ++ include/linux/shm.h | 3 +++ ipc/msg.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ ipc/sem.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ ipc/shm.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ ipc/util.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ ipc/util.h | 1 + 8 files changed, 197 insertions(+) Index: b/include/linux/msg.h =================================================================== --- a/include/linux/msg.h +++ b/include/linux/msg.h @@ -63,6 +63,7 @@ struct msginfo { #ifdef __KERNEL__ #include <linux/list.h> +#include <linux/ipc_namespace.h> /* one msg_msg structure for each message */ struct msg_msg { @@ -96,6 +97,7 @@ extern long do_msgsnd(int msqid, long mt size_t msgsz, int msgflg); extern long do_msgrcv(int msqid, long *pmtype, void __user *mtext, size_t msgsz, long msgtyp, int msgflg); +long msg_chid(struct ipc_namespace *ns, int id, int newid); #endif /* __KERNEL__ */ Index: b/include/linux/sem.h =================================================================== --- a/include/linux/sem.h +++ b/include/linux/sem.h @@ -138,9 +138,11 @@ struct sysv_sem { }; #ifdef CONFIG_SYSVIPC +#include <linux/ipc_name…
creator: Pierre Peiffer
date: 2008-01-29T16:02:38-00:00

[PATCH 2.6.24-rc8-mm1 10/15] (RFC) IPC: new IPC_SETID command to modify an ID
type: item
description: From: Pierre Peiffer <pierre.peiffer@bull.net> This patch adds a new IPC_SETID command to the System V IPCs set of commands, which allows to change the ID of an existing IPC. This command can be used through the semctl/shmctl/msgctl API, with the new ID passed as the third argument for msgctl and shmctl (instead of a pointer) and through the fourth argument for semctl. To be successful, the following rules must be respected: - the IPC exists - the user must be allowed to change the IPC attributes regarding the IPC permissions. - the new ID must satisfy the ID computation rule. - the entry (in the kernel internal table of IPCs) corresponding to the new ID must be free. Signed-off-by: Pierre Peiffer <pierre.peiffer@bull.net> Acked-by: Serge Hallyn <serue@us.ibm.com> --- include/linux/ipc.h | 9 +++++---- ipc/compat.c | 3 +++ ipc/msg.c | 27 ++++++++++++++++++++++++++- ipc/sem.c | 27 ++++++++++++++++++++++++++- ipc/shm.c | 27 ++++++++++++++++++++++++++- security/selinux/hooks.c | 3 +++ 6 files changed, 89 insertions(+), 7 deletions(-) Index: b/include/linux/ipc.h =================================================================== --- a/include/linux/ipc.h +++ b/include/linux/ipc.h @@ -35,10 +35,11 @@ struct ipc_perm * Control commands used with semctl, msgctl and shmctl * see also specific commands in sem.h, msg.h and shm.h */ -#define IPC_RMID 0 /* remove resource */ -#define IPC_SET 1 /* set ipc_perm options */ -#define IPC_STAT 2 /* get ipc_perm options */ -#define IPC_INFO 3 /* see ipcs */ +#define IPC_RMID 0 /* remove resource */ +#define IPC_SET 1 /* set ipc_perm options */ +#define IPC_STAT 2 /* get ipc_perm options */ +#define IPC_INFO 3 /* see ipcs */ +#define IPC_SETID 4 /* set ipc ID */ /* * Version flags for semctl, msgctl, and shmctl commands Index: b/ipc/compat.c =================================================================== --- a/ipc/compat.c +++ b/ipc/compat.c @@ -253,6 +253,7 @@ long compat_sys_semctl(int first, int se switch (third & (~IPC_64)) { c…
creator: Pierre Peiffer
date: 2008-01-29T16:02:39-00:00

[PATCH 2.6.24-rc8-mm1 11/15] (RFC) IPC: new IPC_SETALL command to modify all settings
type: item
description: From: Pierre Peiffer <pierre.peiffer@bull.net> This patch adds a new IPC_SETALL command to the System V IPCs set of commands, which allows to change all the settings of an IPC It works exactly the same way as the IPC_SET command, except that it additionally changes all the times and the pids values Signed-off-by: Pierre Peiffer <pierre.peiffer@bull.net> Acked-by: Serge Hallyn <serue@us.ibm.com> --- include/linux/ipc.h | 1 + ipc/compat.c | 3 +++ ipc/msg.c | 15 +++++++++++++-- ipc/sem.c | 10 +++++++++- ipc/shm.c | 13 ++++++++++++- ipc/util.c | 7 ++++++- security/selinux/hooks.c | 3 +++ 7 files changed, 47 insertions(+), 5 deletions(-) Index: b/include/linux/ipc.h =================================================================== --- a/include/linux/ipc.h +++ b/include/linux/ipc.h @@ -40,6 +40,7 @@ struct ipc_perm #define IPC_STAT 2 /* get ipc_perm options */ #define IPC_INFO 3 /* see ipcs */ #define IPC_SETID 4 /* set ipc ID */ +#define IPC_SETALL 5 /* set all parameters */ /* * Version flags for semctl, msgctl, and shmctl commands Index: b/ipc/compat.c =================================================================== --- a/ipc/compat.c +++ b/ipc/compat.c @@ -282,6 +282,7 @@ long compat_sys_semctl(int first, int se err = -EFAULT; break; + case IPC_SETALL: case IPC_SET: if (version == IPC_64) { err = get_compat_semid64_ds(&s64, compat_ptr(pad)); @@ -431,6 +432,7 @@ long compat_sys_msgctl(int first, int se err = sys_msgctl(first, second, uptr); break; + case IPC_SETALL: case IPC_SET: if (version == IPC_64) { err = get_compat_msqid64(&m64, uptr); @@ -621,6 +623,7 @@ long compat_sys_shmctl(int first, int se break; + case IPC_SETALL: case IPC_SET: if (version == IPC_64) { err = get_compat_shmid64_ds(&s64, uptr); Index: b/ipc/msg.c =================================================================== --- a/ipc/msg.c +++ b/ipc/msg.c @@ -426,7 +426,7 @@ static int msgctl_down(struct ipc_namesp struct msg_queue *msq; int err; - if (cmd == IPC_SET) { + if (cmd == IPC_SET || cm…
creator: Pierre Peiffer
date: 2008-01-29T16:02:40-00:00

[PATCH 2.6.24-rc8-mm1 12/15] (RFC) IPC/semaphores: make use of RCU to free the sem_undo_list
type: item
description: From: Pierre Peiffer <pierre.peiffer@bull.net> Today, the sem_undo_list is freed when the last task using it exits. There is no mechanism in place, that allows a safe concurrent access to the sem_undo_list of a target task and protects efficiently against a task-exit. That is okay for now as we don't need this. As I would like to provide a /proc interface to access this data, I need such a safe access, without blocking the target task if possible. This patch proposes to introduce the use of RCU to delay the real free of these sem_undo_list structures. They can then be accessed in a safe manner by any tasks inside read critical section, this way: struct sem_undo_list *undo_list; int ret; ... rcu_read_lock(); undo_list = rcu_dereference(task->sysvsem.undo_list); if (undo_list) ret = atomic_inc_not_zero(&undo_list->refcnt); rcu_read_unlock(); ... if (undo_list && ret) { /* section where undo_list can be used quietly */ ... } ... Signed-off-by: Pierre Peiffer <pierre.peiffer@bull.net> --- include/linux/sem.h | 7 +++++-- ipc/sem.c | 42 ++++++++++++++++++++++++++---------------- 2 files changed, 31 insertions(+), 18 deletions(-) Index: b/include/linux/sem.h =================================================================== --- a/include/linux/sem.h +++ b/include/linux/sem.h @@ -115,7 +115,8 @@ struct sem_queue { }; /* Each task has a list of undo requests. They are executed automatically - * when the process exits. + * when the last refcnt of sem_undo_list is released (ie when the process exits + * in the general case) */ struct sem_undo { struct sem_undo * proc_next; /* next entry on this process */ @@ -125,12 +126,14 @@ struct sem_undo { }; /* sem_undo_list controls shared access to the list of sem_undo structures - * that may be shared among all a CLONE_SYSVSEM task group. + * that may be shared among all a CLONE_SYSVSEM task group or with an external + * process which changes the list through procfs. */ struct sem_undo_list { atomic_t refcnt; spinlock_t lock; struct …
creator: Pierre Peiffer
date: 2008-01-29T16:02:41-00:00

[PATCH 2.6.24-rc8-mm1 13/15] (RFC) IPC/semaphores: per <pid> semundo file in procfs
type: item
description: From: Pierre Peiffer <pierre.peiffer@bull.net> This patch adds a new procfs interface to display the per-process semundo data. A new per-PID file is added, named "semundo". It contains one line per semaphore IPC where there is something to undo for this process. Then, each line contains the semid followed by each undo value corresponding to each semaphores of the semaphores array. This interface will be specially useful to allow a user to access these data, for example for checkpointing a process Signed-off-by: Pierre Peiffer <pierre.peiffer@bull.net> Acked-by: Serge Hallyn <serue@us.ibm.com> --- fs/proc/base.c | 3 + fs/proc/internal.h | 1 ipc/sem.c | 153 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 157 insertions(+) Index: b/fs/proc/base.c =================================================================== --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -2255,6 +2255,9 @@ static const struct pid_entry tgid_base_ #ifdef CONFIG_TASK_IO_ACCOUNTING INF("io", S_IRUGO, pid_io_accounting), #endif +#ifdef CONFIG_SYSVIPC + REG("semundo", S_IRUGO, semundo), +#endif }; static int proc_tgid_base_readdir(struct file * filp, Index: b/fs/proc/internal.h =================================================================== --- a/fs/proc/internal.h +++ b/fs/proc/internal.h @@ -64,6 +64,7 @@ extern const struct file_operations proc extern const struct file_operations proc_smaps_operations; extern const struct file_operations proc_clear_refs_operations; extern const struct file_operations proc_pagemap_operations; +extern const struct file_operations proc_semundo_operations; void free_proc_entry(struct proc_dir_entry *de); Index: b/ipc/sem.c =================================================================== --- a/ipc/sem.c +++ b/ipc/sem.c @@ -1435,4 +1435,157 @@ static int sysvipc_sem_proc_show(struct sma->sem_otime, sma->sem_ctime); } + + +/* iterator */ +static void *semundo_start(struct seq_file *m, loff_t *ppos) +{ + struct sem_undo_list *undo_list = m->priva…
creator: Pierre Peiffer
date: 2008-01-29T16:02:42-00:00

[PATCH 2.6.24-rc8-mm1 14/15] (RFC) IPC/semaphores: prepare semundo code to work on another task than
type: item
description: From: Pierre Peiffer <pierre.peiffer@bull.net> In order to modify the semundo-list of a task from procfs, we must be able to work on any target task. But all the existing code playing with the semundo-list, currently works only on the 'current' task, and does not allow to specify any target task. This patch changes all these routines to allow them to work on a specified task, passed in parameter, instead of current. This is mainly a preparation for the semundo_write() operation, on the /proc/<pid>/semundo file, as provided in the next patch. Signed-off-by: Pierre Peiffer <pierre.peiffer@bull.net> --- ipc/sem.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 68 insertions(+), 22 deletions(-) Index: b/ipc/sem.c =================================================================== --- a/ipc/sem.c +++ b/ipc/sem.c @@ -1017,8 +1017,9 @@ asmlinkage long sys_semctl (int semid, i } /* If the task doesn't already have a undo_list, then allocate one - * here. We guarantee there is only one thread using this undo list, - * and current is THE ONE + * here. + * The target task (tsk) is current in the general case, except when + * accessed from the procfs (ie when writting to /proc/<pid>/semundo) * * If this allocation and assignment succeeds, but later * portions of this code fail, there is no need to free the sem_undo_list. @@ -1026,22 +1027,60 @@ asmlinkage long sys_semctl (int semid, i * at exit time. * * This can block, so callers must hold no locks. + * + * Note: task_lock is used to synchronize 1. several possible concurrent + * creations and 2. the free of the undo_list (done when the task using it + * exits). In the second case, we check the PF_EXITING flag to not create + * an undo_list for a task which has exited. + * If there already is an undo_list for this task, there is no need + * to held the task-lock to retrieve it, as the pointer can not change + * afterwards. */ -static inline int get_undo_list(struct sem_undo_list **undo_listp…
creator: Pierre Peiffer
date: 2008-01-29T16:02:43-00:00

[PATCH 2.6.24-rc8-mm1 15/15] (RFC) IPC/semaphores: add write() operation to semundo file in procfs
type: item
description: From: Pierre Peiffer <pierre.peiffer@bull.net> This patch adds the write operation to the semundo file. This write operation allows root to add or update the semundo list and their values for a given process. The user must provide some lines, each containing the semaphores ID followed by the semaphores values to undo. The operation failed if the given semaphore ID does not exist or if the number of values does not match the number of semaphores in the array. Signed-off-by: Pierre Peiffer <pierre.peiffer@bull.net> --- fs/proc/base.c | 2 ipc/sem.c | 232 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 227 insertions(+), 7 deletions(-) Index: b/fs/proc/base.c =================================================================== --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -2256,7 +2256,7 @@ static const struct pid_entry tgid_base_ INF("io", S_IRUGO, pid_io_accounting), #endif #ifdef CONFIG_SYSVIPC - REG("semundo", S_IRUGO, semundo), + REG("semundo", S_IWUSR|S_IRUGO, semundo), #endif }; Index: b/ipc/sem.c =================================================================== --- a/ipc/sem.c +++ b/ipc/sem.c @@ -1580,6 +1580,9 @@ static struct seq_operations semundo_op /* * semundo_open: open operation for /proc/<PID>/semundo file + * + * If the file is opened in write mode and no semundo list exists for + * this target PID, it is created here. */ static int semundo_open(struct inode *inode, struct file *file) { @@ -1598,18 +1601,31 @@ static int semundo_open(struct inode *in undo_list = rcu_dereference(task->sysvsem.undo_list); if (undo_list) ret = !atomic_inc_not_zero(&undo_list->refcnt); - put_task_struct(task); } rcu_read_unlock(); - if (!task || ret) + if (!task) return -EINVAL; - ret = seq_open(file, &semundo_op); + if (ret) { + put_task_struct(task); + return -EINVAL; + } + + + /* Create an undo_list if needed and if file is opened in write mode */ + if (!undo_list && (file->f_flags & O_WRONLY || file->f_flags & O_RDWR)) + ret = get_undo_list(t…
creator: Pierre Peiffer
date: 2008-01-29T16:02:44-00:00

Re: [PATCH 2.6.24-rc8-mm1 09/15] (RFC) IPC: new kernel API to change an ID
type: item
description: On Tue, Jan 29, 2008 at 05:02:38PM +0100, pierre.peiffer@bull.net wrote: > This patch provides three new API to change the ID of an existing > System V IPCs. > > These APIs are: > long msg_chid(struct ipc_namespace *ns, int id, int newid); > long sem_chid(struct ipc_namespace *ns, int id, int newid); > long shm_chid(struct ipc_namespace *ns, int id, int newid); > > They return 0 or an error code in case of failure. > > They may be useful for setting a specific ID for an IPC when preparing > a restart operation. > > To be successful, the following rules must be respected: > - the IPC exists (of course...) > - the new ID must satisfy the ID computation rule. > - the entry in the idr corresponding to the new ID must be free. > ipc/util.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ > ipc/util.h | 1 + > 8 files changed, 197 insertions(+) For the record, OpenVZ uses "create with predefined ID" method which leads to less code. For example, change at the end is all we want from ipc/util.c . Also, if ids were A and B at the moment of checkpoint, and during restart they became B and A you'll get collision in both ways which you techically can avoid by classic "tmp = A, A = B, B = tmp" but you also can avoid all other loops just by creating with ID you need. _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Alexey Dobriyan
date: 2008-01-29T21:06:56-00:00

Re: [PATCH 2.6.24-rc8-mm1 09/15] (RFC) IPC: new kernel API to change an ID
type: item
description: Alexey Dobriyan wrote: > On Tue, Jan 29, 2008 at 05:02:38PM +0100, pierre.peiffer@bull.net wrote: >> This patch provides three new API to change the ID of an existing >> System V IPCs. >> >> These APIs are: >> long msg_chid(struct ipc_namespace *ns, int id, int newid); >> long sem_chid(struct ipc_namespace *ns, int id, int newid); >> long shm_chid(struct ipc_namespace *ns, int id, int newid); >> >> They return 0 or an error code in case of failure. >> >> They may be useful for setting a specific ID for an IPC when preparing >> a restart operation. >> >> To be successful, the following rules must be respected: >> - the IPC exists (of course...) >> - the new ID must satisfy the ID computation rule. >> - the entry in the idr corresponding to the new ID must be free. > >> ipc/util.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ >> ipc/util.h | 1 + >> 8 files changed, 197 insertions(+) > > For the record, OpenVZ uses "create with predefined ID" method which > leads to less code. For example, change at the end is all we want from > ipc/util.c . > Yes, indeed, I saw that. The idea here is, at the end, to propose a more "userspace oriented" solution. As we can't use msgget(), etc, API to specify an ID, I think we can at least change it afterwards > Also, if ids were A and B at the moment of checkpoint, and during > restart they became B and A you'll get collision in both ways which you > techically can avoid by classic "tmp = A, A = B, B = tmp" In the general case, yes, you're right. In the case of the checkpoint/restart, this is not necessarily a problem, as we will probably restart an application in an empty "container"/"namespace"; Thus we can create all needed IPCs in an empty IPC namespace like this: 1. create first IPC 2. change its ID 3. create the second IPC 4. change its ID 5. etc.. But yes, I agree that if we can directly create an IPC with the right ID, it would be better; may be with an IPC_CREATE command or something like that if the direction is to do …
creator: Pierre Peiffer
date: 2008-01-30T09:52:14-00:00

Re: [PATCH 2.6.24-rc8-mm1 12/15] (RFC) IPC/semaphores: make use of RCU to free the sem_undo_list
type: item
description: Quoting pierre.peiffer@bull.net (pierre.peiffer@bull.net): > From: Pierre Peiffer <pierre.peiffer@bull.net> > > Today, the sem_undo_list is freed when the last task using it exits. > There is no mechanism in place, that allows a safe concurrent access to > the sem_undo_list of a target task and protects efficiently against a > task-exit. > > That is okay for now as we don't need this. > > As I would like to provide a /proc interface to access this data, I need > such a safe access, without blocking the target task if possible. > > This patch proposes to introduce the use of RCU to delay the real free of > these sem_undo_list structures. They can then be accessed in a safe manner > by any tasks inside read critical section, this way: > > struct sem_undo_list *undo_list; > int ret; > ... > rcu_read_lock(); > undo_list = rcu_dereference(task->sysvsem.undo_list); > if (undo_list) > ret = atomic_inc_not_zero(&undo_list->refcnt); > rcu_read_unlock(); > ... > if (undo_list && ret) { > /* section where undo_list can be used quietly */ > ... > } > ... And of course then if (atomic_dec_and_test(&undo_list->refcnt)) free_semundo_list(undo_list); by that task. > > Signed-off-by: Pierre Peiffer <pierre.peiffer@bull.net> Looks correct in terms of locking/refcounting. Signed-off-by: Serge Hallyn <serue@us.ibm.com> thanks, -serge > --- > > include/linux/sem.h | 7 +++++-- > ipc/sem.c | 42 ++++++++++++++++++++++++++---------------- > 2 files changed, 31 insertions(+), 18 deletions(-) > > Index: b/include/linux/sem.h > =================================================================== > --- a/include/linux/sem.h > +++ b/include/linux/sem.h > @@ -115,7 +115,8 @@ struct sem_queue { > }; > > /* Each task has a list of undo requests. They are executed automatically > - * when the process exits. > + * when the last refcnt of sem_undo_list is released (ie when the process exits > + * in the general case) > */ > struct sem_undo { > struct sem_undo * proc_next; /* next entry on this process …
creator: serue
date: 2008-01-30T21:26:50-00:00

Re: [PATCH 2.6.24-rc8-mm1 14/15] (RFC) IPC/semaphores: prepare semundo code to work on another task
type: item
description: Quoting pierre.peiffer@bull.net (pierre.peiffer@bull.net): > From: Pierre Peiffer <pierre.peiffer@bull.net> > > In order to modify the semundo-list of a task from procfs, we must be able to > work on any target task. > But all the existing code playing with the semundo-list, currently works > only on the 'current' task, and does not allow to specify any target task. > > This patch changes all these routines to allow them to work on a specified > task, passed in parameter, instead of current. > > This is mainly a preparation for the semundo_write() operation, on the > /proc/<pid>/semundo file, as provided in the next patch. > > Signed-off-by: Pierre Peiffer <pierre.peiffer@bull.net> > --- > > ipc/sem.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++---------------- > 1 file changed, 68 insertions(+), 22 deletions(-) > > Index: b/ipc/sem.c > =================================================================== > --- a/ipc/sem.c > +++ b/ipc/sem.c > @@ -1017,8 +1017,9 @@ asmlinkage long sys_semctl (int semid, i > } > > /* If the task doesn't already have a undo_list, then allocate one > - * here. We guarantee there is only one thread using this undo list, > - * and current is THE ONE > + * here. > + * The target task (tsk) is current in the general case, except when > + * accessed from the procfs (ie when writting to /proc/<pid>/semundo) > * > * If this allocation and assignment succeeds, but later > * portions of this code fail, there is no need to free the sem_undo_list. > @@ -1026,22 +1027,60 @@ asmlinkage long sys_semctl (int semid, i > * at exit time. > * > * This can block, so callers must hold no locks. > + * > + * Note: task_lock is used to synchronize 1. several possible concurrent > + * creations and 2. the free of the undo_list (done when the task using it > + * exits). In the second case, we check the PF_EXITING flag to not create > + * an undo_list for a task which has exited. > + * If there already is an undo_list for this task, there is no need > + * to…
creator: serue
date: 2008-01-30T21:44:30-00:00

Re: [PATCH 2.6.24-rc8-mm1 05/15] IPC/semaphores: remove one unused parameter from semctl_down()
type: item
description: pierre.peiffer@bull.net wrote: > From: Pierre Peiffer <pierre.peiffer@bull.net> > > semctl_down() takes one unused parameter: semnum. > This patch proposes to get rid of it. > > Signed-off-by: Pierre Peiffer <pierre.peiffer@bull.net> > Acked-by: Serge Hallyn <serue@us.ibm.com> > --- > ipc/sem.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > Index: b/ipc/sem.c > =================================================================== > --- a/ipc/sem.c > +++ b/ipc/sem.c > @@ -882,8 +882,8 @@ static inline unsigned long copy_semid_f > * to be held in write mode. > * NOTE: no locks must be held, the rw_mutex is taken inside this function. > */ > -static int semctl_down(struct ipc_namespace *ns, int semid, int semnum, > - int cmd, int version, union semun arg) > +static int semctl_down(struct ipc_namespace *ns, int semid, > + int cmd, int version, union semun arg) > { > struct sem_array *sma; > int err; > @@ -974,7 +974,7 @@ asmlinkage long sys_semctl (int semid, i > return err; > case IPC_RMID: > case IPC_SET: > - err = semctl_down(ns,semid,semnum,cmd,version,arg); > + err = semctl_down(ns, semid, cmd, version, arg); > return err; > default: > return -EINVAL; > Looks like semnum is only used in semctl_main(). Why not removing it from semctl_nolock() too? Regards, Nadia _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Nadia Derbey
date: 2008-01-31T08:32:46-00:00

Re: [PATCH 2.6.24-rc8-mm1 09/15] (RFC) IPC: new kernel API to change an ID
type: item
description: Hi again, Thinking more about this, I think I must clarify why I choose this way. In fact, the idea of these patches is to provide the missing user APIs (or extend the existing ones) that allow to set or update _all_ properties of all IPCs, as needed in the case of the checkpoint/restart of an application (the current user API does not allow to specify an ID for a created IPC, for example). And this, without changing the existing API of course. And msgget(), semget() and shmget() does not have any parameter we can use to specify an ID. That's why I've decided to not change these routines and add a new control command, IP_SETID, with which we can can change the ID of an IPC. (that looks to me more straightforward and logical) Now, this patch is, in fact, only a preparation for the patch 10/15 which really complete the user API by adding this IPC_SETID command. (... continuing below ...) Alexey Dobriyan wrote: > On Tue, Jan 29, 2008 at 05:02:38PM +0100, pierre.peiffer@bull.net wrote: >> This patch provides three new API to change the ID of an existing >> System V IPCs. >> >> These APIs are: >> long msg_chid(struct ipc_namespace *ns, int id, int newid); >> long sem_chid(struct ipc_namespace *ns, int id, int newid); >> long shm_chid(struct ipc_namespace *ns, int id, int newid); >> >> They return 0 or an error code in case of failure. >> >> They may be useful for setting a specific ID for an IPC when preparing >> a restart operation. >> >> To be successful, the following rules must be respected: >> - the IPC exists (of course...) >> - the new ID must satisfy the ID computation rule. >> - the entry in the idr corresponding to the new ID must be free. > >> ipc/util.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ >> ipc/util.h | 1 + >> 8 files changed, 197 insertions(+) > > For the record, OpenVZ uses "create with predefined ID" method which > leads to less code. For example, change at the end is all we want from > ipc/util.c . And in fact, you do that from kernel s…
creator: Pierre Peiffer
date: 2008-01-31T09:00:55-00:00

Re: [PATCH 2.6.24-rc8-mm1 14/15] (RFC) IPC/semaphores: prepare semundo code to work on another task
type: item
description: Serge E. Hallyn wrote: > Quoting pierre.peiffer@bull.net (pierre.peiffer@bull.net): >> From: Pierre Peiffer <pierre.peiffer@bull.net> >> >> In order to modify the semundo-list of a task from procfs, we must be able to >> work on any target task. >> But all the existing code playing with the semundo-list, currently works >> only on the 'current' task, and does not allow to specify any target task. >> >> This patch changes all these routines to allow them to work on a specified >> task, passed in parameter, instead of current. >> >> This is mainly a preparation for the semundo_write() operation, on the >> /proc/<pid>/semundo file, as provided in the next patch. >> >> Signed-off-by: Pierre Peiffer <pierre.peiffer@bull.net> >> --- >> >> ipc/sem.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++---------------- >> 1 file changed, 68 insertions(+), 22 deletions(-) >> >> Index: b/ipc/sem.c >> =================================================================== >> --- a/ipc/sem.c >> +++ b/ipc/sem.c >> @@ -1017,8 +1017,9 @@ asmlinkage long sys_semctl (int semid, i >> } >> >> /* If the task doesn't already have a undo_list, then allocate one >> - * here. We guarantee there is only one thread using this undo list, >> - * and current is THE ONE >> + * here. >> + * The target task (tsk) is current in the general case, except when >> + * accessed from the procfs (ie when writting to /proc/<pid>/semundo) >> * >> * If this allocation and assignment succeeds, but later >> * portions of this code fail, there is no need to free the sem_undo_list. >> @@ -1026,22 +1027,60 @@ asmlinkage long sys_semctl (int semid, i >> * at exit time. >> * >> * This can block, so callers must hold no locks. >> + * >> + * Note: task_lock is used to synchronize 1. several possible concurrent >> + * creations and 2. the free of the undo_list (done when the task using it >> + * exits). In the second case, we check the PF_EXITING flag to not create >> + * an undo_list for a task which has exited. >> + * If …
creator: Pierre Peiffer
date: 2008-01-31T09:48:56-00:00

Re: [PATCH 2.6.24-rc8-mm1 12/15] (RFC) IPC/semaphores: make use of RCU to free the sem_undo_list
type: item
description: Serge E. Hallyn wrote: > Quoting pierre.peiffer@bull.net (pierre.peiffer@bull.net): >> From: Pierre Peiffer <pierre.peiffer@bull.net> >> >> Today, the sem_undo_list is freed when the last task using it exits. >> There is no mechanism in place, that allows a safe concurrent access to >> the sem_undo_list of a target task and protects efficiently against a >> task-exit. >> >> That is okay for now as we don't need this. >> >> As I would like to provide a /proc interface to access this data, I need >> such a safe access, without blocking the target task if possible. >> >> This patch proposes to introduce the use of RCU to delay the real free of >> these sem_undo_list structures. They can then be accessed in a safe manner >> by any tasks inside read critical section, this way: >> >> struct sem_undo_list *undo_list; >> int ret; >> ... >> rcu_read_lock(); >> undo_list = rcu_dereference(task->sysvsem.undo_list); >> if (undo_list) >> ret = atomic_inc_not_zero(&undo_list->refcnt); >> rcu_read_unlock(); >> ... >> if (undo_list && ret) { >> /* section where undo_list can be used quietly */ >> ... >> } >> ... > > And of course then > > if (atomic_dec_and_test(&undo_list->refcnt)) > free_semundo_list(undo_list); > > by that task. > I will precise this too. >> Signed-off-by: Pierre Peiffer <pierre.peiffer@bull.net> > > Looks correct in terms of locking/refcounting. > > Signed-off-by: Serge Hallyn <serue@us.ibm.com> > Thanks ! -- Pierre _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Pierre Peiffer
date: 2008-01-31T09:52:08-00:00

Re: [PATCH 2.6.24-rc8-mm1 09/15] (RFC) IPC: new kernel API to change an ID
type: item
description: Why user space can need this API? for checkpointing only? Then I would not consider it for inclusion until it is clear how to implement checkpointing. As for me personally - I'm against exporting such APIs, since they are not needed in real-life user space applications and maintaining it forever for compatibility doesn't worth it. Also such APIs allow creation of non-GPL checkpointing in user-space, which can be of concern as well. Kirill Pierre Peiffer wrote: > Hi again, > > Thinking more about this, I think I must clarify why I choose this way. > In fact, the idea of these patches is to provide the missing user APIs (or > extend the existing ones) that allow to set or update _all_ properties of all > IPCs, as needed in the case of the checkpoint/restart of an application (the > current user API does not allow to specify an ID for a created IPC, for > example). And this, without changing the existing API of course. > > And msgget(), semget() and shmget() does not have any parameter we can use to > specify an ID. > That's why I've decided to not change these routines and add a new control > command, IP_SETID, with which we can can change the ID of an IPC. (that looks to > me more straightforward and logical) > > Now, this patch is, in fact, only a preparation for the patch 10/15 which > really complete the user API by adding this IPC_SETID command. > > (... continuing below ...) > > Alexey Dobriyan wrote: >> On Tue, Jan 29, 2008 at 05:02:38PM +0100, pierre.peiffer@bull.net wrote: >>> This patch provides three new API to change the ID of an existing >>> System V IPCs. >>> >>> These APIs are: >>> long msg_chid(struct ipc_namespace *ns, int id, int newid); >>> long sem_chid(struct ipc_namespace *ns, int id, int newid); >>> long shm_chid(struct ipc_namespace *ns, int id, int newid); >>> >>> They return 0 or an error code in case of failure. >>> >>> They may be useful for setting a specific ID for an IPC when preparing >>> a restart operation. >>> >>> To be successful, t…
creator: dev
date: 2008-01-31T09:54:10-00:00

Re: [PATCH 2.6.24-rc8-mm1 05/15] IPC/semaphores: remove one unused parameter from semctl_down()
type: item
description: Nadia Derbey wrote: > pierre.peiffer@bull.net wrote: >> From: Pierre Peiffer <pierre.peiffer@bull.net> >> >> semctl_down() takes one unused parameter: semnum. >> This patch proposes to get rid of it. >> >> Signed-off-by: Pierre Peiffer <pierre.peiffer@bull.net> >> Acked-by: Serge Hallyn <serue@us.ibm.com> >> --- >> ipc/sem.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> Index: b/ipc/sem.c >> =================================================================== >> --- a/ipc/sem.c >> +++ b/ipc/sem.c >> @@ -882,8 +882,8 @@ static inline unsigned long copy_semid_f >> * to be held in write mode. >> * NOTE: no locks must be held, the rw_mutex is taken inside this >> function. >> */ >> -static int semctl_down(struct ipc_namespace *ns, int semid, int semnum, >> - int cmd, int version, union semun arg) >> +static int semctl_down(struct ipc_namespace *ns, int semid, >> + int cmd, int version, union semun arg) >> { >> struct sem_array *sma; >> int err; >> @@ -974,7 +974,7 @@ asmlinkage long sys_semctl (int semid, i >> return err; >> case IPC_RMID: >> case IPC_SET: >> - err = semctl_down(ns,semid,semnum,cmd,version,arg); >> + err = semctl_down(ns, semid, cmd, version, arg); >> return err; >> default: >> return -EINVAL; >> > > Looks like semnum is only used in semctl_main(). Why not removing it > from semctl_nolock() too? Indeed. In fact, I already fixed that in a previous patch, included in -mm since kernel 2.6.24.rc3-mm2 (patch named ipc-semaphores-consolidate-sem_stat-and.patch) -- Pierre _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Pierre Peiffer
date: 2008-01-31T10:18:30-00:00

Re: [PATCH 2.6.24-rc8-mm1 05/15] IPC/semaphores: remove one unused parameter from semctl_down()
type: item
description: Pierre Peiffer wrote: > > Nadia Derbey wrote: > >>pierre.peiffer@bull.net wrote: >> >>>From: Pierre Peiffer <pierre.peiffer@bull.net> >>> >>>semctl_down() takes one unused parameter: semnum. >>>This patch proposes to get rid of it. >>> >>>Signed-off-by: Pierre Peiffer <pierre.peiffer@bull.net> >>>Acked-by: Serge Hallyn <serue@us.ibm.com> >>>--- >>> ipc/sem.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>>Index: b/ipc/sem.c >>>=================================================================== >>>--- a/ipc/sem.c >>>+++ b/ipc/sem.c >>>@@ -882,8 +882,8 @@ static inline unsigned long copy_semid_f >>> * to be held in write mode. >>> * NOTE: no locks must be held, the rw_mutex is taken inside this >>>function. >>> */ >>>-static int semctl_down(struct ipc_namespace *ns, int semid, int semnum, >>>- int cmd, int version, union semun arg) >>>+static int semctl_down(struct ipc_namespace *ns, int semid, >>>+ int cmd, int version, union semun arg) >>> { >>> struct sem_array *sma; >>> int err; >>>@@ -974,7 +974,7 @@ asmlinkage long sys_semctl (int semid, i >>> return err; >>> case IPC_RMID: >>> case IPC_SET: >>>- err = semctl_down(ns,semid,semnum,cmd,version,arg); >>>+ err = semctl_down(ns, semid, cmd, version, arg); >>> return err; >>> default: >>> return -EINVAL; >>> >> >>Looks like semnum is only used in semctl_main(). Why not removing it >>from semctl_nolock() too? > > > Indeed. > In fact, I already fixed that in a previous patch, included in -mm since kernel > 2.6.24.rc3-mm2 (patch named ipc-semaphores-consolidate-sem_stat-and.patch) > Oops.. Sorry for the "noise"! _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Nadia Derbey
date: 2008-01-31T11:30:51-00:00

Re: [PATCH 2.6.24-rc8-mm1 09/15] (RFC) IPC: new kernel API to change an ID
type: item
description: Kirill Korotaev wrote: > Why user space can need this API? for checkpointing only? I would say "at least for checkpointing"... ;) May be someone else may find an interest about this for something else. In fact, I'm sure that you have some interest in checkpointing; and thus, you have probably some ideas in mind; but whatever the solution you will propose, I'm pretty sure that I could say the same thing for your solution. And what I finally think is: even if it's for "checkpointing only", if many people are interested by this, it may be sufficient to push this ? > Then I would not consider it for inclusion until it is clear how to implement checkpointing. > As for me personally - I'm against exporting such APIs, since they are not needed in real-life user space applications and maintaining it forever for compatibility doesn't worth it. Maintaining these patches is not a big deal, really, but this is not the main point; the "need in real life" (1) is in fact the main one, and then, the "is this solution the best one ?" (2) the second one. About (1), as said in my first mail, as the namespaces and containers are being integrated into the mainline kernel, checkpoint/restart is (or will be) the next need. About (2), my solution propose to do that, as much as possible from userspace, to minimize the kernel impact. Of course, this is subject to discussion. My opinion is that doing a full checkpoint/restart from kernel space will need lot of new specific and intrusive code; I'm not sure that this will be acceptable by the community. But this is my opinion only. Discusion is opened. > Also such APIs allow creation of non-GPL checkpointing in user-space, which can be of concern as well. Honestly, I don't think this really a concern at all. I mean: I've never seen "this allows non-GPL binary and thus, this is bad" as an argument to reject a functionality, but I may be wrong, and thus, it can be discussed as well. I think the points (1) and (2) as stated above are the key ones.…
creator: Pierre Peiffer
date: 2008-01-31T11:57:00-00:00

Re: [PATCH 2.6.24-rc8-mm1 09/15] (RFC) IPC: new kernel API to change an ID
type: item
description: Pierre, my point is that after you've added interface "set IPCID", you'll need more and more for checkpointing: - "create/setup conntrack" (otherwise connections get dropped), - "set task start time" (needed for Oracle checkpointing BTW), - "set some statistics counters (e.g. networking or taskstats)" - "restore inotify" and so on and so forth. Exporting such intimate kernel interfaces to user space doesn't look sane. Exactly from compatibility and maintenance POV. You'll be burden with supporting them for a long time. Remember recent story with SLUB and /proc/slabinfo? Hope I made my argument more clear this time. Thanks, Kirill Pierre Peiffer wrote: > > Kirill Korotaev wrote: >> Why user space can need this API? for checkpointing only? > > I would say "at least for checkpointing"... ;) May be someone else may find an > interest about this for something else. > In fact, I'm sure that you have some interest in checkpointing; and thus, you > have probably some ideas in mind; but whatever the solution you will propose, > I'm pretty sure that I could say the same thing for your solution. > And what I finally think is: even if it's for "checkpointing only", if many > people are interested by this, it may be sufficient to push this ? > >> Then I would not consider it for inclusion until it is clear how to implement checkpointing. >> As for me personally - I'm against exporting such APIs, since they are not needed in real-life user space applications and maintaining it forever for compatibility doesn't worth it. > > Maintaining these patches is not a big deal, really, but this is not the main > point; the "need in real life" (1) is in fact the main one, and then, the "is > this solution the best one ?" (2) the second one. > > About (1), as said in my first mail, as the namespaces and containers are being > integrated into the mainline kernel, checkpoint/restart is (or will be) the next > need. > About (2), my solution propose to do that, as much as possible from userspace…
creator: dev
date: 2008-01-31T13:11:26-00:00

Re: [PATCH 2.6.24-rc8-mm1 09/15] (RFC) IPC: new kernel API to change an ID
type: item
description: Hello Kirill ! Kirill Korotaev wrote: > Pierre, > > my point is that after you've added interface "set IPCID", you'll need more and more for checkpointing: > - "create/setup conntrack" (otherwise connections get dropped), > - "set task start time" (needed for Oracle checkpointing BTW), > - "set some statistics counters (e.g. networking or taskstats)" > - "restore inotify" > and so on and so forth. right. we know that we will have to handle a lot of these and more and we will need an API for it :) so how should we handle it ? through a dedicated syscall that would be able to checkpoint and/or restart a process, an ipc object, an ipc namespace, a full container ? will it take a fd or a big binary blob ? I personally really liked Pavel idea's of filesystem. but we dropped the thread. that's for the user API but we will need also kernel services to expose (checkpoint) states and restore them. If it's too early to talk about the user API, we could try first to refactor the kernel internals to expose correctly what we need. That's what Pierre's patchset is trying to do. Cheers, C. _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Cedric Le Goater
date: 2008-01-31T16:10:03-00:00

Re: [PATCH 2.6.24-rc8-mm1 14/15] (RFC) IPC/semaphores: prepare semundo code to work on another task
type: item
description: Quoting Pierre Peiffer (pierre.peiffer@bull.net): > > > Serge E. Hallyn wrote: > > Quoting pierre.peiffer@bull.net (pierre.peiffer@bull.net): > >> From: Pierre Peiffer <pierre.peiffer@bull.net> > >> > >> In order to modify the semundo-list of a task from procfs, we must be able to > >> work on any target task. > >> But all the existing code playing with the semundo-list, currently works > >> only on the 'current' task, and does not allow to specify any target task. > >> > >> This patch changes all these routines to allow them to work on a specified > >> task, passed in parameter, instead of current. > >> > >> This is mainly a preparation for the semundo_write() operation, on the > >> /proc/<pid>/semundo file, as provided in the next patch. > >> > >> Signed-off-by: Pierre Peiffer <pierre.peiffer@bull.net> > >> --- > >> > >> ipc/sem.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++---------------- > >> 1 file changed, 68 insertions(+), 22 deletions(-) > >> > >> Index: b/ipc/sem.c > >> =================================================================== > >> --- a/ipc/sem.c > >> +++ b/ipc/sem.c > >> @@ -1017,8 +1017,9 @@ asmlinkage long sys_semctl (int semid, i > >> } > >> > >> /* If the task doesn't already have a undo_list, then allocate one > >> - * here. We guarantee there is only one thread using this undo list, > >> - * and current is THE ONE > >> + * here. > >> + * The target task (tsk) is current in the general case, except when > >> + * accessed from the procfs (ie when writting to /proc/<pid>/semundo) > >> * > >> * If this allocation and assignment succeeds, but later > >> * portions of this code fail, there is no need to free the sem_undo_list. > >> @@ -1026,22 +1027,60 @@ asmlinkage long sys_semctl (int semid, i > >> * at exit time. > >> * > >> * This can block, so callers must hold no locks. > >> + * > >> + * Note: task_lock is used to synchronize 1. several possible concurrent > >> + * creations and 2. the free of the undo_list (done when the task usin…
creator: serue
date: 2008-01-31T18:01:25-00:00

Re: [PATCH 2.6.24-rc8-mm1 14/15] (RFC) IPC/semaphores: prepare semundo code to work on another task
type: item
description: Serge E. Hallyn wrote: > Quoting Pierre Peiffer (pierre.peiffer@bull.net): >> >> Serge E. Hallyn wrote: >>> Quoting pierre.peiffer@bull.net (pierre.peiffer@bull.net): >>>> From: Pierre Peiffer <pierre.peiffer@bull.net> >>>> >>>> In order to modify the semundo-list of a task from procfs, we must be able to >>>> work on any target task. >>>> But all the existing code playing with the semundo-list, currently works >>>> only on the 'current' task, and does not allow to specify any target task. >>>> >>>> This patch changes all these routines to allow them to work on a specified >>>> task, passed in parameter, instead of current. >>>> >>>> This is mainly a preparation for the semundo_write() operation, on the >>>> /proc/<pid>/semundo file, as provided in the next patch. >>>> >>>> Signed-off-by: Pierre Peiffer <pierre.peiffer@bull.net> >>>> --- >>>> >>>> ipc/sem.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++---------------- >>>> 1 file changed, 68 insertions(+), 22 deletions(-) >>>> >>>> Index: b/ipc/sem.c >>>> =================================================================== >>>> --- a/ipc/sem.c >>>> +++ b/ipc/sem.c >>>> @@ -1017,8 +1017,9 @@ asmlinkage long sys_semctl (int semid, i >>>> } >>>> >>>> /* If the task doesn't already have a undo_list, then allocate one >>>> - * here. We guarantee there is only one thread using this undo list, >>>> - * and current is THE ONE >>>> + * here. >>>> + * The target task (tsk) is current in the general case, except when >>>> + * accessed from the procfs (ie when writting to /proc/<pid>/semundo) >>>> * >>>> * If this allocation and assignment succeeds, but later >>>> * portions of this code fail, there is no need to free the sem_undo_list. >>>> @@ -1026,22 +1027,60 @@ asmlinkage long sys_semctl (int semid, i >>>> * at exit time. >>>> * >>>> * This can block, so callers must hold no locks. >>>> + * >>>> + * Note: task_lock is used to synchronize 1. several possible concurrent >>>> + * creations and 2. the free of the undo_list…
creator: Pierre Peiffer
date: 2008-02-01T12:09:48-00:00

Re: [PATCH 2.6.24-rc8-mm1 00/15] IPC: code rewrite + new functionalities
type: item
description: Hi! > * Patches 9 to 15 propose to add some functionalities, and thus are > submitted here for RFC, about both the interest and their implementation. > These functionalities are: > - Two new control-commands: > . IPC_SETID: to change an IPC's id. > . IPC_SETALL: behaves as IPC_SET, except that it also sets all time > and pid values) > - add a /proc/<pid>/semundo file to read and write the undo values of > some semaphores for a given process. > > As the namespaces and the "containers" are being integrated in the > kernel, these functionalities may be a first step to implement the > checkpoint/restart of an application: in fact the existing API does not allow > to specify or to change an ID when creating an IPC, when restarting an > application, and the times/pids values of each IPCs are also altered. May be > someone may find another interest about this ? > > So again, comments are welcome. Checkpoint/restart is nice, but... sysV ipc is broken by design, do we really want to extend it? Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Pavel Machek
date: 2008-02-02T18:23:52-00:00

Re: [PATCH 2.6.24-rc8-mm1 09/15] (RFC) IPC: new kernel API to change an ID
type: item
description: Cedric Le Goater wrote: > Hello Kirill ! > > Kirill Korotaev wrote: >> Pierre, >> >> my point is that after you've added interface "set IPCID", you'll need >> more and more for checkpointing: >> - "create/setup conntrack" (otherwise connections get dropped), >> - "set task start time" (needed for Oracle checkpointing BTW), >> - "set some statistics counters (e.g. networking or taskstats)" >> - "restore inotify" >> and so on and so forth. > > right. we know that we will have to handle a lot of these > and more and we will need an API for it :) so how should we handle it ? > through a dedicated syscall that would be able to checkpoint and/or > restart a process, an ipc object, an ipc namespace, a full container ? > will it take a fd or a big binary blob ? > I personally really liked Pavel idea's of filesystem. but we dropped the > thread. Imho having a file system interface means having all its problems. Imagine you have some information about tasks exported with a file system interface. Obviously to collect the information you have to hold some spinlock like tasklist_lock or similar. Obviously, you have to drop the lock between sys_read() syscalls. So interface gets much more complicated - you have to rescan the objects and somehow find the place where you stopped previous read. Or you have to to force reader to read everything at once. > that's for the user API but we will need also kernel services to expose > (checkpoint) states and restore them. If it's too > early to talk about the user API, we could try first to refactor > the kernel internals to expose correctly what we need. That's what I would start with. > That's what Pierre's patchset is trying to do. Not exactly. For checkpointing/restoring we actually need only one new API call for each subsystem - create some object with given ID (and maybe parameters, if they are not dynamically changeable by user). While Pierre's patchset adds different API call - change object ID. Thanks, Kirill ______________________…
creator: dev
date: 2008-02-04T13:41:26-00:00

Re: [PATCH 2.6.24-rc8-mm1 00/15] IPC: code rewrite + new functionalities
type: item
description: Pavel Machek wrote: > Hi! > >> * Patches 9 to 15 propose to add some functionalities, and thus are >> submitted here for RFC, about both the interest and their implementation. >> These functionalities are: >> - Two new control-commands: >> . IPC_SETID: to change an IPC's id. >> . IPC_SETALL: behaves as IPC_SET, except that it also sets all time >> and pid values) >> - add a /proc/<pid>/semundo file to read and write the undo values of >> some semaphores for a given process. >> >> As the namespaces and the "containers" are being integrated in the >> kernel, these functionalities may be a first step to implement the >> checkpoint/restart of an application: in fact the existing API does not allow >> to specify or to change an ID when creating an IPC, when restarting an >> application, and the times/pids values of each IPCs are also altered. May be >> someone may find another interest about this ? >> >> So again, comments are welcome. > > Checkpoint/restart is nice, but... sysV ipc is broken by design, do we > really want to extend it? If we want to support all kind of applications, yes, we must also support SysVipc. We must support all kernel subsystems at the end. I've started with IPC, because it's relatively simple and isolated. -- Pierre _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Pierre Peiffer
date: 2008-02-04T13:52:17-00:00

Re: Re: [PATCH 2.6.24-rc8-mm1 09/15] (RFC) IPC: new kernel API to change an ID
type: item
description: Kirill Korotaev wrote: > > Cedric Le Goater wrote: >> Hello Kirill ! >> >> Kirill Korotaev wrote: >>> Pierre, >>> >>> my point is that after you've added interface "set IPCID", you'll need >>> more and more for checkpointing: >>> - "create/setup conntrack" (otherwise connections get dropped), >>> - "set task start time" (needed for Oracle checkpointing BTW), >>> - "set some statistics counters (e.g. networking or taskstats)" >>> - "restore inotify" >>> and so on and so forth. >> right. we know that we will have to handle a lot of these >> and more and we will need an API for it :) so how should we handle it ? >> through a dedicated syscall that would be able to checkpoint and/or >> restart a process, an ipc object, an ipc namespace, a full container ? >> will it take a fd or a big binary blob ? >> I personally really liked Pavel idea's of filesystem. but we dropped the >> thread. > > Imho having a file system interface means having all its problems. > Imagine you have some information about tasks exported with a file system interface. > Obviously to collect the information you have to hold some spinlock like tasklist_lock or similar. > Obviously, you have to drop the lock between sys_read() syscalls. > So interface gets much more complicated - you have to rescan the objects and somehow find the place where > you stopped previous read. Or you have to to force reader to read everything at once. To remember the place when we stopped previous read we have a "pos" counter on the struct file. Actually, tar utility, that I propose to perform the most simple migration reads the directory contents with 4Kb buffer - that's enough for ~500 tasks. Besides, is this a real problem for a frozen container? >> that's for the user API but we will need also kernel services to expose >> (checkpoint) states and restore them. If it's too >> early to talk about the user API, we could try first to refactor >> the kernel internals to expose correctly what we need. > > That's what I would st…
creator: Pavel Emelianov
date: 2008-02-04T14:06:23-00:00

Re: Re: [PATCH 2.6.24-rc8-mm1 09/15] (RFC) IPC: new kernel API to change an ID
type: item
description: Pavel Emelyanov wrote: > Kirill Korotaev wrote: >> Cedric Le Goater wrote: >>> Hello Kirill ! >>> >>> Kirill Korotaev wrote: >>>> Pierre, >>>> >>>> my point is that after you've added interface "set IPCID", you'll need >>>> more and more for checkpointing: >>>> - "create/setup conntrack" (otherwise connections get dropped), >>>> - "set task start time" (needed for Oracle checkpointing BTW), >>>> - "set some statistics counters (e.g. networking or taskstats)" >>>> - "restore inotify" >>>> and so on and so forth. >>> right. we know that we will have to handle a lot of these >>> and more and we will need an API for it :) so how should we handle it ? >>> through a dedicated syscall that would be able to checkpoint and/or >>> restart a process, an ipc object, an ipc namespace, a full container ? >>> will it take a fd or a big binary blob ? >>> I personally really liked Pavel idea's of filesystem. but we dropped the >>> thread. >> Imho having a file system interface means having all its problems. >> Imagine you have some information about tasks exported with a file system interface. >> Obviously to collect the information you have to hold some spinlock like tasklist_lock or similar. >> Obviously, you have to drop the lock between sys_read() syscalls. >> So interface gets much more complicated - you have to rescan the objects and somehow find the place where >> you stopped previous read. Or you have to to force reader to read everything at once. > > To remember the place when we stopped previous read we have a "pos" counter > on the struct file. > > Actually, tar utility, that I propose to perform the most simple migration > reads the directory contents with 4Kb buffer - that's enough for ~500 tasks. > > Besides, is this a real problem for a frozen container? I like the idea of a C/R filesystem. Does it implies a specific user space program to orchestrate the checkpoint/restart of the different subsystems ? I mean the checkpoint is easy but what about the restart ? We must…
creator: Daniel Lezcano
date: 2008-02-04T15:00:33-00:00

Re: Re: [PATCH 2.6.24-rc8-mm1 09/15] (RFC) IPC: new kernel API to change an ID
type: item
description: Daniel Lezcano wrote: > Pavel Emelyanov wrote: >> Kirill Korotaev wrote: >>> Cedric Le Goater wrote: >>>> Hello Kirill ! >>>> >>>> Kirill Korotaev wrote: >>>>> Pierre, >>>>> >>>>> my point is that after you've added interface "set IPCID", you'll need >>>>> more and more for checkpointing: >>>>> - "create/setup conntrack" (otherwise connections get dropped), >>>>> - "set task start time" (needed for Oracle checkpointing BTW), >>>>> - "set some statistics counters (e.g. networking or taskstats)" >>>>> - "restore inotify" >>>>> and so on and so forth. >>>> right. we know that we will have to handle a lot of these >>>> and more and we will need an API for it :) so how should we handle it ? >>>> through a dedicated syscall that would be able to checkpoint and/or >>>> restart a process, an ipc object, an ipc namespace, a full container ? >>>> will it take a fd or a big binary blob ? >>>> I personally really liked Pavel idea's of filesystem. but we dropped the >>>> thread. >>> Imho having a file system interface means having all its problems. >>> Imagine you have some information about tasks exported with a file system interface. >>> Obviously to collect the information you have to hold some spinlock like tasklist_lock or similar. >>> Obviously, you have to drop the lock between sys_read() syscalls. >>> So interface gets much more complicated - you have to rescan the objects and somehow find the place where >>> you stopped previous read. Or you have to to force reader to read everything at once. >> To remember the place when we stopped previous read we have a "pos" counter >> on the struct file. >> >> Actually, tar utility, that I propose to perform the most simple migration >> reads the directory contents with 4Kb buffer - that's enough for ~500 tasks. >> >> Besides, is this a real problem for a frozen container? > > I like the idea of a C/R filesystem. Does it implies a specific user > space program to orchestrate the checkpoint/restart of the different > subsystems ? I …
creator: Pavel Emelianov
date: 2008-02-04T15:16:43-00:00

Re: [PATCH 2.6.24-rc8-mm1 00/15] IPC: code rewrite + new functionalities
type: item
description: Pavel Machek wrote: > Hi! > >> * Patches 9 to 15 propose to add some functionalities, and thus are >> submitted here for RFC, about both the interest and their implementation. >> These functionalities are: >> - Two new control-commands: >> . IPC_SETID: to change an IPC's id. >> . IPC_SETALL: behaves as IPC_SET, except that it also sets all time >> and pid values) >> - add a /proc/<pid>/semundo file to read and write the undo values of >> some semaphores for a given process. >> >> As the namespaces and the "containers" are being integrated in the >> kernel, these functionalities may be a first step to implement the >> checkpoint/restart of an application: in fact the existing API does not allow >> to specify or to change an ID when creating an IPC, when restarting an >> application, and the times/pids values of each IPCs are also altered. May be >> someone may find another interest about this ? >> >> So again, comments are welcome. > > Checkpoint/restart is nice, but... sysV ipc is broken by design, do we > really want to extend it? > Pavel For my personal culture, what do you mean by "broken by design"? Even if it's broken, don't you think some people could be interested in checkpointing "legacy" applications that use SysV IPC? Benjamin -- B e n j a m i n T h e r y - BULL/DT/Open Software R&D http://www.bull.com _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Benjamin Thery
date: 2008-02-04T15:44:34-00:00

Re: [PATCH 2.6.24-rc8-mm1 00/15] IPC: code rewrite + new functionalities
type: item
description: Hi! >>> As the namespaces and the "containers" are being integrated in the >>> kernel, these functionalities may be a first step to implement the >>> checkpoint/restart of an application: in fact the existing API does not allow >>> to specify or to change an ID when creating an IPC, when restarting an >>> application, and the times/pids values of each IPCs are also altered. May be >>> someone may find another interest about this ? >>> >>> So again, comments are welcome. >> >> Checkpoint/restart is nice, but... sysV ipc is broken by design, do we >> really want to extend it? > > For my personal culture, what do you mean by "broken by design"? int shmget(key_t key, size_t size, int shmflg); ....so how do you produce key in a way that is guaranteed not to interfere with other uses? Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Pavel Machek
date: 2008-02-04T19:51:50-00:00

Re: [PATCH 2.6.24-rc8-mm1 09/15] (RFC) IPC: new kernel API to change an ID
type: item
description: I strongly second Kirill on this matter. IMHO, we should _avoid_ as much as possible exposing internal kernel state to applications, unless a _real_ need for it is _clearly_ demonstrated. The reasons for this are quite obvious. It isn't strictly necessary to export a new interface in order to support checkpoint/restart. **. Hence, I think that the speculation "we may need it in the future" is too abstract and isn't a good excuse to commit to a new, currently unneeded, interface. Should the need arise in the future, it will be easy to design a new interface (also based on aggregated experience until then). ** In fact, the suggested interface may prove problematic (as noted earlier in this thread): if you first create the resource with some arbitrary identifier and then modify the identifier (in our case, IPC id), then the restart procedure is bound to execute sequentially, because of lack of atomicity. That said, I suggest the following method instead (this is the method we use in Zap to determine the desired resource identifier when a new resource is allocated; I recall that we had discussed it in the past, perhaps the mini-summit in september ?): 1) The process/thread tells the kernel that it wishes to pre-determine the resource identifier of a subsequent call (this can be done via a new syscall, or by writing to /proc/self/...). 2) Each system call that allocates a resource and assigns an identifier is modified to check this per-thread field first; if it is set then it will attempt to allocate that particular value (if already taken, return an error, eg. EBUSY). Otherwise it will proceed as it is today. (I left out some details - eg. the kernel will keep the desire value on a per-thread field, when it will be reset, whether we want to also tag the field with its type and so on, but the idea is now clear). The main two advantages are that first, we don't need to devise a new method for every syscall that allocates said resources (sigh... just think of clone() night…
creator: Oren Laadan
date: 2008-02-05T09:51:09-00:00

Re: [PATCH 2.6.24-rc8-mm1 09/15] (RFC) IPC: new kernel API to change an ID
type: item
description: On Tue, 2008-02-05 at 04:51 -0500, Oren Laadan wrote: > That said, I suggest the following method instead (this is the method > we use in Zap to determine the desired resource identifier when a new > resource is allocated; I recall that we had discussed it in the past, > perhaps the mini-summit in september ?): > > 1) The process/thread tells the kernel that it wishes to pre-determine > the resource identifier of a subsequent call (this can be done via a > new syscall, or by writing to /proc/self/...). > > 2) Each system call that allocates a resource and assigns an > identifier > is modified to check this per-thread field first; if it is set then > it will attempt to allocate that particular value (if already taken, > return an error, eg. EBUSY). Otherwise it will proceed as it is today. You forgot to attach the patch to your mail. ;) -- Dave _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Dave Hansen
date: 2008-02-05T18:00:25-00:00

Re: [PATCH 2.6.24-rc8-mm1 09/15] (RFC) IPC: new kernel API to change an ID
type: item
description: Quoting Oren Laadan (orenl@cs.columbia.edu): > > I strongly second Kirill on this matter. > > IMHO, we should _avoid_ as much as possible exposing internal kernel > state to applications, unless a _real_ need for it is _clearly_ > demonstrated. The reasons for this are quite obvious. Hmm, sure, but this sentence is designed to make us want to agree. Yes, we want to avoid exporting kernel internals, but generally that means things like the precise layout of the task_struct. What Pierre is doing is in fact the opposite, exporting resource information in a kernel version invariant way. In fact, the very reason not to go the route you and Pavel are advocating is that if we just dump task state to a file or filesystem from the kernel in one shot, we'll be much more tempted to lay out data in a way that exports and ends up depending on kernel internals. So we'll just want to read and write the task_struct verbatim. So, there are two very different approaches we can start with. Whichever one we follow, we want to avoid having kernel version dependencies. They both have their merits to be sure. But note that in either case we need to deal with a bunch of locking. So getting back to Pierre's patchset, IIRC 1-8 are cleanups worth doing no matter 1. 9-11 sound like they are contentuous until we decide whether we want to go with a create_with_id() type approach or a set_id(). 12 is IMO a good locking cleanup regardless. 13 and 15 are contentous until we decide whether we want userspace-controlled checkpoint or a one-shot fs. 14 IMO is useful for both c/r approaches. Is that pretty accurate? > It isn't strictly necessary to export a new interface in order to > support checkpoint/restart. **. Hence, I think that the speculation > "we may need it in the future" is too abstract and isn't a good > excuse to commit to a new, currently unneeded, interface. OTOH it did succeed in starting some conversation :) > Should the > need arise in the future, it will be easy to design a new inte…
creator: serue
date: 2008-02-05T18:42:34-00:00

Re: [PATCH 2.6.24-rc8-mm1 09/15] (RFC) IPC: new kernel API to change an ID
type: item
description: Serge E. Hallyn wrote: > Quoting Oren Laadan (orenl@cs.columbia.edu): >> I strongly second Kirill on this matter. >> >> IMHO, we should _avoid_ as much as possible exposing internal kernel >> state to applications, unless a _real_ need for it is _clearly_ >> demonstrated. The reasons for this are quite obvious. > > Hmm, sure, but this sentence is designed to make us want to agree. Yes, > we want to avoid exporting kernel internals, but generally that means > things like the precise layout of the task_struct. What Pierre is doing > is in fact the opposite, exporting resource information in a kernel > version invariant way. LOL ... a bit of misunderstanding - let me put some order here: my response what with respect to the new interface that Pierre suggested, that is - to add a new IPC call to change an identifier after it has been allocated (and assigned). This is necessary for the restart because applications expect to see the same resource id's as they had at the time of the checkpoint. What you are referring to is the more recent part of the thread, where the topic became how data should be saved - in other words, the format of the checkpoint data. This is entirely orthogonal to my argument. Now please re-read my email :) That said, I'd advocate for something in between a raw dump and a pure "parametric" representation of the data. Raw data tends to be, well, too raw, which makes the task of reading data from older version by newer kernels harder to maintain. On the other hand, it is impossible to abstract everything into kernel-independent format. > > In fact, the very reason not to go the route you and Pavel are > advocating is that if we just dump task state to a file or filesystem > from the kernel in one shot, we'll be much more tempted to lay out data > in a way that exports and ends up depending on kernel internals. So > we'll just want to read and write the task_struct verbatim. > > So, there are two very different approaches we can start with. > Whichever…
creator: Oren Laadan
date: 2008-02-06T02:06:27-00:00

Re: [PATCH 2.6.24-rc8-mm1 09/15] (RFC) IPC: new kernel API to change an ID
type: item
description: Quoting Oren Laadan (orenl@cs.columbia.edu): > > > Serge E. Hallyn wrote: >> Quoting Oren Laadan (orenl@cs.columbia.edu): >>> I strongly second Kirill on this matter. >>> >>> IMHO, we should _avoid_ as much as possible exposing internal kernel >>> state to applications, unless a _real_ need for it is _clearly_ >>> demonstrated. The reasons for this are quite obvious. >> Hmm, sure, but this sentence is designed to make us want to agree. Yes, >> we want to avoid exporting kernel internals, but generally that means >> things like the precise layout of the task_struct. What Pierre is doing >> is in fact the opposite, exporting resource information in a kernel >> version invariant way. > > LOL ... a bit of misunderstanding - let me put some order here: > > my response what with respect to the new interface that Pierre > suggested, that is - to add a new IPC call to change an identifier > after it has been allocated (and assigned). This is necessary for the > restart because applications expect to see the same resource id's as > they had at the time of the checkpoint. > > What you are referring to is the more recent part of the thread, where > the topic became how data should be saved - in other words, the format > of the checkpoint data. This is entirely orthogonal to my argument. > > Now please re-read my email :) Heh - by the end of my response I was pretty sure that was the case :) > That said, I'd advocate for something in between a raw dump and a pure > "parametric" representation of the data. Raw data tends to be, well, > too raw, which makes the task of reading data from older version by > newer kernels harder to maintain. On the other hand, it is impossible > to abstract everything into kernel-independent format. Well, that's probably getting a little pedantic, but true. >> In fact, the very reason not to go the route you and Pavel are >> advocating is that if we just dump task state to a file or filesystem >> from the kernel in one shot, we'll be much more tempt…
creator: serue
date: 2008-02-06T05:00:39-00:00

Re: [PATCH 2.6.24-rc8-mm1 09/15] (RFC) IPC: new kernel API to change an ID
type: item
description: Serge E. Hallyn wrote: > > But note that in either case we need to deal with a bunch of locking. > So getting back to Pierre's patchset, IIRC 1-8 are cleanups worth > doing no matter 1. 9-11 sound like they are contentuous until > we decide whether we want to go with a create_with_id() type approach > or a set_id(). 12 is IMO a good locking cleanup regardless. 13 and > 15 are contentous until we decide whether we want userspace-controlled > checkpoint or a one-shot fs. 14 IMO is useful for both c/r approaches. > > Is that pretty accurate? > Ok, so, so far, the discussion stays opened about the new functionalities for c/r. As there were no objection about the first patches, which rewrite/enhance the existing code, Andrew, could you consider them (ie patches 1 to 8 of this series) for inclusion in -mm ? (I mean, as soon as it is possible, as I guess you're pretty busy for now with the merge for 2.6.25) If you prefer, I can resend them separately ? Thanks, Pierre >> It isn't strictly necessary to export a new interface in order to >> support checkpoint/restart. **. Hence, I think that the speculation >> "we may need it in the future" is too abstract and isn't a good >> excuse to commit to a new, currently unneeded, interface. > > OTOH it did succeed in starting some conversation :) > >> Should the >> need arise in the future, it will be easy to design a new interface >> (also based on aggregated experience until then). > > What aggregated experience? We have to start somewhere... > >> ** In fact, the suggested interface may prove problematic (as noted >> earlier in this thread): if you first create the resource with some >> arbitrary identifier and then modify the identifier (in our case, >> IPC id), then the restart procedure is bound to execute sequentially, >> because of lack of atomicity. > > Hmm? Lack of atomicity wrt what? All the tasks being restarted were > checkpointed at the same time so there will be no conflict in the > requested IDs, so I don't know what y…
creator: Pierre Peiffer
date: 2008-02-08T10:12:33-00:00