From owner-freebsd-net@FreeBSD.ORG Sat Sep 28 15:49:50 2013 Return-Path: Delivered-To: freebsd-net@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 ESMTP id 8E1D6CEC for ; Sat, 28 Sep 2013 15:49:50 +0000 (UTC) (envelope-from gnn@neville-neil.com) Received: from vps.hungerhost.com (vps.hungerhost.com [216.38.53.176]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 5A13522BF for ; Sat, 28 Sep 2013 15:49:50 +0000 (UTC) Received: from sub-736ip109.rev.onenet.an ([190.88.36.109]:64760 helo=[10.1.17.227]) by vps.hungerhost.com with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.80.1) (envelope-from ) id 1VPwmA-0006u8-Qe; Sat, 28 Sep 2013 11:49:47 -0400 Content-Type: multipart/signed; boundary="Apple-Mail=_A45CAE38-AC98-44E2-9683-BD22C3A8BE5A"; protocol="application/pgp-signature"; micalg=pgp-sha1 Mime-Version: 1.0 (Mac OS X Mail 6.6 \(1510\)) Subject: Re: Multiqueue support for bpf From: George Neville-Neil In-Reply-To: Date: Sat, 28 Sep 2013 11:49:44 -0400 Message-Id: References: To: Takuya ASADA X-Mailer: Apple Mail (2.1510) X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - vps.hungerhost.com X-AntiAbuse: Original Domain - freebsd.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - neville-neil.com X-Get-Message-Sender-Via: vps.hungerhost.com: authenticated_id: gnn@neville-neil.com Cc: FreeBSD Net , Luigi Rizzo X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 28 Sep 2013 15:49:50 -0000 --Apple-Mail=_A45CAE38-AC98-44E2-9683-BD22C3A8BE5A Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=us-ascii Bump Has anyone else reviewed this code? I have looked it over but not run = it. Visually it looks fine to me. Best, George On Sep 4, 2013, at 4:04 , Takuya ASADA wrote: > Hi, >=20 > This is 2nd version of multiqueue bpf patch, I think I fixed things = what > you commented on previous mail. > Here's a change list of the patch: >=20 > - Drop added functions on struct > ifnet(if_get_[rt]xqueue_len/if_get_[rt]xqueue_affinity). > HW queue number and queue affinity informations are maybe useful for = some > applications, but it's not really directly related to multiqueue bpf. = I > think we should discuss them separately. >=20 > - Use BITSET for queue mask. > It seems better to use BITSET for queue mask structure, instead of = boolean > array. >=20 > - Drop tcpdump/libpcap changes. > It also should discuss separately. >=20 > - M_QUEUEID/IFCAP_QUEUEID > M_QUEUEID is the flag for mbuf which contains hw queue id. > IFCAP_QUEUEID is the flag which means the driver has ability to set = queue > id on mbuf. >=20 >=20 >=20 > 2013/7/3 Luigi Rizzo >=20 >>=20 >>=20 >>=20 >> On Tue, Jul 2, 2013 at 5:56 PM, Takuya ASADA = wrote: >>=20 >>> Hi, >>>=20 >>> Do you have an updated URL for the diffs ? The link below from your >>>> original message >>>> seems not working now (NXDOMAIN) >>>>=20 >>>> http://www.dokukino.com/mq_bpf_20110813.diff >>>>=20 >>>=20 >>> Changes with recent head is on my repository: >>> http://svnweb.freebsd.org/base/user/syuu/mq_bpf/ >>> And I attached a diff file on this mail. >>>=20 >>>=20 >> thanks for the diffs (the URL to the repo is useful too, >> but a URL to generate diffs is more convenient for reviewing = changes). >>=20 >> I believe it still needs a bit of work before being merged. >>=20 >> My comments (in order of the patch): >>=20 >> =3D=3D=3D ifnet.9 (and related code in if.c, sockio.h) =3D=3D=3D >> - if_get_rxqueue_len()/if_get_rxqueue_len() is not a good name, >> as to me at least it suggests that it returns the size of the >> individual queue, rather than the number of queues. >>=20 >> - cpu affinity (in userspace) is a bitmask, whereas in the BSD kernel >> we almost never use the term "affinity", and favour "couid" or = "oncpu" >> (i.e. an individual CPU id). >> I think you should either rename if_get_txqueue_affinity(), or make >> the return type a cpuset (which seems more sensible as the return >> value is passed to userspace) >>=20 >> =3D=3D=3D bpf.4 (and related code) =3D=3D=3D >>=20 >> - the ioctl() to attach/detach/report queues attached to a specific >> bpf descriptor talk about "mask bit" but neither the internal nor >> the external implementation deal with bits. >> I'd rather document those ioctl as "attaching queue to file = descriptor". >>=20 >> - the BPF ioctl names are generally inconsistent (using either S or = SET >> and G or GET for the setter and getter methods). >> But you should pick one of the patterns and stick with it, >> not introduce a third variant (GT/ST). >> Given we are in 2013 we might go for the long form GET and SET >> so i suggest the following (spaces for clarity) >>=20 >> +#define BIOC ENA QMASK _IO('B', 133) >> +#define BIOC DIS QMASK _IO('B', 134) >> +#define BIOC SET RXQMASK _IOWR('B', 135, uint32_t) >> +#define BIOC CLR RXQMASK _IOWR('B', 136, uint32_t) >> +#define BIOC GET RXQMASK _IOR('B', 137, uint32_t) >> +#define BIOC SET TXQMASK _IOWR('B', 138, uint32_t) >> +#define BIOC CLR TXQMASK _IOWR('B', 139, uint32_t) >> +#define BIOC GET TXQMASK _IOR('B', 140, uint32_t) >> +#define BIOC SET OTHERMASK _IO('B', 141) >> +#define BIOC CLR OTHERMASK _IO('B', 142) >> +#define BIOC GET OTHERMASK _IOR('B', 143, uint32_t) >>=20 >> Also related: the existing ioctls() use u_int as argumnts, rather >> than uint32_t. I personally prefer the uint32_t form, but you >> should at least add a comment to indicate that the choice is >> deliberate. >>=20 >> =3D=3D=3D if.c =3D=3D=3D >>=20 >>=20 >> - you have a KASSERT to report if ifp->if_get_*xqueue_affinity() is = not >> set, but i'd rather run the function only if is set, so you can >> have a multiqueue interface which does not bind queues to specific = cores >> (which i am not sure is always a great idea; too many processes >> statically bound to the same queue mean you lose opportunity to >> parallelize work.) >>=20 >> =3D=3D=3D mbuf.h =3D=3D=3D >>=20 >> as mentioned earlier, the modification to struct mbuf should >> be avoided if possible at all. It seems that you need just one >> direction bit (which maybe is available already from the context) >> and one queue identifier, which in the rx path, at least in your >> implementation is always a replica of the 'flowid' field. >> Can you see if perhaps the flowid field can be (ab)used on the >> tx path as well ? >>=20 >>=20 >> =3D=3D=3D if.h =3D=3D=3D >>=20 >> - in if.h, can you use individual variables instead of arrays >> for ifr_queue_affinity_index and friends ? >> The macros to map the fields of ifr_ifru one >> level up are a necessary evil, >> but there is no point in using the arrays. >>=20 >> - SIOCGIFTXQAFFINITY seems to use the receive function (copy&paste = typo) >> talks about >> Also, this function is probably something that should be coordinated >> with work on generic multiqueue support >>=20 >>=20 >> =3D=3D=3D bpf.c =3D=3D=3D >>=20 >> - in linux (and hopefully in FreeBSD at some point) the number of = queues >> can be changed at runtime. >> So i suggest that you cache the current number of queues when >> you allocate the arrays (qm_*xq_qmask[] ) rather than invoking >> ifp->if_get_*xqueue_len() everytime you need to do a boundary check. >> This will save us from all sort of problems later. >>=20 >> - in terms of code, the six BIOC*XQMASK are very similar, you are = probably >> better off having one single case in the switch >>=20 >> - can you some comments in the code for the chunk at @@ -2117,6 = +2391,42 @@ >> I do not completely understand why you are returning if the *queue = tag >> in the mbuf is out of range (my impression is that you should >> just continue, or if you think the packet is incorrect it should >> be filtered out before entering the LIST_FOREACH() ). >> Secondly, you should use the cached value of *queue_len >>=20 >>=20 >>=20 >> cheers >> luigi >>=20 >>=20 >> -- >> = -----------------------------------------+------------------------------- >> Prof. Luigi RIZZO, rizzo@iet.unipi.it . Dip. di Ing. = dell'Informazione >> http://www.iet.unipi.it/~luigi/ . Universita` di Pisa >> TEL +39-050-2211611 . via Diotisalvi 2 >> Mobile +39-338-6809875 . 56122 PISA (Italy) >>=20 >> = -----------------------------------------+------------------------------- >>=20 >>=20 > = _______________________________________________ > freebsd-net@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-net > To unsubscribe, send any mail to "freebsd-net-unsubscribe@freebsd.org" --Apple-Mail=_A45CAE38-AC98-44E2-9683-BD22C3A8BE5A Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename=signature.asc Content-Type: application/pgp-signature; name=signature.asc Content-Description: Message signed with OpenPGP using GPGMail -----BEGIN PGP SIGNATURE----- Comment: GPGTools - http://gpgtools.org iEUEARECAAYFAlJG+pgACgkQYdh2wUQKM9KHGwCXTrt6bufqJtU9VaobGP7/7eWo 6gCePwVX7l60Wv9PLuDv/lgk99gSgjQ= =ddBo -----END PGP SIGNATURE----- --Apple-Mail=_A45CAE38-AC98-44E2-9683-BD22C3A8BE5A--