Multiqueue support for bpf

Wed Sep 4 08:05:04 UTC 2013

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 at iet.unipi.it>

>
>
>
> On Tue, Jul 2, 2013 at 5:56 PM, Takuya ASADA <syuu at 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 at 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)
>
>  -----------------------------------------+-------------------------------
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mq_bpf_201309041611.diff
Type: application/octet-stream
Size: 18591 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-net/attachments/20130904/a824e20b/attachment.obj>