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 <[email protected]> wrote: > Hi, > > 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: > > - 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. > > - Use BITSET for queue mask. > It seems better to use BITSET for queue mask structure, instead of boolean > array. > > - Drop tcpdump/libpcap changes. > It also should discuss separately. > > - 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. > > > > 2013/7/3 Luigi Rizzo <[email protected]> > >> >> >> >> On Tue, Jul 2, 2013 at 5:56 PM, Takuya ASADA <[email protected]> wrote: >> >>> Hi, >>> >>> Do you have an updated URL for the diffs ? The link below from your >>>> original message >>>> seems not working now (NXDOMAIN) >>>> >>>> http://www.dokukino.com/mq_bpf_20110813.diff >>>> >>> >>> 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. >>> >>> >> thanks for the diffs (the URL to the repo is useful too, >> but a URL to generate diffs is more convenient for reviewing changes). >> >> I believe it still needs a bit of work before being merged. >> >> My comments (in order of the patch): >> >> === ifnet.9 (and related code in if.c, sockio.h) === >> - 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. >> >> - 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) >> >> === bpf.4 (and related code) === >> >> - 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". >> >> - 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) >> >> +#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) >> >> 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. >> >> === if.c === >> >> >> - 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.) >> >> === mbuf.h === >> >> 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 ? >> >> >> === if.h === >> >> - 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. >> >> - 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 >> >> >> === bpf.c === >> >> - 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. >> >> - in terms of code, the six BIOC*XQMASK are very similar, you are probably >> better off having one single case in the switch >> >> - 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 >> >> >> >> cheers >> luigi >> >> >> -- >> -----------------------------------------+------------------------------- >> Prof. Luigi RIZZO, [email protected] . 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) >> >> -----------------------------------------+------------------------------- >> >> > <mq_bpf_201309041611.diff>_______________________________________________ > [email protected] mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-net > To unsubscribe, send any mail to "[email protected]"
signature.asc
Description: Message signed with OpenPGP using GPGMail
