From owner-freebsd-arch@FreeBSD.ORG Sun Apr 6 15:16:51 2014 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 8973C963; Sun, 6 Apr 2014 15:16:51 +0000 (UTC) Received: from mail-qc0-x22f.google.com (mail-qc0-x22f.google.com [IPv6:2607:f8b0:400d:c01::22f]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 3A58B80F; Sun, 6 Apr 2014 15:16:51 +0000 (UTC) Received: by mail-qc0-f175.google.com with SMTP id e16so5447047qcx.34 for ; Sun, 06 Apr 2014 08:16:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; bh=w1V1H0iBj1f/nzCtdOiOjrxXAlcR6CeM7O7KJKEyQ8A=; b=NaGxBWmsMZudexdV0RUvGgR3tf6G077DAmw+VYhmcJqdBxSTDKGenwFMfZfqsNOshs KVZppmO6MpYE1du8U74UDbwi3t4JQisPjsUslchQT3BAAVV1QxcKlaXM+WRKydwUoGm/ 3Ny/HJwPJTP1e4/X2x9edahKi+JxbA+pShW/FEU5uNW1fe/gqJtslcANTwSyBryjpRI1 t7wnFqDzX9rKnIQDS413b55db7RH6c1c/LocxheZOKwBFJ/H6uO3Zf2hfU/YZo+SHCK7 M7k5D9bYzwEiZHVgUFHwQuK1xtWZwJDp2lwHoaXAC8Yd8HcyH6PhdWeEIiG51EhUDR3H vIZQ== MIME-Version: 1.0 X-Received: by 10.229.198.2 with SMTP id em2mr1720202qcb.21.1396797409571; Sun, 06 Apr 2014 08:16:49 -0700 (PDT) Sender: edschouten@gmail.com Received: by 10.140.96.203 with HTTP; Sun, 6 Apr 2014 08:16:49 -0700 (PDT) In-Reply-To: <20140405153621.GH21331@kib.kiev.ua> References: <20140405153621.GH21331@kib.kiev.ua> Date: Sun, 6 Apr 2014 17:16:49 +0200 X-Google-Sender-Auth: ENszJSg_2Mt60BLpmnN5lBllZ4U Message-ID: Subject: Re: [Patch] kqueue(2) <-> procdesc(4): EVFILT_PROCDESC From: Ed Schouten To: Konstantin Belousov Content-Type: text/plain; charset=UTF-8 Cc: Robert Watson , freebsd-arch@freebsd.org X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 06 Apr 2014 15:16:51 -0000 Hi there, First of all, thanks for the quick review. Well appreciated! On 5 April 2014 17:36, Konstantin Belousov wrote: > procdesc_kqops_event() should mimic the filt_proc() much more closely > than you do. In particular: > - I do not see why do you check for NOTE_EXIT in kqops_event(). > If enforcing NOTE_EXIT anywhere, procdesc_kqfilter() is more natural place > to put the temporal restriction. I do think that signalling and execing > are equally useful as exiting notifications, but this is for future. > - The whole structure of filt_proc(), which copies masked sfflags to > fflags is easier to extend later. It seems that just repeating > most of the code from filt_proc() would do what needed. My plan was initially to mimick filt_proc() almost entirely and just use KNOTE_LOCKED(..., NOTE_EXIT) just like EVFILT_PROC does, but unfortunately this doesn't seem possible, because if we went down this path, there would be no way for us to activate the knote in procdesc_kqfilter() immediately, as KNOTE_ACTIVATE() is not available outside of kern_event.c. This is why I just changed the code, so that procdesc_kqops_event() is almost literally a copy of filt_proc(), which the difference that it tests against PDF_EXITED instead of using the hint. While there, I made some style fixes to filt_proc(). Thoughts? http://80386.nl/pub/kqueue-evfilt-procdesc.txt (refresh) > I think you must do knlist_clear() before knlist_destroy(). If I understand kqueue internals properly, I am pretty certain that knlist_clear() is not needed in this case, for two reasons: - knlist_clear man page entry: "This function must not be used when f_isfd is set in struct filterops, as the td argument of fdrop() will be NULL." Other file descriptor types also don't seem to call this function. - struct procdesc is only deallocated if the file descriptor is closed. This means that the knlist is guaranteed to be empty at this point in time. knlist_destroy() also has an assertion for this, which is good. -- Ed Schouten