Re: [PATCH 0/1] SIGWINCH problem with terminal apps still alive

["Adam Tlałka" <atlka@pg.gda.pl>]

Fri, 10 Oct 2008 12:35:17 +0200 - Adam Tlałka <atlka@pg.gda.pl>:

> Fri, 10 Oct 2008 10:29:06 +0100 - Alan Cox <alan@lxorguk.ukuu.org.uk>:
> > 
> > NAK again
> > 
> > Moving the copies around simply moves the race, it might be that it
> > fixes your box and unfixes other peoples.
> > 
> 
> I don't think so. Race appears because of kill_pgrp() call which
> generates SIGWINCH so it leads to reschedule and ioctl() which reads
> termios sizes before they are updated - from time to time. So if we
> update variables before signal generation there will be no race.
> Moving the point of variables update eliminates
> possibility of reading old values. So even if after kill_pgrp() the
> other process will not lock here on this mutex values obtained will be
> sane.
> 
> Whats more we could protect by mutex variable only test and change
> operations and it stil will work correctly.
> 
> Because now we have 2.6.27 I tested this kind of code in
> tty_io.c(tty_do_resize):
> 
> ...
> 
> So it works, and change of tty->winsize and real_tty->winsize are
> protected . Why another process should wait more if winsize is
> already properly set?

Next if we want to speed up our code in case of resize we could remove
one of two comparizons so values always be updated in tty_io.c(tty_do_resize):

	struct pid *pgrp, *rpgrp;
        unsigned long flags;

	/* For a PTY we need to lock the tty side */                                                                 
        mutex_lock(&real_tty->termios_mutex);                                                                        
        flags = memcmp(ws, &tty->winsize, sizeof(*ws));
        tty->winsize = *ws;
        real_tty->winsize = *ws;
        mutex_unlock(&real_tty->termios_mutex);                                                                      
        if (flags){                                                                                                  
                /* Get the PID values and reference them so we can                                                   
                   avoid holding the tty ctrl lock while sending signals */                                          
                spin_lock_irqsave(&tty->ctrl_lock, flags);                                                           
                pgrp = get_pid(tty->pgrp);                                                                           
                rpgrp = get_pid(real_tty->pgrp);                                                                     
                spin_unlock_irqrestore(&tty->ctrl_lock, flags);                                                      
                                                                                                                     
                if (pgrp)                                                                                            
                        kill_pgrp(pgrp, SIGWINCH, 1);                                                                
                if (rpgrp != pgrp && rpgrp)                                                                          
                        kill_pgrp(rpgrp, SIGWINCH, 1);                                                               
                                                                                                                     
                put_pid(pgrp);                                                                                       
                put_pid(rpgrp);                                                                                      
        }                                                                                                            
                                                                                                                     
        return 0;

We could assume that ioctl which sets the same values is rather rare
so we want faster code in case of changes. Presented above code for 
kernel 2.6.27 works quit nicely and I can't observe any bad effect of it.
Anyway we can prove on paper by time diagrams that there will be no races
according to update and reading winsize variables.

Regards

-- 
Adam Tlałka       mailto:atlka@pg.gda.pl    ^v^ ^v^ ^v^
System  & Network Administration Group       - - - ~~~~~~
Computer Center, Gdańsk University of Technology, Poland