Multiqueue support for bpf
rizzo at iet.unipi.it
Tue Jul 2 21:15:56 UTC 2013
On Tue, Jul 2, 2013 at 5:56 PM, Takuya ASADA <syuu at dokukino.com> wrote:
> Do you have an updated URL for the diffs ? The link below from your
>> original message
>> seems not working now (NXDOMAIN)
> Changes with recent head is on my repository:
> 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
=== 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
=== 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)
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
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)
More information about the freebsd-net