From owner-freebsd-current@freebsd.org Fri Oct 14 16:24:00 2016 Return-Path: Delivered-To: freebsd-current@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 40B1BC1119D for ; Fri, 14 Oct 2016 16:24:00 +0000 (UTC) (envelope-from scott.k.mitch1@gmail.com) Received: from mail-yb0-x236.google.com (mail-yb0-x236.google.com [IPv6:2607:f8b0:4002:c09::236]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 0936CE2C for ; Fri, 14 Oct 2016 16:23:59 +0000 (UTC) (envelope-from scott.k.mitch1@gmail.com) Received: by mail-yb0-x236.google.com with SMTP id 184so44387977yby.2 for ; Fri, 14 Oct 2016 09:23:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=q2B0lQAYKRnepNKPw+4HSbxo9hXKc+Blmou/rJgqkh8=; b=oPbx8ArYBZk+yR7ydC8usch4/otq24DAMK5GcXheEq2+vosCDxwAht9zAMwovaV+K4 IAn23dI821vBPOFbkez/crHwpyHLg/cCp6VOXGHbpAcfbj0ayPG/Ra7SWRZNAOCChQOC ijqX9w6k8XXZTPcxWfiNMEBnwjimsDA/QApj72LBm2zUaSmDl9VSbgYWZBXirWIn7/k5 qRvRxUNc6FkOKhqLGSOYTOqjdsROVdBjUF/0i7sredNEbol3XP6SvcDS32b9UavTfGwR Hm440pZz+43xxf7VKmJk5jkHcpySPrA788fxhVhHG4azr3kvFjUcpcpl31rTA+mqG49c RQ9A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=q2B0lQAYKRnepNKPw+4HSbxo9hXKc+Blmou/rJgqkh8=; b=VkhGWZzoGkzMDk5XDQh7Bwq/47EbJh0KzfQOMvbCV5hEqw/oD3Da2+H0kYaxl399Bq +gh8LHnJINxF1H9nYPSYSL10AjSPVzb8ld75jWxk1eXSJtuZJtrI0jD55rAWOIeOvZg4 eAj2KbCEAVMEYYa9TcXNyKrYptv0GYvfMJmo0EruX+d1Gw20rReXcTeeFgqY9gw//BTe Laxd/5dA0xujU1sNNfKSH5zlz8XAq2pc3oy7bZRbJjJ4I5UGHod4L6qbKpJ/xj2Wyqb4 B41Wuthfz0jzKICWR9kwcIvRB1fCpoxMy9lUTHzYXBmZQFitiAPHi+NUyDb9Hk0kIEOe EmWg== X-Gm-Message-State: AA6/9RlOAOAjBfFgWMaVjBsi0r/uWhcx7/hybOPcCHXJZN60k8wRBmD4WpdmHxEYNcVZM2YTVmdpMkxm9ObY2A== X-Received: by 10.37.19.136 with SMTP id 130mr10948558ybt.26.1476462239191; Fri, 14 Oct 2016 09:23:59 -0700 (PDT) MIME-Version: 1.0 Received: by 10.13.223.198 with HTTP; Fri, 14 Oct 2016 09:23:58 -0700 (PDT) In-Reply-To: <20161014120333.GB2404@kib.kiev.ua> References: <611243783F62AF48AFB07BC25FA4B1061CE64959@DLREXMBX01.intra.dlr.de> <20161014120333.GB2404@kib.kiev.ua> From: Scott Mitchell Date: Fri, 14 Oct 2016 09:23:58 -0700 Message-ID: Subject: Re: asio and kqueue (2nd trye) (was: RE: (boost::)asio and kqueue problem) To: Konstantin Belousov Cc: Hartmut.Brandt@dlr.de, freebsd-current@freebsd.org, Sepherosa Ziehau , Adrian Chadd X-Mailman-Approved-At: Fri, 14 Oct 2016 16:35:15 +0000 Content-Type: text/plain; charset=UTF-8 X-Content-Filtered-By: Mailman/MimeDel 2.1.23 X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 14 Oct 2016 16:24:00 -0000 Patch generally lgtm ... just 1 nit comment: + } else { + if (sbavail(&so->so_rcv) >= so->so_rcv.sb_lowat) + return 1; + } Collapse the else and the block inside to just make it an `else if` for less branching. On Fri, Oct 14, 2016 at 5:03 AM, Konstantin Belousov wrote: > On Fri, Oct 14, 2016 at 09:21:52AM +0000, Hartmut.Brandt@dlr.de wrote: > > Hi all, > > > > here is the 2nd try taking into account the comments I received. Since > I'm not familiar with the locking in the sockets area I ask somebody with > that knowledge to check it before I commit it. > > I have only style notes, the factual code changes in the patch look good > to me. > > Index: uipc_socket.c > =================================================================== > --- uipc_socket.c (revision 307091) > +++ uipc_socket.c (working copy) > @@ -159,15 +159,9 @@ > static int filt_soread(struct knote *kn, long hint); > static void filt_sowdetach(struct knote *kn); > static int filt_sowrite(struct knote *kn, long hint); > -static int filt_solisten(struct knote *kn, long hint); > static int inline hhook_run_socket(struct socket *so, void *hctx, int32_t > h_id); > fo_kqfilter_t soo_kqfilter; > > -static struct filterops solisten_filtops = { > - .f_isfd = 1, > - .f_detach = filt_sordetach, > - .f_event = filt_solisten, > -}; > static struct filterops soread_filtops = { > .f_isfd = 1, > .f_detach = filt_sordetach, > @@ -3075,10 +3069,7 @@ > > switch (kn->kn_filter) { > case EVFILT_READ: > - if (so->so_options & SO_ACCEPTCONN) > - kn->kn_fop = &solisten_filtops; > - else > - kn->kn_fop = &soread_filtops; > + kn->kn_fop = &soread_filtops; > sb = &so->so_rcv; > break; > case EVFILT_WRITE: > @@ -3282,29 +3273,34 @@ > static int > filt_soread(struct knote *kn, long hint) > { > - struct socket *so; > + struct socket *so = kn->kn_fp->f_data; > Style is against mixing declaration and initialization. Please keep the > next removed line instead. > > - so = kn->kn_fp->f_data; > This one. > > - SOCKBUF_LOCK_ASSERT(&so->so_rcv); > + if (so->so_options & SO_ACCEPTCONN) { > + kn->kn_data = so->so_qlen; > + return (!TAILQ_EMPTY(&so->so_comp)); > > - kn->kn_data = sbavail(&so->so_rcv) - so->so_rcv.sb_ctl; > - if (so->so_rcv.sb_state & SBS_CANTRCVMORE) { > - kn->kn_flags |= EV_EOF; > - kn->kn_fflags = so->so_error; > - return (1); > - } else if (so->so_error) /* temporary udp error */ > - return (1); > + } else { > You do not need else {} block, 'then' branch ends with return(). If you > remove else, you do not need additional indent for the old filt_soread() > function' body. > > + SOCKBUF_LOCK_ASSERT(&so->so_rcv); > > - if (kn->kn_sfflags & NOTE_LOWAT) { > - if (kn->kn_data >= kn->kn_sdata) > - return 1; > - } else { > - if (sbavail(&so->so_rcv) >= so->so_rcv.sb_lowat) > - return 1; > + kn->kn_data = sbavail(&so->so_rcv) - so->so_rcv.sb_ctl; > + if (so->so_rcv.sb_state & SBS_CANTRCVMORE) { > + kn->kn_flags |= EV_EOF; > + kn->kn_fflags = so->so_error; > + return (1); > + } else if (so->so_error) /* temporary udp error */ > + return (1); > + > + if (kn->kn_sfflags & NOTE_LOWAT) { > + if (kn->kn_data >= kn->kn_sdata) > + return 1; > return (1); > since you change the line anyway. > > + } else { > + if (sbavail(&so->so_rcv) >= so->so_rcv.sb_lowat) > + return 1; > Same. > > + } > + > + /* This hook returning non-zero indicates an event, not > error */ > + return (hhook_run_socket(so, NULL, HHOOK_FILT_SOREAD)); > } > - > - /* This hook returning non-zero indicates an event, not error */ > - return (hhook_run_socket(so, NULL, HHOOK_FILT_SOREAD)); > } > > static void > @@ -3346,16 +3342,6 @@ > return (kn->kn_data >= so->so_snd.sb_lowat); > } > > -/*ARGSUSED*/ > -static int > -filt_solisten(struct knote *kn, long hint) > -{ > - struct socket *so = kn->kn_fp->f_data; > - > - kn->kn_data = so->so_qlen; > - return (!TAILQ_EMPTY(&so->so_comp)); > -} > - > int > socheckuid(struct socket *so, uid_t uid) > { >