Date: Mon, 17 Mar 2008 22:02:39 +0900 From: Alexander Nedotsukov <bland@bbnest.net> To: Kostik Belousov <kostikbel@gmail.com> Cc: yokota@freebsd.org, FreeBSD Current <freebsd-current@freebsd.org>, Joe Marcus Marcus Clarke <marcus@freebsd.org> Subject: Re: page fault panic in scioctl and console-kit-daemon Message-ID: <641CAF57-F235-4F0D-A120-2B58F0B13861@bbnest.net> In-Reply-To: <20080222172930.GZ57756@deviant.kiev.zoral.com.ua> References: <4796356D.9080809@gmail.com> <20080123051648.GV57756@deviant.kiev.zoral.com.ua> <479796E1.4000500@gmail.com> <1201118159.38742.17.camel@shumai.marcuscom.com> <20080123211149.GA57756@deviant.kiev.zoral.com.ua> <1201123933.62127.9.camel@shumai.marcuscom.com> <20080124124213.GD57756@deviant.kiev.zoral.com.ua> <72E58ECA-D743-4D5E-9222-7129104E4BAC@bbnest.net> <20080221154714.GS57756@deviant.kiev.zoral.com.ua> <20CD87E3-27BC-4789-A51F-BBFDB3258B47@bbnest.net> <20080222172930.GZ57756@deviant.kiev.zoral.com.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
--Apple-Mail-92--181507418
Content-Type: text/plain;
charset=US-ASCII;
format=flowed;
delsp=yes
Content-Transfer-Encoding: 7bit
Better late than never :-) I am back. I understand your concerns and
can assure that use-mode code will do right. I changed wchan address
from system wide cdev pointer to syscons private address. What else
need to be done to get this checked in and/or will you do that or I
can proceed myself?
--Apple-Mail-92--181507418
Content-Disposition: attachment;
filename=sys_dev_syscons.patch
Content-Type: application/octet-stream; x-unix-mode=0644;
name="sys_dev_syscons.patch"
Content-Transfer-Encoding: 7bit
Index: syscons.c
===================================================================
RCS file: /home/ncvs/src/sys/dev/syscons/syscons.c,v
retrieving revision 1.457
diff -u -r1.457 syscons.c
--- syscons.c 24 Jan 2008 15:37:48 -0000 1.457
+++ syscons.c 17 Mar 2008 12:43:55 -0000
@@ -160,6 +160,7 @@
#define SC_CONSOLECTL 255
+#define VTY_WCHAN(sc, vty) (&SC_DEV(sc, vty))
#define VIRTUAL_TTY(sc, x) (SC_DEV((sc), (x)) != NULL ? \
SC_DEV((sc), (x))->si_tty : NULL)
#define ISTTYOPEN(tp) ((tp) && ((tp)->t_state & TS_ISOPEN))
@@ -1065,17 +1066,9 @@
i = (*(int *)data == 0) ? scp->index : (*(int *)data - 1);
if ((i < sc->first_vty) || (i >= sc->first_vty + sc->vtys))
return EINVAL;
- s = spltty();
- error = sc_clean_up(sc->cur_scp);
- splx(s);
- if (error)
- return error;
- scp = sc_get_stat(SC_DEV(sc, i));
- if (scp == NULL)
- return (ENXIO);
- if (scp == scp->sc->cur_scp)
+ if (i == sc->cur_scp->index)
return 0;
- error = tsleep(&scp->smode, PZERO | PCATCH, "waitvt", 0);
+ error = tsleep(VTY_WCHAN(sc, i), PZERO | PCATCH, "waitvt", 0);
return error;
case VT_GETACTIVE: /* get active vty # */
@@ -2335,7 +2328,7 @@
* be invoked at splhigh().
*/
if (debugger == 0)
- wakeup(&sc->new_scp->smode);
+ wakeup(VTY_WCHAN(sc,next_scr));
splx(s);
DPRINTF(5, ("switch done (new == old)\n"));
return 0;
@@ -2358,7 +2351,7 @@
/* wake up processes waiting for this vty */
if (debugger == 0)
- wakeup(&sc->cur_scp->smode);
+ wakeup(VTY_WCHAN(sc,next_scr));
/* wait for the controlling process to acknowledge, if necessary */
if (signal_vt_acq(sc->cur_scp)) {
@@ -2384,7 +2377,7 @@
exchange_scr(sc);
s = spltty();
/* sc->cur_scp == sc->new_scp */
- wakeup(&sc->cur_scp->smode);
+ wakeup(VTY_WCHAN(sc,sc->cur_scp->index));
/* wait for the controlling process to acknowledge, if necessary */
if (!signal_vt_acq(sc->cur_scp)) {
--Apple-Mail-92--181507418
Content-Type: text/plain;
charset=US-ASCII;
format=flowed;
delsp=yes
Content-Transfer-Encoding: 7bit
Thanks,
Alexander.
On 23.02.2008, at 2:29, Kostik Belousov wrote:
> On Sat, Feb 23, 2008 at 01:01:59AM +0900, Alexander Nedotsukov wrote:
>>
>> On 22.02.2008, at 0:47, Kostik Belousov wrote:
>>
>>> On Thu, Feb 21, 2008 at 09:26:16AM +0900, Alexander Nedotsukov
>>> wrote:
>>>> Hi,
>>>>
>>>> May I ask to revisit this issue? To me ENXIO is not semantically
>>>> correct in this particular case. It also turns out that doing
>>>> workaround in userspace may not be that good as we used to think. I
>>>> propose is to fix VT_WAITACTIVE so it simply wait for bound device
>>>> activation. For my understanding this change should not have any
>>>> impact on existing code. I also removed really strange console
>>>> cleanup
>>>> bit sticked in a long time ago (see ioctl() part).
>>>> It will be nice to see this patch
>>>
>>>
>>>> (successfully tested by our affected users) committed to all
>>>> branches.
>>>>
>>>> Thanks,
>>>> Alexander.
>>>
>>> I mostly agree with the patch, given it is tested.
>>>
>>> The first (not important) note is that change of the wait channel
>>> from
>>> the address of the private structure to the address of the cdev
>>> could
>>> cause more spurious wakeups then before. I expect you usermode
>>> code to
>>> deal with it properly.
>> Do you know any potential wakeup()s around the kernel? I did not see
>> any spurious wakeups myself nor user reported it so far. However they
>> are not welcome so if it considered to be unsafe we can use address
>> of
>> cdev pointer (&SC_DEV()) which is private to syscons.
> Nothing prevents any code in the the kernel from performing wakeup on
> any wait channel. Due to tradition, wait channel is usually an address
> of some data structure that is owned by the code performing wakeup.
> I do not know of any other code that uses cdev address as wait
> channel,
>
> The issue of spurious wakeup is not very important per se. I think
> more
> essential for the correct operation is the fact that when the user-
> mode
> code is executed, console may already be inactive (again). This is
> quite
> similar to the unintended wakeups.
>
> Does the code handle this ? If yes, I think it shall handle the
> wakeups too without any additional actions.
>
> I underline that this is not an objection against the patch.
--Apple-Mail-92--181507418--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?641CAF57-F235-4F0D-A120-2B58F0B13861>