On Thu, Aug 27, 2015 at 06:40:32PM +0300, Cyrill Gorcunov wrote:
> On Thu, Aug 27, 2015 at 06:00:28PM +0300, Vladimir Davydov wrote:
> > > +
> > > +static struct file_operations vtty_fops;
> > > +
> > > +#define vtty_match_index(idx) ((idx) >= 0 && (idx) <
> > > MAX_NR_VTTY_CONSOLES)
> >
> > nit: this line is longer than 80 symbols
>
> yes, but recall that there is no 80 syms limit. The last discussion/flame
> over line length I saw was calmed down with agreemen that 90 symbols are
Hmm, checkpatch still has max_line_length set to 80. Could you please
share a link to this agreement?
> fine. Wonder, do you really still sit on 80 chars terminal?
I use a 12" laptop. With the window vertically split into two panes, I
have only ~80 characters per each pane.
...
> > > +static void vtty_set_context(envid_t veid)
> > > +{
> >
> > nit: WARN_ON(veid)
>
> I believe you meant veid == 0
Right.
...
> > > +static void vtty_close(struct tty_struct *tty, struct file *filp)
> > > +{
> > > + struct tty_struct *peer = tty->link;
> > > + vtty_map_t *map = tty->driver_data;
> > > +
> > > + spin_lock(&map->close_lock);
> >
> > This spinlock doesn't protect us from concurrent modifications of
> > peer->count from e.g. tty_reopen. What about taking tty_mutex instead?
> >
> > On the second thought, this wouldn't help us if we patched mainstream
> > kernel, because tty_mutex is not held for tty->count modifications there
> > anymore. That said, if we simply substitute the spinlock with tty_mutex,
> > we will surely have problems sooner or later...
> >
> > May be, we'd better move our ->count tweaking to the generic path, i.e.
> > tty_release, similarly to pty? We could distinguish vtty from pty by
> > checking TTY_EXTRA_REF. What do you think?
>
> you know, i really don't wanna put our code into generic one, its gonna
> be hard to support. maybe we should lock the peer manually inside
Yes, you're right :-(
OTOH, this would reveal potential problems on build stage, while playing
with locks can end up with a dead lock after rebase :-/
> our close routine, so in this way we would have to take own lock, then
> take peer lock under it, which would protect from abba deadlock. Hm?
>
> vtty_close
> spin_lock(&map->close_lock);
> tty_lock(tty->link);
> ...
> tty_unlock(tty->link);
> spin_unlock(&map->close_lock);
>
> and instead of spinlock I would use mutex here. Won't this work?
Hmm, ->close is called under tty_lock(tty). That means we can't call
tty_lock(tty->link) here, neither can we acquire tty_mutex. OTOH, hand
if we pull patches bringing in tty_lock_slave, this should work. Will it
be difficult to pull them?
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
