Date: Sat, 28 Sep 2013 11:49:44 -0400 From: George Neville-Neil <gnn@neville-neil.com> To: Takuya ASADA <syuu@dokukino.com> Cc: FreeBSD Net <freebsd-net@freebsd.org>, Luigi Rizzo <rizzo@iet.unipi.it> Subject: Re: Multiqueue support for bpf Message-ID: <C21D071A-EE32-4D50-8F97-51D7FCABE8EF@neville-neil.com> In-Reply-To: <CALG4x-UVgT9aXSBzrjDxeCD-f6Yo_TBeRsqjXapz2iyr_=tCLw@mail.gmail.com> References: <CALG4x-V-OLoqMXQarSNy5Lv3kNVu01AiN4A49Nv7t-Ysfr1DBg@mail.gmail.com> <CA%2BhQ2%2BgwW6FOQS79xmWVLSWWHrZMFnhaUM98Kp6aDVaUePNfTA@mail.gmail.com> <CALG4x-UYBFsMttpZx1-c_wtVf5MST8%2B_t1psY2HQskTiOZDFLA@mail.gmail.com> <CA%2BhQ2%2Bi_qu7RouPW%2Bihfb5nL_1SQWMpFxTpoHfhaCvhtS8-EHQ@mail.gmail.com> <CALG4x-UVgT9aXSBzrjDxeCD-f6Yo_TBeRsqjXapz2iyr_=tCLw@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--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 <syuu@dokukino.com> 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 <rizzo@iet.unipi.it> >=20 >>=20 >>=20 >>=20 >> On Tue, Jul 2, 2013 at 5:56 PM, Takuya ASADA <syuu@dokukino.com> = 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 > = <mq_bpf_201309041611.diff>_______________________________________________ > 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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?C21D071A-EE32-4D50-8F97-51D7FCABE8EF>