'Re: Multiqueue support for bpf'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       freebsd-net
Subject:    Re: Multiqueue support for bpf
From:       George Neville-Neil <gnn () neville-neil ! com>
Date:       2013-09-28 15:49:44
Message-ID: C21D071A-EE32-4D50-8F97-51D7FCABE8EF () neville-neil ! com
[Download RAW message or body]

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,
> 
> 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 <rizzo@iet.unipi.it>
> 
>> 
>> 
>> 
>> On Tue, Jul 2, 2013 at 5:56 PM, Takuya ASADA <syuu@dokukino.com> 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, 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)
>> 
>> -----------------------------------------+-------------------------------
>> 
>> 
> <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"

["signature.asc" (signature.asc)]

-----BEGIN PGP SIGNATURE-----
Comment: GPGTools - http://gpgtools.org

iEUEARECAAYFAlJG+pgACgkQYdh2wUQKM9KHGwCXTrt6bufqJtU9VaobGP7/7eWo
6gCePwVX7l60Wv9PLuDv/lgk99gSgjQ=
=ddBo
-----END PGP SIGNATURE-----

[prev in list] [next in list] [prev in thread] [next in thread] 