* Jan Safranek <jsafr...@redhat.com> [2011-03-15 14:40:24]:
> Current libcgroup design handles each hierarchy only once. If a hierarchy
> is mounted twice or more times, only the first mount point is taken into
> account and the others are 'invisible' to libcgroup.
>
> This causes cgsnapshot and lssubsys to show only one mount point for a
> hierarchy and especially in case of cgsnapshot it's not what user expects.
> The patch below enhances internal table of mount points with duplicate
> mounts of the same hierarchy.
>
> In addition. cgroup_get_controller_begin is enhanced too. This introduces
> incompatible ABI change, sizeof() public structure changes and all apps
> needs to be recompiled - how does it sound to you? Or should I make new set
> of begin/next/end functions to get all mount points of a hierarchy?
>
>
> Please note that the patch is not intended to be accepted, I need to test
> it and also some changes in lssubsys and cgsnapshot are needed.
>
I was going to ask about the test case and how this was tested.
> Signed-off-by: Jan Safranek <jsafr...@redhat.com>
> ---
>
> include/libcgroup/iterators.h | 15 +++++++++++++++
> src/api.c | 41
> +++++++++++++++++++++++++----------------
> src/config.c | 4 ++++
> src/libcgroup-internal.h | 6 ++++++
> 4 files changed, 50 insertions(+), 16 deletions(-)
>
> diff --git a/include/libcgroup/iterators.h b/include/libcgroup/iterators.h
> index 8ecc53f..a897336 100644
> --- a/include/libcgroup/iterators.h
> +++ b/include/libcgroup/iterators.h
> @@ -305,6 +305,19 @@ int cgroup_get_task_end(void **handle);
> */
>
> /**
> + * Flags for struct cgroup_mount_point.
> + */
> +enum cgroup_mount_point_flags {
> + /**
> + * When this flag is set, cgroup_mount_point describes a controller,
> + * which has already been reported by cgroup_get_controller_begin()
> + * or cgroup_get_controller_next(). This happens when one hierarchy
> + * is mounted to two or more different mount points.
> + */
> + CGROUP_MOUNT_POINT_DUPLICATE
> +};
> +
> +/**
> * Information about mounted controller.
> */
> struct cgroup_mount_point {
> @@ -312,6 +325,8 @@ struct cgroup_mount_point {
> char name[FILENAME_MAX];
> /** Mount point of the controller. */
> char path[FILENAME_MAX];
> + /** Flags; combination of enum cgroup_mount_point_flags. */
> + int flags;
> };
>
> /**
> diff --git a/src/api.c b/src/api.c
> index b76b793..508fc1f 100644
> --- a/src/api.c
> +++ b/src/api.c
> @@ -823,16 +823,12 @@ int cgroup_init(void)
> break;
> }
> }
> - if (duplicate) {
> - cgroup_dbg("controller %s is already mounted on
> %s\n",
> - mntopt, cg_mount_table[j].path);
> - break;
> - }
> -
> strcpy(cg_mount_table[found_mnt].name, controllers[i]);
> strcpy(cg_mount_table[found_mnt].path, ent->mnt_dir);
> - cgroup_dbg("Found cgroup option %s, count %d\n",
> - ent->mnt_opts, found_mnt);
> + cg_mount_table[found_mnt].duplicate = duplicate;
I presume you have this under the if (duplicate) part, but the diff
seems to indicate otherwise.
> + cgroup_dbg("Found cgroup option %s, count %d, " \
> + "duplicate %d\n", ent->mnt_opts, found_mnt,
> + duplicate);
> found_mnt++;
> }
>
> @@ -856,16 +852,12 @@ int cgroup_init(void)
> }
> }
>
> - if (duplicate) {
> - cgroup_dbg("controller %s is already mounted on
> %s\n",
> - mntopt, cg_mount_table[j].path);
> - continue;
> - }
> -
> strcpy(cg_mount_table[found_mnt].name, mntopt);
> strcpy(cg_mount_table[found_mnt].path, ent->mnt_dir);
> - cgroup_dbg("Found cgroup option %s, count %d\n",
> - ent->mnt_opts, found_mnt);
> + cg_mount_table[found_mnt].duplicate = duplicate;
> + cgroup_dbg("Found cgroup option %s, count %d, " \
> + "duplicate %d\n", ent->mnt_opts, found_mnt,
> + duplicate);
> found_mnt++;
> }
> }
> @@ -954,6 +946,12 @@ static char *cg_build_path_locked(const char *name, char
> *path,
> int i;
> for (i = 0; cg_mount_table[i].name[0] != '\0'; i++) {
> /*
> + * Don't examine duplicates, those controllers were already
> + * checked.
> + */
> + if (cg_mount_table[i].duplicate)
> + continue;
> + /*
> * XX: Change to snprintf once you figure what n should be
> */
> if (strcmp(cg_mount_table[i].name, type) == 0) {
> @@ -1046,6 +1044,8 @@ int cgroup_attach_task_pid(struct cgroup *cgroup, pid_t
> tid)
> pthread_rwlock_rdlock(&cg_mount_table_lock);
> for (i = 0; i < CG_CONTROLLER_MAX &&
> cg_mount_table[i].name[0] != '\0'; i++) {
> + if (cg_mount_table[i].duplicate)
> + continue;
> if (!cg_build_path_locked(NULL, path,
> cg_mount_table[i].name))
> continue;
> @@ -2119,6 +2119,9 @@ int cgroup_get_cgroup(struct cgroup *cgroup)
> struct stat stat_buffer;
> int path_len;
>
> + if (cg_mount_table[i].duplicate)
> + continue;
> +
> if (!cg_build_path_locked(NULL, path,
> cg_mount_table[i].name))
> continue;
> @@ -2245,6 +2248,8 @@ static int cg_prepare_cgroup(struct cgroup *cgroup,
> pid_t pid,
> pthread_rwlock_rdlock(&cg_mount_table_lock);
> for (j = 0; j < CG_CONTROLLER_MAX &&
> cg_mount_table[j].name[0] != '\0'; j++) {
> + if (cg_mount_table[j].duplicate)
> + continue;
> cgroup_dbg("Adding controller %s\n",
> cg_mount_table[j].name);
> cptr = cgroup_add_controller(cgroup,
> @@ -3282,6 +3287,10 @@ int cgroup_get_controller_next(void **handle, struct
> cgroup_mount_point *info)
> strncpy(info->name, cg_mount_table[*pos].name, FILENAME_MAX);
>
> strncpy(info->path, cg_mount_table[*pos].path, FILENAME_MAX);
> + if (cg_mount_table[*pos].duplicate)
> + info->flags = CGROUP_MOUNT_POINT_DUPLICATE;
> + else
> + info->flags = 0;
>
> (*pos)++;
> *handle = pos;
> diff --git a/src/config.c b/src/config.c
> index 0f71003..6a50eaa 100644
> --- a/src/config.c
> +++ b/src/config.c
> @@ -498,6 +498,8 @@ static int config_validate_namespaces(void)
>
> pthread_rwlock_wrlock(&cg_mount_table_lock);
> for (i = 0; cg_mount_table[i].name[0] != '\0'; i++) {
> + if (cg_mount_table[i].duplicate)
> + continue;
> /*
> * If we get the path in the first run, then we
> * are good, else we will need to go for two
> @@ -626,6 +628,8 @@ static int config_order_namespace_table(void)
> int j;
> int flag = 0;
> for (j = 0; cg_mount_table[j].name[0] != '\0'; j++) {
> + if (cg_mount_table[j].duplicate)
> + continue;
> if (strncmp(config_namespace_table[i].name,
> cg_mount_table[j].name, FILENAME_MAX) == 0) {
>
> diff --git a/src/libcgroup-internal.h b/src/libcgroup-internal.h
> index 801b35e..5cf8fbe 100644
> --- a/src/libcgroup-internal.h
> +++ b/src/libcgroup-internal.h
> @@ -93,6 +93,12 @@ struct cg_mount_table_s {
> char name[FILENAME_MAX];
> char path[FILENAME_MAX];
> int index;
> +
> + /**
> + * When set, the same controller is already in mount table with lower
> + * index - used when one hierarchy is mounted multiple times.
> + */
> + int duplicate;
> };
>
There are quite a lot of checks for if
(cg_mount_table[idx].duplicate), is there an easier way to fix this?
Should we have a unique list as well for use by these larger
functions?
--
Three Cheers,
Balbir
------------------------------------------------------------------------------
Colocation vs. Managed Hosting
A question and answer guide to determining the best fit
for your organization - today and in the future.
http://p.sf.net/sfu/internap-sfd2d
_______________________________________________
Libcg-devel mailing list
Libcg-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libcg-devel
