[Devel] [RFC rh7 v6] ve/tty: vt -- Implement per VE support for console and terminals
Vladimir Davydov
vdavydov at parallels.com
Mon Aug 31 10:19:50 PDT 2015
On Mon, Aug 31, 2015 at 02:25:42PM +0300, Cyrill Gorcunov wrote:
...
> Index: linux-pcs7.git/drivers/tty/pty.c
> ===================================================================
> --- linux-pcs7.git.orig/drivers/tty/pty.c
> +++ linux-pcs7.git/drivers/tty/pty.c
> @@ -820,10 +820,524 @@ static void __init unix98_pty_init(void)
> static inline void unix98_pty_init(void) { }
> #endif
>
> +#if defined(CONFIG_VE)
> +
> +/*
> + * VTTY architecture overview.
> + *
> + * With VTTY we make /dev/console and /dev/tty[X] virtualized
> + * per container (note the real names may vary because the
> + * kernel itself uses major:minor numbers to distinguish
> + * devices and doesn't care how they are named inside /dev.
> + * /dev/console stands for TTYAUX_MAJOR:1 while /dev/tty[X]
> + * stands for TTY_MAJOR:[0:12]. That said from inside of
> + * VTTY /dev/console is the same as /dev/tty0.
> + *
> + * For every container here is a tty map represented by
> + * vtty_map_t. It carries @veid of VE and associated slave
> + * tty peers.
> + *
> + * map
> + * veid -> CTID
> + * vttys -> [ 0 ]
> + * `- @slave -> link -> @master
> + * [ 1 ]
> + * `- @slave -> link -> @master
> + */
> +
> +#include <linux/ve.h>
> +#include <linux/file.h>
> +#include <linux/anon_inodes.h>
> +
> +static struct tty_driver *vttym_driver;
> +static struct tty_driver *vttys_driver;
> +static DEFINE_IDR(vtty_idr);
> +
> +static struct file_operations vtty_fops;
> +
> +#define vtty_match_index(idx) ((idx) >= 0 && (idx) < MAX_NR_VTTY_CONSOLES)
> +
> +typedef struct {
> + envid_t veid;
> + struct tty_struct *vttys[MAX_NR_VTTY_CONSOLES];
> +} vtty_map_t;
> +
> +static vtty_map_t *vtty_map_lookup(envid_t veid)
> +{
> + lockdep_assert_held(&tty_mutex);
> + return idr_find(&vtty_idr, veid);
> +}
> +
> +static void vtty_map_set(vtty_map_t *map, struct tty_struct *tty)
> +{
> + lockdep_assert_held(&tty_mutex);
> + WARN_ON(map->vttys[tty->index]);
> + map->vttys[tty->index] = tty;
> +}
> +
> +#define vtty_zap_tty_map(__tty) \
> + (__tty)->driver_data = \
> + (__tty)->link->driver_data = NULL
IMO this macro looks obfuscating. Since it's only used in two places, I
think it's OK to inline it.
> +
> +static void vtty_map_del(vtty_map_t *map, struct tty_struct *tty)
This function is an antipode of vtty_map_set, so IMO we'd better call it
vtty_map_clear.
And since map always equals tty->driver_data, let's drop map argument.
To make vtty_map_set and vtty_map_clear look symmetric we can initialize
driver_data for tty and tty->link in vtty_map_set instead of
vtty_standard_install.
> +{
> + lockdep_assert_held(&tty_mutex);
> + if (map) {
> + struct tty_struct *p = map->vttys[tty->index];
> + WARN_ON(p != (tty->driver == vttys_driver ? tty : tty->link));
> + map->vttys[tty->index] = NULL;
> + vtty_zap_tty_map(tty);
> + }
> +}
> +
> +static void vtty_map_free(vtty_map_t *map)
> +{
> + lockdep_assert_held(&tty_mutex);
> + idr_remove(&vtty_idr, map->veid);
> + kfree(map);
> +}
> +
> +static vtty_map_t *vtty_map_alloc(envid_t veid)
> +{
> + vtty_map_t *map = kzalloc(sizeof(*map), GFP_KERNEL);
> +
> + lockdep_assert_held(&tty_mutex);
> + if (map) {
> + map->veid = veid;
> + veid = idr_alloc(&vtty_idr, map, veid, veid + 1, GFP_KERNEL);
> + if (veid < 0) {
> + kfree(map);
> + return ERR_PTR(veid);
> + }
> + } else
> + map = ERR_PTR(-ENOMEM);
> + return map;
> +}
> +
> +/*
> + * vttys are never supposed to be opened from inside
> + * of VE0 except special ioctl call, so treat zero as
> + * "unused" sign.
> + */
> +static unsigned long current_veid;
It's a kind of vtty-private thing, so we'd better name it
vtty_current_veid or vtty_context (the latter sounds better to me,
because it conforms to vtty_{set,drop}_context).
> +
> +static void vtty_set_context(envid_t veid)
> +{
> + lockdep_assert_held(&tty_mutex);
> + WARN_ON(!veid);
> + current_veid = veid;
> +}
> +
> +static void vtty_drop_context(void)
> +{
> + lockdep_assert_held(&tty_mutex);
> + current_veid = 0;
> +}
> +
> +static envid_t vtty_get_context(void)
> +{
> + BUILD_BUG_ON(sizeof(current_veid) < sizeof(envid_t));
Let's use envid_t for current_veid type and zap this assertion.
> + lockdep_assert_held(&tty_mutex);
> +
> + return current_veid ?: get_exec_env()->veid;
> +}
> +
> +static struct tty_struct *vtty_lookup(struct tty_driver *driver,
> + struct inode *inode, int idx)
> +{
> + vtty_map_t *map = vtty_map_lookup(vtty_get_context());
> + struct tty_struct *tty;
> +
> + if (!vtty_match_index(idx))
> + return ERR_PTR(-EIO);
> +
> + /*
> + * Nothing ever been opened yet, allocate a new
> + * tty map together with both peers from the scratch
> + * in install procedure.
> + */
> + if (!map)
> + return NULL;
> +
> + tty = map->vttys[idx];
> + if (tty) {
> + if (driver == vttym_driver)
> + tty = tty->link;
> + WARN_ON(!tty);
> + }
> + return tty;
> +}
> +
> +static void vtty_standard_install(vtty_map_t *map, struct tty_driver *driver,
> + struct tty_struct *tty)
> +{
> + WARN_ON(tty_init_termios(tty));
> +
> + tty_driver_kref_get(driver);
> + tty->driver_data = map;
> +
> + tty_port_init(tty->port);
> + tty->port->itty = tty;
> +}
> +
> +static struct tty_struct *vtty_install_peer(vtty_map_t *map,
> + struct tty_driver *driver,
> + struct tty_port *port, int index)
> +{
> + struct tty_struct *tty;
> +
> + tty = alloc_tty_struct();
> + if (!tty)
> + return ERR_PTR(-ENOMEM);
> + initialize_tty_struct(tty, driver, index);
> +
> + tty->port = port;
> + vtty_standard_install(map, driver, tty);
> + return tty;
> +}
> +
> +static int vtty_install(struct tty_driver *driver, struct tty_struct *tty)
> +{
> + envid_t veid = vtty_get_context();
> + struct tty_port *peer_port;
> + struct tty_struct *peer;
> + vtty_map_t *map;
> + int ret;
> +
> + WARN_ON_ONCE(driver != vttys_driver);
> +
> + map = vtty_map_lookup(veid);
> + if (!map) {
> + map = vtty_map_alloc(veid);
> + if (IS_ERR(map))
> + return PTR_ERR(map);
> + }
> +
> + tty->port = kzalloc(sizeof(*tty->port), GFP_KERNEL);
> + peer_port = kzalloc(sizeof(*peer_port), GFP_KERNEL);
> + if (!tty->port || !peer_port) {
> + ret = -ENOMEM;
> + goto err_free;
> + }
> +
> + peer = vtty_install_peer(map, vttym_driver, peer_port, tty->index);
> + if (IS_ERR(peer)) {
> + ret = PTR_ERR(peer);
> + goto err_free;
> + }
> +
> + vtty_standard_install(map, vttys_driver, tty);
> + vtty_map_set(map, tty);
> + tty->count++;
> +
> + tty->link = peer;
> + peer->link = tty;
Let's set TTY_PINNED_BY_OTHER on master (i.e. peer) here, instead of in
vtty_open_master.
> + return 0;
> +
> +err_free:
> + kfree(tty->port);
> + kfree(peer_port);
> + return ret;
> +}
> +
> +static int vtty_open(struct tty_struct *tty, struct file *filp)
> +{
> + set_bit(TTY_THROTTLED, &tty->flags);
> + return 0;
> +}
> +
> +static void vtty_close(struct tty_struct *tty, struct file *filp)
> +{
> + if (tty->driver == vttym_driver) {
> + /*
> + * Closing master with active slave peer
> + * may be deferred so wake up users
> + * manually.
> + */
If the master is active, slave close will be deferred too, so we should
wake up read/write waiters in either case. I think this should do the
trick:
if (tty->count <= (tty->driver == vttym_driver) ? 1 : 2) {
> + wake_up_interruptible(&tty->read_wait);
> + wake_up_interruptible(&tty->write_wait);
And AFAIU we need to wake up the other end too then.
> + }
> +}
...
> +static void ve_vtty_fini(void *data)
> +{
> + struct ve_struct *ve = data;
> + vtty_map_t *map;
> + int i;
> +
> + mutex_lock(&tty_mutex);
> + map = vtty_map_lookup(ve->veid);
> + if (map) {
> + for (i = 0; i < MAX_NR_VTTY_CONSOLES; i++) {
> + struct tty_struct *tty = map->vttys[i];
> + if (!tty)
> + continue;
> + vtty_zap_tty_map(tty);
> + }
I think this hunk would look more natural if it were folded in
vtty_map_free.
> + vtty_map_free(map);
> + }
> + mutex_unlock(&tty_mutex);
> +}
...
> +int vtty_open_master(envid_t veid, int idx)
> +{
> + struct tty_struct *tty;
> + struct file *file;
> + char devname[64];
> + int fd, ret;
> +
> + if (!vtty_match_index(idx))
> + return -EIO;
> +
> + fd = get_unused_fd_flags(0);
> + if (fd < 0)
> + return fd;
> +
> + snprintf(devname, sizeof(devname), "v%utty%d", veid, idx);
> + file = anon_inode_getfile(devname, &vtty_fops, NULL, O_RDWR);
> + if (IS_ERR(file)) {
> + ret = PTR_ERR(file);
> + goto err_put_unused_fd;
> + }
> + nonseekable_open(NULL, file);
> +
> + ret = tty_alloc_file(file);
> + if (ret)
> + goto err_fput;
> +
> + /*
> + * Opening comes from ve0 context so
> + * setup VE's context until master fetched.
> + * This is done under @tty_mutex so noone
> + * else would access it while we're holding
> + * the lock.
> + */
> + mutex_lock(&tty_mutex);
> + vtty_set_context(veid);
> +
> + tty = vtty_lookup(vttym_driver, NULL, idx);
> + if (!tty ||
> + (test_bit(TTY_CLOSING, &tty->flags) ||
> + test_bit(TTY_CLOSING, &tty->link->flags))) {
> + /*
> + * The previous connection is about to
> + * be closed so drop it from the map and
> + * allocate a new one.
> + */
> + if (tty) {
> + vtty_map_del(tty->driver_data, tty);
> + vtty_zap_tty_map(tty);
It's redundant - tty->driver_data is cleared in vtty_map_del.
> + }
> + tty = tty_init_dev(vttys_driver, idx);
> + if (IS_ERR(tty))
> + goto err_install;
So we have master:0 slave:1 here, right?
> + tty->count--;
master:0 slave:0
> + tty_unlock(tty);
> + tty = tty->link;
> + }
> +
> + /* One master at a time */
> + if (tty->count >= 1) {
> + ret = -EBUSY;
> + goto err_install;
> + }
> +
> + vtty_drop_context();
> +
> + WARN_ON(!test_bit(TTY_LDISC, &tty->flags));
> +
> + /*
> + * We're the master peer so increment
> + * slave counter as well.
> + */
> + tty_add_file(tty, file);
> + tty->count++;
If we just created the pair:
master:1 slave:0
otherwise:
master:1 slave:1+nusers
> + tty->link->count++;
If we just created the pair:
master:1 slave:1 - that's correct
otherwise:
master:1 slave:2+nusers - that's not correct, slave count must
be equal to 1+nusers here
I think we only want to increment tty->link->count here if the pari was
just created. Therefore, to fix this, you can just drop this increment
along with the tty->count decrement after tty_init_dev.
> + fd_install(fd, file);
> + vtty_open(tty, file);
> +
> + /*
> + * Defer our closing if the slave peer
> + * will be alive at this moment.
> + */
> + set_bit(TTY_PINNED, &tty->flags);
Do it in vtty_install please.
> +
> + mutex_unlock(&tty_mutex);
> + ret = fd;
> +out:
> + return ret;
> +
> +err_install:
> + vtty_drop_context();
> + mutex_unlock(&tty_mutex);
> + tty_free_file(file);
> +err_fput:
> + file->f_op = NULL;
> + fput(file);
> +err_put_unused_fd:
> + put_unused_fd(fd);
> + goto out;
> +}
...
> Index: linux-pcs7.git/include/linux/tty.h
> ===================================================================
> --- linux-pcs7.git.orig/include/linux/tty.h
> +++ linux-pcs7.git/include/linux/tty.h
> @@ -322,6 +322,9 @@ struct tty_file_private {
> #define TTY_HUPPING 21 /* ->hangup() in progress */
> #define TTY_LDISC_HALTED 22 /* Line discipline is halted */
> #define TTY_CHARGED 23 /* Charged as ub resource */
> +#ifdef CONFIG_VE
> +#define TTY_PINNED 24 /* TTY is pinned, defer closing */
Call it TTY_PINNED_BY_OTHER please.
> +#endif
>
> #define TTY_WRITE_FLUSH(tty) tty_write_flush((tty))
>
> Index: linux-pcs7.git/include/linux/ve.h
> ===================================================================
> --- linux-pcs7.git.orig/include/linux/ve.h
> +++ linux-pcs7.git/include/linux/ve.h
> @@ -66,13 +66,6 @@ struct ve_struct {
> struct binfmt_misc *binfmt_misc;
> #endif
>
> -#define VZ_VT_MAX_DEVS 12
> - struct tty_driver *vz_vt_driver;
> - struct tty_struct *vz_tty_vt[VZ_VT_MAX_DEVS];
> -
> - struct tty_struct *vz_tty_conm;
> - struct tty_struct *vz_tty_cons;
> -
> #ifdef CONFIG_TTY
> struct device *consdev;
> #endif
> @@ -209,17 +202,18 @@ extern unsigned long long ve_relative_cl
> extern void monotonic_abs_to_ve(clockid_t which_clock, struct timespec *tp);
> extern void monotonic_ve_to_abs(clockid_t which_clock, struct timespec *tp);
>
> -#ifdef CONFIG_VTTYS
> -extern int vtty_open_master(int veid, int idx);
> -extern struct tty_driver *vtty_driver;
> -#else
> -static inline int vtty_open_master(int veid, int idx) { return -ENODEV; }
> -#endif
> -
> void ve_stop_ns(struct pid_namespace *ns);
> void ve_exit_ns(struct pid_namespace *ns);
> int ve_start_container(struct ve_struct *ve);
>
> +#define MAX_NR_VTTY_CONSOLES (12)
It is not used anywhere outside pty.c, so please make it pty.c-private.
More information about the Devel
mailing list