* [PATCH] net: add __must_check to sk_add_backlog
From: Zhu Yi @ 2010-03-08 2:21 UTC (permalink / raw)
To: davem; +Cc: netdev, Zhu Yi
Add the "__must_check" tag to sk_add_backlog() so that any failure to
check and drop packets will be warned about.
Signed-off-by: Zhu Yi <yi.zhu@intel.com>
---
include/net/sock.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/include/net/sock.h b/include/net/sock.h
index 170353d..092b055 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -604,7 +604,7 @@ static inline void __sk_add_backlog(struct sock *sk, struct sk_buff *skb)
}
/* The per-socket spinlock must be held here. */
-static inline int sk_add_backlog(struct sock *sk, struct sk_buff *skb)
+static inline __must_check int sk_add_backlog(struct sock *sk, struct sk_buff *skb)
{
if (sk->sk_backlog.len >= max(sk->sk_backlog.limit, sk->sk_rcvbuf << 1))
return -ENOBUFS;
--
1.6.3.3
^ permalink raw reply related
* Re: [RFC][ PATCH 3/3] vhost-net: Add mergeable RX buffer support to vhost-net
From: David Stevens @ 2010-03-08 2:06 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, netdev, rusty, virtualization
In-Reply-To: <20100307162633.GC24997@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> wrote on 03/07/2010 08:26:33 AM:
> On Tue, Mar 02, 2010 at 05:20:34PM -0700, David Stevens wrote:
> > This patch glues them all together and makes sure we
> > notify whenever we don't have enough buffers to receive
> > a max-sized packet, and adds the feature bit.
> >
> > Signed-off-by: David L Stevens <dlstevens@us.ibm.com>
>
> Maybe split this up?
I can. I was looking mostly at size (and this is the smallest
of the bunch). But the feature requires all of them together, of course.
This last one is just "everything left over" from the other two.
> > @@ -110,6 +90,7 @@
> > size_t len, total_len = 0;
> > int err, wmem;
> > struct socket *sock = rcu_dereference(vq->private_data);
> > +
>
> I tend not to add empty lines if line below it is already short.
This leaves no blank line between the declarations and the start
of code. It's habit for me-- not sure of kernel coding standards address
that or not, but I don't think I've seen it anywhere else.
>
> > if (!sock)
> > return;
> >
> > @@ -166,11 +147,11 @@
> > /* Skip header. TODO: support TSO. */
> > msg.msg_iovlen = out;
> > head.iov_len = len = iov_length(vq->iov, out);
> > +
>
> I tend not to add empty lines if line below it is a comment.
I added this to separate the logical "skip header" block from
the next, unrelated piece. Not important to me, though.
>
> > /* Sanity check */
> > if (!len) {
> > vq_err(vq, "Unexpected header len for TX: "
> > - "%zd expected %zd\n",
> > - len, vq->guest_hlen);
> > + "%zd expected %zd\n", len,
vq->guest_hlen);
> > break;
> > }
> > /* TODO: Check specific error and bomb out unless
ENOBUFS?
> > */
> > /* TODO: Should check and handle checksum. */
> > + if (vhost_has_feature(&net->dev,
VIRTIO_NET_F_MRG_RXBUF))
> > {
> > + struct virtio_net_hdr_mrg_rxbuf *vhdr =
> > + (struct virtio_net_hdr_mrg_rxbuf *)
> > + vq->iov[0].iov_base;
> > + /* add num_bufs */
> > + vq->iov[0].iov_len = vq->guest_hlen;
> > + vhdr->num_buffers = headcount;
>
> I don't understand this. iov_base is a userspace pointer, isn't it.
> How can you assign values to it like that?
> Rusty also commented earlier that it's not a good idea to assume
> specific layout, such as first chunk being large enough to
> include virtio_net_hdr_mrg_rxbuf.
>
> I think we need to use memcpy to/from iovec etc.
I guess you mean put_user() or copy_to_user(); yes, I suppose
it could be paged since we read it.
The code doesn't assume that it'll fit so much as arranged for
it to fit. We allocate guest_hlen bytes in the buffer, but set the
iovec to the (smaller) sock_hlen; do the read, then this code adds
back the 2 bytes in the middle that we didn't read into (where
num_buffers goes). But the allocator does require that guest_hlen
will fit in a single buffer (and reports error if it doesn't). The
alternative is significantly more complicated, and only fails if
the guest doesn't give us at least the buffer size the guest header
requires (a truly lame guest). I'm not sure it's worth a lot of
complexity in vhost to support the guest giving us <12 byte buffers;
those guests don't exist now and maybe they never should?
> > /* This actually signals the guest, using eventfd. */
> > void vhost_signal(struct vhost_dev *dev, struct vhost_virtqueue *vq)
> > {
> > __u16 flags = 0;
> > +
>
> I tend not to add empty lines if a line above it is already short.
Again, separating declarations from code-- never seen different
in any other kernel code.
>
> > if (get_user(flags, &vq->avail->flags)) {
> > vq_err(vq, "Failed to get flags");
> > return;
> > @@ -1125,7 +1140,7 @@
> >
> > /* If they don't want an interrupt, don't signal, unless
empty. */
> > if ((flags & VRING_AVAIL_F_NO_INTERRUPT) &&
> > - (vq->avail_idx != vq->last_avail_idx ||
> > + (vhost_available(vq) > vq->maxheadcount ||
>
> I don't understand this change. It seems to make
> code not match the comments.
It redefines "empty". Without mergeable buffers, we can empty
the ring down to nothing before we require notification. With
mergeable buffers, if the packet requires, say, 3 buffers, and we
have only 2 left, we are empty and require notification and new
buffers to read anything. In both cases, we notify when we can't
read another packet without more buffers.
I can think about changing the comment to reflect this more
clearly.
>
> > !vhost_has_feature(dev, VIRTIO_F_NOTIFY_ON_EMPTY)))
> > return;
> >
> > diff -ruN net-next-p2/drivers/vhost/vhost.h
> > net-next-p3/drivers/vhost/vhost.h
> > --- net-next-p2/drivers/vhost/vhost.h 2010-03-02 13:02:03.000000000
> > -0800
> > +++ net-next-p3/drivers/vhost/vhost.h 2010-03-02 14:29:44.000000000
> > -0800
> > @@ -85,6 +85,7 @@
> > struct iovec iov[VHOST_NET_MAX_SG+1]; /* an extra for vnet hdr
*/
> > struct iovec heads[VHOST_NET_MAX_SG];
> > size_t guest_hlen, sock_hlen;
> > + int maxheadcount;
>
> I don't completely understand what does this field does.
> It seems to be only set on rx? Maybe name should reflect this?
This is a way for me to dynamically guess how many heads I need
for a
max-sized packet for whatever the MTU/GRO settings are without waiting to
detect the need for more buffers until a read fails. Without mergeable
buffers,
we can always fit a max-sized packet in 1 head, but with, we need more
than
one head to do the read.
I didn't want to hard-code 64K (which it usually is, but not always), and
I didn't want to wait until a read fails every time the ring is near full.
I played with re-enabling notify on 1/4 available or some such, but that
delays
reads unnecessarily, so I came up with this method: use maxheadcount to
track
the biggest packet we've ever seen and always make sure we have at least
that
many available for the next read. If it increases, we may fail the read,
which'll
notify, but this allows us to notify before we try and fail in normal
operation,
while still not doing a notify on every read.
+-DLS
^ permalink raw reply
* Re: [RFC][ PATCH 2/3] vhost-net: handle vnet_hdr processing for MRG_RX_BUF
From: David Stevens @ 2010-03-08 1:28 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, netdev, rusty, virtualization
In-Reply-To: <20100307161229.GB24997@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> wrote on 03/07/2010 08:12:29 AM:
> On Tue, Mar 02, 2010 at 05:20:26PM -0700, David Stevens wrote:
> > This patch adds vnet_hdr processing for mergeable buffer
> > support to vhost-net.
> >
> > Signed-off-by: David L Stevens <dlstevens@us.ibm.com>
> >
> > diff -ruN net-next-p1/drivers/vhost/net.c
net-next-p2/drivers/vhost/net.c
>
> Could you please add -p to diff flags so that it's easier
> to figure out which function is changes?
Sure.
> > @@ -148,25 +146,45 @@
> > "out %d, int %d\n", out, in);
> > break;
> > }
> > + if (vq->guest_hlen > vq->sock_hlen) {
> > + if (msg.msg_iov[0].iov_len == vq->guest_hlen)
> > + msg.msg_iov[0].iov_len =
vq->sock_hlen;
> > + else if (out == ARRAY_SIZE(vq->iov))
> > + vq_err(vq, "handle_tx iov overflow!");
> > + else {
> > + int i;
> > +
> > + /* give header its own iov */
> > + for (i=out; i>0; ++i)
> > + msg.msg_iov[i+1] =
msg.msg_iov[i];
> > + msg.msg_iov[0].iov_len =
vq->sock_hlen;
> > + msg.msg_iov[1].iov_base +=
vq->guest_hlen;
> > + msg.msg_iov[1].iov_len -=
vq->guest_hlen;
> > + out++;
>
> We seem to spend a fair bit of code here and elsewhere trying to cover
> the diff between header size in guest and host. In hindsight, it was
> not a good idea to put new padding between data and the header:
> virtio-net should have added it *before*. But it is what it is.
>
> Wouldn't it be easier to just add an ioctl to tap so that
> vnet header size is made bigger by 4 bytes?
I'd be ok with that, but if we support raw sockets, we have
some of the issues (easier, since it's 0). "num_buffers" is only
16 bits, so just add 2 bytes, but yeah-- pushing it to tap would
be easier for vhost.
> > /* TODO: Check specific error and bomb out unless
ENOBUFS?
> > */
> > err = sock->ops->sendmsg(NULL, sock, &msg, len);
> > if (unlikely(err < 0)) {
> > - vhost_discard(vq, 1);
> > - tx_poll_start(net, sock);
> > + if (err == -EAGAIN) {
>
> The comment mentions ENOBUFS. Are you sure it's EAGAIN?
The comment's from the original -- the code changes should do
the "TODO", so probably should remove the comment.
>
> > + tx_poll_start(net, sock);
>
> Don't we need to call discard to move the last avail header back?
Yes, I think you're right. We might consider dropping the packet
in the EAGAIN case here instead, though. It's not clear retrying the
same packet with a delay here is better than getting new ones when the
socket buffer is full (esp. with queues on both sides already). Maybe
we should fold EAGAIN into the other error cases? Otherwise, you're
right, it looks like it should have a discard here.
>
> > + } else {
> > + vq_err(vq, "sendmsg: errno %d\n",
-err);
> > + /* drop packet; do not discard/resend
*/
> > + vhost_add_used_and_signal(&net->dev,vq,&head,1);
> > + }
> > break;
> > - }
> > - if (err != len)
> > + } else if (err != len)
>
>
> Previous if ends with break so no need for else here.
Yes.
> > @@ -232,25 +243,18 @@
> > headcount = vhost_get_heads(vq, datalen, &in, vq_log,
> > &log);
> > /* OK, now we need to know about added descriptors. */
> > if (!headcount) {
> > - if (unlikely(vhost_enable_notify(vq))) {
> > - /* They have slipped one in as we were
> > - * doing that: check again. */
> > - vhost_disable_notify(vq);
> > - continue;
> > - }
> > - /* Nothing new? Wait for eventfd to tell us
> > - * they refilled. */
> > + vhost_enable_notify(vq);
>
>
> You don't think this race can happen?
The notify case has to allow for multiple heads, not just the one,
so if he added just one head, it doesn't mean we're ok to continue-- the
packet may require multiple. Instead, the notifier now checks for enough
to handle a max-sized packet (whether or not NONOTIFY is set). I'll think
about this some more, but I concluded it wasn't needed at the time. :-)
> > @@ -519,6 +524,20 @@
> >
> > vhost_net_disable_vq(n, vq);
> > rcu_assign_pointer(vq->private_data, sock);
> > +
> > + if (sock && sock->sk) {
> > + if (!vhost_sock_is_raw(sock) ||
>
> I dislike this backend-specific code, it ties us with specifics of
> backend implementations, which change without notice. Ideally all
> backend-specific stuff should live in userspace, userspace programs
> vhost device appropriately.
We could do that; we need to know whether the socket has a
vnet header on it or not and the existing flags don't tell us. If
qemu sets a flag telling us the socket is has or doesn't have vnet,
that'd be equivalent.
> > + vhost_has_feature(&n->dev,
> > VHOST_NET_F_VIRTIO_NET_HDR)) {
> > + vq->sock_hlen = sizeof(struct virtio_net_hdr);
> > + if (vhost_has_feature(&n->dev,
> > VIRTIO_NET_F_MRG_RXBUF))
> > + vq->guest_hlen =
> > + sizeof(struct
> > virtio_net_hdr_mrg_rxbuf);
> > + else
> > + vq->guest_hlen = sizeof(struct
> > virtio_net_hdr);
> > + } else
> > + vq->guest_hlen = vq->sock_hlen = 0;
> > + } else
> > + vq_err(vq, "vhost_net_set_backend: sock->sk is NULL");
>
> As proposed above, IMO it would be nicer to add an ioctl in tap
> that let us program header size.
Do you know if Herbert or Dave have an opinion on that solution?
> > +static int
> > +vhost_get_hdr(struct vhost_virtqueue *vq, int *in, struct vhost_log
*log,
> > + int *log_num)
> > +{
> > + struct iovec *heads = vq->heads;
> > + struct iovec *iov = vq->iov;
> > + int out;
> > +
> > + *in = 0;
> > + iov[0].iov_len = 0;
> > +
> > + /* get buffer, starting from iov[1] */
> > + heads[0].iov_base = (void *)vhost_get_vq_desc(vq->dev, vq,
> > + vq->iov+1, ARRAY_SIZE(vq->iov)-1, &out, in, log,
log_num);
> > + if (out || *in <= 0) {
> > + vq_err(vq, "unexpected descriptor format for RX: out
%d, "
> > + "in %d\n", out, *in);
> > + return 0;
> > + }
> > + if (heads[0].iov_base == (void *)vq->num)
> > + return 0;
> > +
> > + /* make iov[0] the header */
> > + if (!vq->guest_hlen) {
> > + if (vq->sock_hlen) {
> > + static struct virtio_net_hdr junk; /* bit
bucket
> > */
> > +
> > + iov[0].iov_base = &junk;
> > + iov[0].iov_len = sizeof(junk);
> > + } else
> > + iov[0].iov_len = 0;
> > + }
> > + if (vq->sock_hlen < vq->guest_hlen) {
> > + iov[0].iov_base = iov[1].iov_base;
> > + iov[0].iov_len = vq->sock_hlen;
> > +
> > + if (iov[1].iov_len < vq->sock_hlen) {
> > + vq_err(vq, "can't fit header in one buffer!");
> > + vhost_discard(vq, 1);
> > + return 0;
> > + }
> > + if (!vq->sock_hlen) {
> > + static const struct virtio_net_hdr_mrg_rxbuf
hdr =
> > {
> > + .hdr.flags = 0,
> > + .hdr.gso_type =
VIRTIO_NET_HDR_GSO_NONE
> > + };
> > + memcpy(iov[0].iov_base, &hdr, vq->guest_hlen);
> > + }
> > + iov[1].iov_base += vq->guest_hlen;
> > + iov[1].iov_len -= vq->guest_hlen;
> > + }
> > + return 1;
>
> The above looks kind of scary, lots of special-casing. I'll send a
> patch for tap to set vnet header size, let's see if it makes life easier
> for us.
Yes, I try to handle all combinations of differing guest and
socket
header lengths (including 0-lengths). If we don't support raw sockets
and/or
we make the tap vnet header bigger in the MRXB case, it'll simplify this
code.
>
> > +}
> > +
> > unsigned vhost_get_heads(struct vhost_virtqueue *vq, int datalen, int
> > *iovcount,
> > struct vhost_log *log, unsigned int *log_num)
> > {
> > struct iovec *heads = vq->heads;
> > - int out, in;
> > + int out, in = 0;
> > + int seg = 0;
> > int hc = 0;
>
> I think it's better to stick to simple names like i
> for index variables, alternatively give it a meaningful
> name like "count".
I'm not sure which one you're talking about. "in & out"
are inherited from the original, "seg" is an iovec segment index, and
"hc" is a head counter. "headcount" or "count" (if that's the
one you mean) instead of "hc" would probably cause some line wraps.
I think "i" is too generic here, but if you mean "hc", I could stick
a comment on the declaration:
int hc = 0; /* head count */
Does that address your concern, or do you mean something else?
>
> >
> > + if (vq->guest_hlen != vq->sock_hlen) {
>
> Sticking guest_hlen/sock_hlen in vhost is somewhat ugly.
Do you mean in the struct? This essentially splits the
original "hdrsize" into the two (possibly different) header
sizes and allows removing the "hdr" iovec and reading the
(partial) header directly into the extended header mergeable
buffers needs. Unless we remove raw socket support and add the
ioctl to tap you suggested, I'm not sure it can be much cleaner.
For me, the ugliness comes from tap, raw and guest headers not
being the same.
+-DLS
^ permalink raw reply
* Re: [RFC][ PATCH 1/3] vhost-net: support multiple buffer heads in receiver
From: David Stevens @ 2010-03-08 0:34 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, netdev, rusty, virtualization
In-Reply-To: <20100307153130.GA24997@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> wrote on 03/07/2010 07:31:30 AM:
> On Tue, Mar 02, 2010 at 05:20:15PM -0700, David Stevens wrote:
> > This patch generalizes buffer handling functions to
NULL, NULL);
> > + head.iov_base = (void *)vhost_get_vq_desc(&net->dev,
vq,
> > + vq->iov, ARRAY_SIZE(vq->iov), &out, &in, NULL,
> > NULL);
>
> Should type for head be changed so that we do not need to cast?
>
> I also prefer aligning descendants on the opening (.
Yes, that's probably better; the indentation with the cast would
still wrap a lot, but I'll see what I can do here.
> > err = sock->ops->sendmsg(NULL, sock, &msg, len);
> > if (unlikely(err < 0)) {
> > - vhost_discard_vq_desc(vq);
> > + vhost_discard(vq, 1);
>
> Isn't the original name a bit more descriptive?
During development, I had both and I generally like
shorter names, but I can change it back.
> > +static int skb_head_len(struct sk_buff_head *skq)
> > +{
> > + struct sk_buff *head;
> > +
> > + head = skb_peek(skq);
> > + if (head)
> > + return head->len;
> > + return 0;
> > +}
> > +
>
> This is done without locking, which I think can crash
> if skb is consumed after we peek it but before we read the
> length.
This thread is the only legitimate consumer, right? But
qemu has the file descriptor and I guess we shouldn't trust
that it won't give it to someone else; it'd break vhost, but
a crash would be inappropriate, of course. I'd like to avoid
the lock, but I have another idea here, so will investigate.
>
>
> > /* Expects to be always run from workqueue - which acts as
> > * read-size critical section for our kind of RCU. */
> > static void handle_rx(struct vhost_net *net)
> > {
> > struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_RX];
> > - unsigned head, out, in, log, s;
> > + unsigned in, log, s;
> > struct vhost_log *vq_log;
> > struct msghdr msg = {
> > .msg_name = NULL,
> > @@ -204,10 +213,11 @@
> > };
> >
> > size_t len, total_len = 0;
> > - int err;
> > + int err, headcount, datalen;
> > size_t hdr_size;
> > struct socket *sock = rcu_dereference(vq->private_data);
> > - if (!sock || skb_queue_empty(&sock->sk->sk_receive_queue))
> > +
> > + if (!sock || !skb_head_len(&sock->sk->sk_receive_queue))
> > return;
> >
>
> Isn't this equivalent? Do you expect zero len skbs in socket?
> If yes, maybe we should discard these, not stop processing ...
A zero return means "no skb's". They are equivalent; I
changed this call just to make it identical to the loop check,
but I don't have a strong attachment to this.
> > vq_log = unlikely(vhost_has_feature(&net->dev,
VHOST_F_LOG_ALL)) ?
> > vq->log : NULL;
> >
> > - for (;;) {
> > - head = vhost_get_vq_desc(&net->dev, vq, vq->iov,
> > - ARRAY_SIZE(vq->iov),
> > - &out, &in,
> > - vq_log, &log);
> > + while ((datalen = skb_head_len(&sock->sk->sk_receive_queue)))
{
>
> This peeks in the queue to figure out how much data we have.
> It's a neat trick, but it does introduce new failure modes
> where an skb is consumed after we did the peek:
> - new skb could be shorter, we should return the unconsumed part
> - new skb could be longer, this will drop a packet.
> maybe this last is not an issue as the race should be rare in practice
As before, this loop is the only legitimate consumer of skb's; if
anyone else is reading the socket, it's broken. But since the file
descriptor is available to qemu, it's probably trusting qemu too much.
I don't believe there is a race at all on a working system; the head
can't change until we read it after this test, but I'll see if I can
come up with something better here. Closing the fd for qemu so vhost
has exclusive access might do it, but otherwise we could just get a
max-sized packet worth of buffers and then return them after the read.
That'd force us to wait with small packets when the ring is nearly
full, where we don't have to now, but I expect locking to read the head
length will hurt performance in all cases. Will try these ideas out.k
>
> > + headcount = vhost_get_heads(vq, datalen, &in, vq_log,
> > &log);
> > /* OK, now we need to know about added descriptors. */
> > - if (head == vq->num) {
> > + if (!headcount) {
> > if (unlikely(vhost_enable_notify(vq))) {
> > /* They have slipped one in as we were
> > * doing that: check again. */
> > @@ -235,13 +242,6 @@
> > * they refilled. */
> > break;
> > }
> > - /* We don't need to be notified again. */
>
> you find this comment unhelpful?
This code is changed in the later patches; the comment was
removed with that, but I got it in the wrong patch on the split.
I guess the comment is ok to stay, anyway, but notification may
require multiple buffers to be available; I had fixed that here, but
the final patch pushes that into the notify code, so yes, this can
go back in.
> > - if (out) {
> > - vq_err(vq, "Unexpected descriptor format for
RX: "
> > - "out %d, int %d\n",
> > - out, in);
> > - break;
> > - }
>
>
> we still need this check, don't we?
It's done in vhost_get_heads(); "out" isn't visible in handle_rx()
anymore.
> > +unsigned vhost_get_heads(struct vhost_virtqueue *vq, int datalen, int
> > *iovcount,
> > + struct vhost_log *log, unsigned int *log_num)
>
> Could you please document this function's parameters? It's not obvious
> what iovcount, log, log_num are.
Yes. To answer the question, they are the same as vhost_get_vq_desc(),
except "iovcount" replaces "in" (and "out" is not needed in the caller).
> > +{
> > + struct iovec *heads = vq->heads;
> > + int out, in;
> > + int hc = 0;
> > +
> > + while (datalen > 0) {
> > + if (hc >= VHOST_NET_MAX_SG) {
> > + vhost_discard(vq, hc);
> > + return 0;
> > + }
> > + heads[hc].iov_base = (void
*)vhost_get_vq_desc(vq->dev,
> > vq,
> > + vq->iov, ARRAY_SIZE(vq->iov), &out, &in, log,
> > log_num);
> > + if (heads[hc].iov_base == (void *)vq->num) {
> > + vhost_discard(vq, hc);
> > + return 0;
> > + }
> > + if (out || in <= 0) {
> > + vq_err(vq, "unexpected descriptor format for
RX: "
> > + "out %d, in %d\n", out, in);
> > + vhost_discard(vq, hc);
> > + return 0;
> > + }
> > + heads[hc].iov_len = iov_length(vq->iov, in);
> > + hc++;
> > + datalen -= heads[hc].iov_len;
> > + }
> > + *iovcount = in;
>
>
> Only the last value?
In this part of the split, it only goes through once; this is
changed to the accumulated value "seg" in a later patch. So, split
artifact.
{
> > struct vring_used_elem *used;
> > + int i;
> >
> > - /* The virtqueue contains a ring of used buffers. Get a
pointer
> > to the
> > - * next entry in that used ring. */
> > - used = &vq->used->ring[vq->last_used_idx % vq->num];
> > - if (put_user(head, &used->id)) {
> > - vq_err(vq, "Failed to write used id");
> > - return -EFAULT;
> > - }
> > - if (put_user(len, &used->len)) {
> > - vq_err(vq, "Failed to write used len");
> > - return -EFAULT;
> > + for (i=0; i<count; i++, vq->last_used_idx++) {
>
> whitespace damage: I prefer space around =, <.
> I also use ++i, etc in this driver, better be consistent?
> Also for clarity, I prefer to put vq->last_used_idx inside the loop.
OK.
>
> > + used = &vq->used->ring[vq->last_used_idx % vq->num];
> > + if (put_user((unsigned)heads[i].iov_base, &used->id))
{
> > + vq_err(vq, "Failed to write used id");
> > + return -EFAULT;
> > + }
> > + if (put_user(heads[i].iov_len, &used->len)) {
> > + vq_err(vq, "Failed to write used len");
> > + return -EFAULT;
> > + }
>
> If this fails, last_used_idx will still be incremented, which I think is
wrong.
True, but if these fail, aren't we dead? I don't think we can
recover
from an EFAULT in any of these; I didn't test those paths from the
original,
but I think we need to bail out entirely for these cases, right?
+-DLS
^ permalink raw reply
* [PATCH 2/2] be2net: remove unused code in be_load_fw
From: Ajit Khaparde @ 2010-03-08 0:23 UTC (permalink / raw)
To: David Miller; +Cc: netdev
This patch cleans up some unused code from be_load_fw().
Signed-off-by: Ajit Khaparde <ajitk@serverengines.com>
---
drivers/net/benet/be_main.c | 9 ---------
1 files changed, 0 insertions(+), 9 deletions(-)
diff --git a/drivers/net/benet/be_main.c b/drivers/net/benet/be_main.c
index 7c9b57e..43e8032 100644
--- a/drivers/net/benet/be_main.c
+++ b/drivers/net/benet/be_main.c
@@ -1993,16 +1993,7 @@ int be_load_fw(struct be_adapter *adapter, u8 *func)
struct be_dma_mem flash_cmd;
int status, i = 0;
const u8 *p;
- char fw_ver[FW_VER_LEN];
- char fw_cfg;
- status = be_cmd_get_fw_ver(adapter, fw_ver);
- if (status)
- return status;
-
- fw_cfg = *(fw_ver + 2);
- if (fw_cfg == '0')
- fw_cfg = '1';
strcpy(fw_file, func);
status = request_firmware(&fw, fw_file, &adapter->pdev->dev);
--
1.6.3.3
^ permalink raw reply related
* [PATCH 1/2] be2net: remove usage of be_pci_func
From: Ajit Khaparde @ 2010-03-08 0:21 UTC (permalink / raw)
To: David Miller; +Cc: netdev
When PCI functions are virtuialized in applications by assigning PCI
functions to VM (PCI passthrough), the be2net driver in the VM sees a
different function number. So, use of PCI function number in any
calculation will break existing code. This patch takes care of it.
Signed-off-by: Ajit Khaparde <ajitk@serverengines.com>
---
drivers/net/benet/be.h | 5 -----
drivers/net/benet/be_cmds.c | 6 ------
drivers/net/benet/be_main.c | 2 +-
3 files changed, 1 insertions(+), 12 deletions(-)
diff --git a/drivers/net/benet/be.h b/drivers/net/benet/be.h
index be81fb2..8f07525 100644
--- a/drivers/net/benet/be.h
+++ b/drivers/net/benet/be.h
@@ -290,11 +290,6 @@ extern const struct ethtool_ops be_ethtool_ops;
#define drvr_stats(adapter) (&adapter->stats.drvr_stats)
-static inline unsigned int be_pci_func(struct be_adapter *adapter)
-{
- return PCI_FUNC(adapter->pdev->devfn);
-}
-
#define BE_SET_NETDEV_OPS(netdev, ops) (netdev->netdev_ops = ops)
#define PAGE_SHIFT_4K 12
diff --git a/drivers/net/benet/be_cmds.c b/drivers/net/benet/be_cmds.c
index 4b1f805..c592153 100644
--- a/drivers/net/benet/be_cmds.c
+++ b/drivers/net/benet/be_cmds.c
@@ -465,8 +465,6 @@ int be_cmd_eq_create(struct be_adapter *adapter,
req->num_pages = cpu_to_le16(PAGES_4K_SPANNED(q_mem->va, q_mem->size));
- AMAP_SET_BITS(struct amap_eq_context, func, req->context,
- be_pci_func(adapter));
AMAP_SET_BITS(struct amap_eq_context, valid, req->context, 1);
/* 4byte eqe*/
AMAP_SET_BITS(struct amap_eq_context, size, req->context, 0);
@@ -629,7 +627,6 @@ int be_cmd_cq_create(struct be_adapter *adapter,
AMAP_SET_BITS(struct amap_cq_context, eventable, ctxt, 1);
AMAP_SET_BITS(struct amap_cq_context, eqid, ctxt, eq->id);
AMAP_SET_BITS(struct amap_cq_context, armed, ctxt, 1);
- AMAP_SET_BITS(struct amap_cq_context, func, ctxt, be_pci_func(adapter));
be_dws_cpu_to_le(ctxt, sizeof(req->context));
be_cmd_page_addrs_prepare(req->pages, ARRAY_SIZE(req->pages), q_mem);
@@ -678,7 +675,6 @@ int be_cmd_mccq_create(struct be_adapter *adapter,
req->num_pages = PAGES_4K_SPANNED(q_mem->va, q_mem->size);
- AMAP_SET_BITS(struct amap_mcc_context, fid, ctxt, be_pci_func(adapter));
AMAP_SET_BITS(struct amap_mcc_context, valid, ctxt, 1);
AMAP_SET_BITS(struct amap_mcc_context, ring_size, ctxt,
be_encoded_q_len(mccq->len));
@@ -727,8 +723,6 @@ int be_cmd_txq_create(struct be_adapter *adapter,
AMAP_SET_BITS(struct amap_tx_context, tx_ring_size, ctxt,
be_encoded_q_len(txq->len));
- AMAP_SET_BITS(struct amap_tx_context, pci_func_id, ctxt,
- be_pci_func(adapter));
AMAP_SET_BITS(struct amap_tx_context, ctx_valid, ctxt, 1);
AMAP_SET_BITS(struct amap_tx_context, cq_id_send, ctxt, cq->id);
diff --git a/drivers/net/benet/be_main.c b/drivers/net/benet/be_main.c
index 22f787f..7c9b57e 100644
--- a/drivers/net/benet/be_main.c
+++ b/drivers/net/benet/be_main.c
@@ -1382,7 +1382,7 @@ rx_eq_free:
/* There are 8 evt ids per func. Retruns the evt id's bit number */
static inline int be_evt_bit_get(struct be_adapter *adapter, u32 eq_id)
{
- return eq_id - 8 * be_pci_func(adapter);
+ return eq_id % 8;
}
static irqreturn_t be_intx(int irq, void *dev)
--
1.6.3.3
^ permalink raw reply related
* Re: [PATCH 6/13] bridge: Add core IGMP snooping support
From: David Miller @ 2010-03-07 23:31 UTC (permalink / raw)
To: herbert; +Cc: paulmck, netdev, shemminger
In-Reply-To: <20100306070739.GB14470@gondor.apana.org.au>
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Sat, 6 Mar 2010 15:07:39 +0800
> On Sat, Mar 06, 2010 at 02:56:55PM +0800, Herbert Xu wrote:
>> On Fri, Mar 05, 2010 at 09:06:56PM -0800, Paul E. McKenney wrote:
>>
>> > Shouldn't the br_mdb_get() code path be using hlist_for_each_entry_rcu()
>> > in __br_mdb_ip_get(), then? Or is something else going on here?
>>
>> Indeed it should, I'll fix this up too.
>
> bridge: Use RCU list primitive in __br_mdb_ip_get
>
> As Paul McKenney correctly pointed out, __br_mdb_ip_get needs
> to use the RCU list walking primitive in order to work correctly
> on platforms where data-dependency ordering is not guaranteed.
>
> Reported-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Applied.
^ permalink raw reply
* Re: [PATCH 6/13] bridge: Add core IGMP snooping support
From: David Miller @ 2010-03-07 23:31 UTC (permalink / raw)
To: herbert; +Cc: paulmck, netdev, shemminger
In-Reply-To: <20100306070335.GA14470@gondor.apana.org.au>
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Sat, 6 Mar 2010 15:03:35 +0800
> On Sat, Mar 06, 2010 at 02:56:55PM +0800, Herbert Xu wrote:
>>
>> > In contrast, rcu_barrier_bh() is guaranteed not to return until all
>> > pending RCU-bh callbacks have executed.
>>
>> You're absolutely right. I'll send a patch to fix this.
>
> bridge: Fix RCU race in br_multicast_stop
>
> Thanks to Paul McKenny for pointing out that it is incorrect to use
> synchronize_rcu_bh to ensure that pending callbacks have completed.
> Instead we should use rcu_barrier_bh.
>
> Reported-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Applied.
^ permalink raw reply
* Re: [PATCH] fix a race in ks8695_poll
From: David Miller @ 2010-03-07 23:28 UTC (permalink / raw)
To: figo1802; +Cc: netdev, shemminger, yegorslists
In-Reply-To: <1267842962.2669.2.camel@myhost>
From: "Figo.zhang" <figo1802@gmail.com>
Date: Sat, 06 Mar 2010 10:36:02 +0800
> fix a race at the end of NAPI processing in ks8695_poll() function.
>
> Signed-off-by:Figo.zhang <figo1802@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH] MAINTAINER: Correct CAN Maintainer responsibilities and paths
From: David Miller @ 2010-03-07 23:28 UTC (permalink / raw)
To: wg; +Cc: socketcan, oliver.hartkopp, urs.thuermann, netdev, socketcan-core
In-Reply-To: <4B929551.3000901@grandegger.com>
From: Wolfgang Grandegger <wg@grandegger.com>
Date: Sat, 06 Mar 2010 18:48:01 +0100
> Oliver Hartkopp wrote:
>> Update the CAN Maintainer responsibilities and add source paths.
>> Additional the SocketCAN core ML is not subscribers-only anymore.
>>
>> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
...
> Acked-by: Wolfgang Grandegger <wg@grandegger.com>
Applied.
^ permalink raw reply
* Re: [patch] sock.c: potential null dereference
From: David Miller @ 2010-03-07 23:28 UTC (permalink / raw)
To: error27
Cc: netdev, eric.dumazet, jengelh, mingo, akpm, linux-kernel,
kernel-janitors
In-Reply-To: <20100306110445.GC4958@bicker>
From: Dan Carpenter <error27@gmail.com>
Date: Sat, 6 Mar 2010 14:04:45 +0300
> We test that "prot->rsk_prot" is non-null right before we dereference it
> on this line.
>
> Signed-off-by: Dan Carpenter <error27@gmail.com>
Applied.
^ permalink raw reply
* Re: [patch] ems_usb: cleanup: remove uneeded check
From: David Miller @ 2010-03-07 23:28 UTC (permalink / raw)
To: error27
Cc: urs.thuermann, oliver.hartkopp, wg, haas, kurt.van.dijck, netdev,
kernel-janitors
In-Reply-To: <20100306111138.GH4958@bicker>
From: Dan Carpenter <error27@gmail.com>
Date: Sat, 6 Mar 2010 14:11:38 +0300
> "skb" is alway non-null here, but even if it were null the check isn't
> needed because dev_kfree_skb() can handle it.
>
> This eliminates a smatch warning about dereferencing a variable before
> checking that it is non-null.
>
> Signed-off-by: Dan Carpenter <error27@gmail.com>
Applied.
^ permalink raw reply
* Re: [patch] bridge: cleanup: remove unneed check
From: David Miller @ 2010-03-07 23:27 UTC (permalink / raw)
To: herbert; +Cc: error27, shemminger, bridge, netdev, kernel-janitors
In-Reply-To: <20100306122031.GA15864@gondor.apana.org.au>
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Sat, 6 Mar 2010 20:20:32 +0800
> On Sat, Mar 06, 2010 at 02:14:09PM +0300, Dan Carpenter wrote:
>> We dereference "port" on the lines immediately before and immediately
>> after the test so port should hopefully never be null here.
>>
>> Signed-off-by: Dan Carpenter <error27@gmail.com>
>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>
> Yes this looks correct to me.
Applied.
^ permalink raw reply
* Re: [PATCH net-2.6] netfilter: Remove stale declaration for ip6_masked_addrcmp().
From: David Miller @ 2010-03-07 23:27 UTC (permalink / raw)
To: yoshfuji; +Cc: kaber, netfilter-devel, netdev
In-Reply-To: <201003070918.o279IfCk029612@94.43.138.210.xn.2iij.net>
From: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
Date: Sun, 7 Mar 2010 17:39:13 +0900
> Commit f2ffd9ee... ("[NETFILTER]: Move ip6_masked_addrcmp to
> include/net/ipv6.h") replaced ip6_masked_addrcmp() with
> ipv6_masked_addr_cmp(). Function definition went away.
>
> Let's remove its declaration as well in header file.
>
> Signed-off-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
Patrick, I assume you'll get this one too.
Thanks.
^ permalink raw reply
* Re: [PATCH net-2.6] netfilter: ebt_ip6: Use ipv6_masked_addr_cmp().
From: David Miller @ 2010-03-07 23:27 UTC (permalink / raw)
To: yoshfuji; +Cc: bart.de.schuymer, kaber, ebtables-devel, netfilter-devel, netdev
In-Reply-To: <201003070918.o279IXtq029570@94.43.138.210.xn.2iij.net>
From: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
Date: Sun, 7 Mar 2010 17:39:10 +0900
> Signed-off-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
Patrick, I assume you'll get this.
^ permalink raw reply
* Re: [PATCH net-next-2.6] ipv6: Optmize translation between IPV6_PREFER_SRC_xxx and RT6_LOOKUP_F_xxx.
From: David Miller @ 2010-03-07 23:26 UTC (permalink / raw)
To: yoshfuji; +Cc: netdev
In-Reply-To: <201003071014.o27AEio4001703@94.43.138.210.xn.2iij.net>
From: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
Date: Sun, 7 Mar 2010 19:14:44 +0900
> IPV6_PREFER_SRC_xxx definitions:
> | #define IPV6_PREFER_SRC_TMP 0x0001
> | #define IPV6_PREFER_SRC_PUBLIC 0x0002
> | #define IPV6_PREFER_SRC_COA 0x0004
>
> RT6_LOOKUP_F_xxx definitions:
> | #define RT6_LOOKUP_F_SRCPREF_TMP 0x00000008
> | #define RT6_LOOKUP_F_SRCPREF_PUBLIC 0x00000010
> | #define RT6_LOOKUP_F_SRCPREF_COA 0x00000020
>
> So, we can translate between these two groups by shift operation
> instead of multiple 'if's.
>
> Signed-off-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
Applied, thanks.
^ permalink raw reply
* Re: [patch] irda-usb: add error handling and fix leak
From: David Miller @ 2010-03-07 23:26 UTC (permalink / raw)
To: error27; +Cc: samuel, shemminger, kaber, tabe, julia, netdev, kernel-janitors
In-Reply-To: <20100307123542.GA6469@bicker>
From: Dan Carpenter <error27@gmail.com>
Date: Sun, 7 Mar 2010 15:35:42 +0300
> If the call to kcalloc() fails then we should return -ENOMEM.
>
> Signed-off-by: Dan Carpenter <error27@gmail.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH 0/3] cpmac fixes
From: David Miller @ 2010-03-07 23:26 UTC (permalink / raw)
To: florian; +Cc: netdev
In-Reply-To: <201003071155.17553.florian@openwrt.org>
From: Florian Fainelli <florian@openwrt.org>
Date: Sun, 7 Mar 2010 11:55:16 +0100
> Here are 2 small cpmac fixes and a driver version bumping. Thanks!
>
> Florian Fainelli (3):
> cpmac: fix the receiving of 802.1q frames
> cpmac: fallback to switch mode if no PHY chip found
> cpmac: bump version to 0.5.2
All applied, thank you.
^ permalink raw reply
* Re: [kernel,2.6.33-git11] lib8390: use spin_lock_irqsave for locking
From: David Miller @ 2010-03-07 23:22 UTC (permalink / raw)
To: ken_kawasaki; +Cc: netdev
This change is not correct.
disable_irq is being intentionally used because the reset
sequence can take a very long time and we don't want to
have cpu interrupts disabled during the entire sequence.
Otherwise slow serial devices will drop characters and
stuff like that.
^ permalink raw reply
* [patch] another pegasus usb net device
From: Petko Manolov @ 2010-03-07 16:10 UTC (permalink / raw)
To: netdev; +Cc: davem, julian, petkan
[-- Attachment #1: Type: TEXT/PLAIN, Size: 214 bytes --]
This one removes trailing whitespace in pegasus.h and more importantly
adds new Pegasus compatible device.
Signed-off-by: Julian Brown <julian@codesourcery.com>
Signed-off-by: Petko Manolov <petkan@nucleusys.com>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: TEXT/X-DIFF; NAME=pegasus-belkin-f5u122-1.diff, Size: 1634 bytes --]
diff --git a/drivers/net/usb/pegasus.h b/drivers/net/usb/pegasus.h
index 5d02f02..b90d876 100644
--- a/drivers/net/usb/pegasus.h
+++ b/drivers/net/usb/pegasus.h
@@ -177,7 +177,7 @@ PEGASUS_DEV( "USB 10/100 Fast Ethernet", VENDOR_ABOCOM, 0x400c,
PEGASUS_DEV( "USB 10/100 Fast Ethernet", VENDOR_ABOCOM, 0xabc1,
DEFAULT_GPIO_RESET )
PEGASUS_DEV( "USB 10/100 Fast Ethernet", VENDOR_ABOCOM, 0x200c,
- DEFAULT_GPIO_RESET | PEGASUS_II )
+ DEFAULT_GPIO_RESET | PEGASUS_II )
PEGASUS_DEV( "Accton USB 10/100 Ethernet Adapter", VENDOR_ACCTON, 0x1046,
DEFAULT_GPIO_RESET )
PEGASUS_DEV( "SpeedStream USB 10/100 Ethernet", VENDOR_ACCTON, 0x5046,
@@ -208,6 +208,8 @@ PEGASUS_DEV( "Allied Telesyn Int. AT-USB100", VENDOR_ALLIEDTEL, 0xb100,
*/
PEGASUS_DEV_CLASS( "Belkin F5D5050 USB Ethernet", VENDOR_BELKIN, 0x0121, 0x00,
DEFAULT_GPIO_RESET | PEGASUS_II )
+PEGASUS_DEV( "Belkin F5U122 10/100 USB Ethernet", VENDOR_BELKIN, 0x0122,
+ DEFAULT_GPIO_RESET | PEGASUS_II )
PEGASUS_DEV( "Billionton USB-100", VENDOR_BILLIONTON, 0x0986,
DEFAULT_GPIO_RESET )
PEGASUS_DEV( "Billionton USBLP-100", VENDOR_BILLIONTON, 0x0987,
@@ -249,7 +251,7 @@ PEGASUS_DEV( "GIGABYTE GN-BR402W Wireless Router", VENDOR_GIGABYTE, 0x8002,
PEGASUS_DEV( "Hawking UF100 10/100 Ethernet", VENDOR_HAWKING, 0x400c,
DEFAULT_GPIO_RESET | PEGASUS_II )
PEGASUS_DEV( "HP hn210c Ethernet USB", VENDOR_HP, 0x811c,
- DEFAULT_GPIO_RESET | PEGASUS_II )
+ DEFAULT_GPIO_RESET | PEGASUS_II )
PEGASUS_DEV( "IO DATA USB ET/TX", VENDOR_IODATA, 0x0904,
DEFAULT_GPIO_RESET )
PEGASUS_DEV( "IO DATA USB ET/TX-S", VENDOR_IODATA, 0x0913,
^ permalink raw reply related
* Re: [RFC][ PATCH 3/3] vhost-net: Add mergeable RX buffer support to vhost-net
From: Michael S. Tsirkin @ 2010-03-07 16:26 UTC (permalink / raw)
To: David Stevens; +Cc: rusty, netdev, kvm, virtualization
In-Reply-To: <OFD112C680.DB48ADA2-ON882576DB.0000CE4F-882576DB.0001E192@us.ibm.com>
On Tue, Mar 02, 2010 at 05:20:34PM -0700, David Stevens wrote:
> This patch glues them all together and makes sure we
> notify whenever we don't have enough buffers to receive
> a max-sized packet, and adds the feature bit.
>
> Signed-off-by: David L Stevens <dlstevens@us.ibm.com>
Maybe split this up?
> diff -ruN net-next-p2/drivers/vhost/net.c net-next-p3/drivers/vhost/net.c
> --- net-next-p2/drivers/vhost/net.c 2010-03-02 13:01:34.000000000
> -0800
> +++ net-next-p3/drivers/vhost/net.c 2010-03-02 15:25:15.000000000
> -0800
> @@ -54,26 +54,6 @@
> enum vhost_net_poll_state tx_poll_state;
> };
>
> -/* Pop first len bytes from iovec. Return number of segments used. */
> -static int move_iovec_hdr(struct iovec *from, struct iovec *to,
> - size_t len, int iov_count)
> -{
> - int seg = 0;
> - size_t size;
> - while (len && seg < iov_count) {
> - size = min(from->iov_len, len);
> - to->iov_base = from->iov_base;
> - to->iov_len = size;
> - from->iov_len -= size;
> - from->iov_base += size;
> - len -= size;
> - ++from;
> - ++to;
> - ++seg;
> - }
> - return seg;
> -}
> -
> /* Caller must have TX VQ lock */
> static void tx_poll_stop(struct vhost_net *net)
> {
> @@ -97,7 +77,7 @@
> static void handle_tx(struct vhost_net *net)
> {
> struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
> - unsigned out, in, s;
> + unsigned out, in;
> struct iovec head;
> struct msghdr msg = {
> .msg_name = NULL,
> @@ -110,6 +90,7 @@
> size_t len, total_len = 0;
> int err, wmem;
> struct socket *sock = rcu_dereference(vq->private_data);
> +
I tend not to add empty lines if line below it is already short.
> if (!sock)
> return;
>
> @@ -166,11 +147,11 @@
> /* Skip header. TODO: support TSO. */
> msg.msg_iovlen = out;
> head.iov_len = len = iov_length(vq->iov, out);
> +
I tend not to add empty lines if line below it is a comment.
> /* Sanity check */
> if (!len) {
> vq_err(vq, "Unexpected header len for TX: "
> - "%zd expected %zd\n",
> - len, vq->guest_hlen);
> + "%zd expected %zd\n", len, vq->guest_hlen);
> break;
> }
> /* TODO: Check specific error and bomb out unless ENOBUFS?
> */
> @@ -214,7 +195,7 @@
> static void handle_rx(struct vhost_net *net)
> {
> struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_RX];
> - unsigned in, log, s;
> + unsigned in, log;
> struct vhost_log *vq_log;
> struct msghdr msg = {
> .msg_name = NULL,
> @@ -245,30 +226,36 @@
> if (!headcount) {
> vhost_enable_notify(vq);
> break;
> - }
> + } else if (vq->maxheadcount < headcount)
> + vq->maxheadcount = headcount;
> /* Skip header. TODO: support TSO/mergeable rx buffers. */
> msg.msg_iovlen = in;
> len = iov_length(vq->iov, in);
> -
> /* Sanity check */
> if (!len) {
> vq_err(vq, "Unexpected header len for RX: "
> - "%zd expected %zd\n",
> - len, vq->guest_hlen);
> + "%zd expected %zd\n", len, vq->guest_hlen);
> break;
> }
> err = sock->ops->recvmsg(NULL, sock, &msg,
> len, MSG_DONTWAIT | MSG_TRUNC);
> - /* TODO: Check specific error and bomb out unless EAGAIN?
> */
> if (err < 0) {
> - vhost_discard(vq, 1);
> + vhost_discard(vq, headcount);
> break;
> }
> /* TODO: Should check and handle checksum. */
> + if (vhost_has_feature(&net->dev, VIRTIO_NET_F_MRG_RXBUF))
> {
> + struct virtio_net_hdr_mrg_rxbuf *vhdr =
> + (struct virtio_net_hdr_mrg_rxbuf *)
> + vq->iov[0].iov_base;
> + /* add num_bufs */
> + vq->iov[0].iov_len = vq->guest_hlen;
> + vhdr->num_buffers = headcount;
I don't understand this. iov_base is a userspace pointer, isn't it.
How can you assign values to it like that?
Rusty also commented earlier that it's not a good idea to assume
specific layout, such as first chunk being large enough to
include virtio_net_hdr_mrg_rxbuf.
I think we need to use memcpy to/from iovec etc.
> + }
> if (err > len) {
> pr_err("Discarded truncated rx packet: "
> " len %d > %zd\n", err, len);
> - vhost_discard(vq, 1);
> + vhost_discard(vq, headcount);
> continue;
> }
> len = err;
> @@ -573,8 +560,6 @@
>
> static int vhost_net_set_features(struct vhost_net *n, u64 features)
> {
> - size_t hdr_size = features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ?
> - sizeof(struct virtio_net_hdr) : 0;
> int i;
> mutex_lock(&n->dev.mutex);
> if ((features & (1 << VHOST_F_LOG_ALL)) &&
> diff -ruN net-next-p2/drivers/vhost/vhost.c
> net-next-p3/drivers/vhost/vhost.c
> --- net-next-p2/drivers/vhost/vhost.c 2010-03-02 12:53:02.000000000
> -0800
> +++ net-next-p3/drivers/vhost/vhost.c 2010-03-02 15:24:50.000000000
> -0800
> @@ -115,6 +115,7 @@
> vq->log_addr = -1ull;
> vq->guest_hlen = 0;
> vq->sock_hlen = 0;
> + vq->maxheadcount = 0;
> vq->private_data = NULL;
> vq->log_base = NULL;
> vq->error_ctx = NULL;
> @@ -410,6 +411,7 @@
> vq->last_avail_idx = s.num;
> /* Forget the cached index value. */
> vq->avail_idx = vq->last_avail_idx;
> + vq->maxheadcount = 0;
> break;
> case VHOST_GET_VRING_BASE:
> s.index = idx;
> @@ -1114,10 +1116,23 @@
> return 0;
> }
>
> +int vhost_available(struct vhost_virtqueue *vq)
> +{
> + int avail;
> +
> + if (!vq->maxheadcount) /* haven't got any yet */
> + return 1;
> + avail = vq->avail_idx - vq->last_avail_idx;
> + if (avail < 0)
> + avail += 0x10000; /* wrapped */
> + return avail;
> +}
> +
> /* This actually signals the guest, using eventfd. */
> void vhost_signal(struct vhost_dev *dev, struct vhost_virtqueue *vq)
> {
> __u16 flags = 0;
> +
I tend not to add empty lines if a line above it is already short.
> if (get_user(flags, &vq->avail->flags)) {
> vq_err(vq, "Failed to get flags");
> return;
> @@ -1125,7 +1140,7 @@
>
> /* If they don't want an interrupt, don't signal, unless empty. */
> if ((flags & VRING_AVAIL_F_NO_INTERRUPT) &&
> - (vq->avail_idx != vq->last_avail_idx ||
> + (vhost_available(vq) > vq->maxheadcount ||
I don't understand this change. It seems to make
code not match the comments.
> !vhost_has_feature(dev, VIRTIO_F_NOTIFY_ON_EMPTY)))
> return;
>
> diff -ruN net-next-p2/drivers/vhost/vhost.h
> net-next-p3/drivers/vhost/vhost.h
> --- net-next-p2/drivers/vhost/vhost.h 2010-03-02 13:02:03.000000000
> -0800
> +++ net-next-p3/drivers/vhost/vhost.h 2010-03-02 14:29:44.000000000
> -0800
> @@ -85,6 +85,7 @@
> struct iovec iov[VHOST_NET_MAX_SG+1]; /* an extra for vnet hdr */
> struct iovec heads[VHOST_NET_MAX_SG];
> size_t guest_hlen, sock_hlen;
> + int maxheadcount;
I don't completely understand what does this field does.
It seems to be only set on rx? Maybe name should reflect this?
> /* We use a kind of RCU to access private pointer.
> * All readers access it from workqueue, which makes it possible
> to
> * flush the workqueue instead of synchronize_rcu. Therefore
> readers do
> @@ -151,7 +152,8 @@
> VHOST_FEATURES = (1 << VIRTIO_F_NOTIFY_ON_EMPTY) |
> (1 << VIRTIO_RING_F_INDIRECT_DESC) |
> (1 << VHOST_F_LOG_ALL) |
> - (1 << VHOST_NET_F_VIRTIO_NET_HDR),
> + (1 << VHOST_NET_F_VIRTIO_NET_HDR) |
> + (1 << VIRTIO_NET_F_MRG_RXBUF),
> };
>
> static inline int vhost_has_feature(struct vhost_dev *dev, int bit)
>
^ permalink raw reply
* Re: [RFC][ PATCH 2/3] vhost-net: handle vnet_hdr processing for MRG_RX_BUF
From: Michael S. Tsirkin @ 2010-03-07 16:12 UTC (permalink / raw)
To: David Stevens; +Cc: rusty, netdev, kvm, virtualization
In-Reply-To: <OF9CBCD776.645508E4-ON882576DB.0000CB6E-882576DB.0001DED1@us.ibm.com>
On Tue, Mar 02, 2010 at 05:20:26PM -0700, David Stevens wrote:
> This patch adds vnet_hdr processing for mergeable buffer
> support to vhost-net.
>
> Signed-off-by: David L Stevens <dlstevens@us.ibm.com>
>
> diff -ruN net-next-p1/drivers/vhost/net.c net-next-p2/drivers/vhost/net.c
Could you please add -p to diff flags so that it's easier
to figure out which function is changes?
> --- net-next-p1/drivers/vhost/net.c 2010-03-01 11:44:22.000000000
> -0800
> +++ net-next-p2/drivers/vhost/net.c 2010-03-02 13:01:34.000000000
> -0800
> @@ -109,7 +109,6 @@
> };
> size_t len, total_len = 0;
> int err, wmem;
> - size_t hdr_size;
> struct socket *sock = rcu_dereference(vq->private_data);
> if (!sock)
> return;
> @@ -124,7 +123,6 @@
>
> if (wmem < sock->sk->sk_sndbuf * 2)
> tx_poll_stop(net);
> - hdr_size = vq->hdr_size;
>
> for (;;) {
> head.iov_base = (void *)vhost_get_vq_desc(&net->dev, vq,
> @@ -148,25 +146,45 @@
> "out %d, int %d\n", out, in);
> break;
> }
> + if (vq->guest_hlen > vq->sock_hlen) {
> + if (msg.msg_iov[0].iov_len == vq->guest_hlen)
> + msg.msg_iov[0].iov_len = vq->sock_hlen;
> + else if (out == ARRAY_SIZE(vq->iov))
> + vq_err(vq, "handle_tx iov overflow!");
> + else {
> + int i;
> +
> + /* give header its own iov */
> + for (i=out; i>0; ++i)
> + msg.msg_iov[i+1] = msg.msg_iov[i];
> + msg.msg_iov[0].iov_len = vq->sock_hlen;
> + msg.msg_iov[1].iov_base += vq->guest_hlen;
> + msg.msg_iov[1].iov_len -= vq->guest_hlen;
> + out++;
We seem to spend a fair bit of code here and elsewhere trying to cover
the diff between header size in guest and host. In hindsight, it was
not a good idea to put new padding between data and the header:
virtio-net should have added it *before*. But it is what it is.
Wouldn't it be easier to just add an ioctl to tap so that
vnet header size is made bigger by 4 bytes?
> + }
> + }
> /* Skip header. TODO: support TSO. */
> - s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, out);
> msg.msg_iovlen = out;
> head.iov_len = len = iov_length(vq->iov, out);
> /* Sanity check */
> if (!len) {
> vq_err(vq, "Unexpected header len for TX: "
> "%zd expected %zd\n",
> - iov_length(vq->hdr, s), hdr_size);
> + len, vq->guest_hlen);
> break;
> }
> /* TODO: Check specific error and bomb out unless ENOBUFS?
> */
> err = sock->ops->sendmsg(NULL, sock, &msg, len);
> if (unlikely(err < 0)) {
> - vhost_discard(vq, 1);
> - tx_poll_start(net, sock);
> + if (err == -EAGAIN) {
The comment mentions ENOBUFS. Are you sure it's EAGAIN?
> + tx_poll_start(net, sock);
Don't we need to call discard to move the last avail header back?
> + } else {
> + vq_err(vq, "sendmsg: errno %d\n", -err);
> + /* drop packet; do not discard/resend */
> + vhost_add_used_and_signal(&net->dev,vq,&head,1);
> + }
> break;
> - }
> - if (err != len)
> + } else if (err != len)
Previous if ends with break so no need for else here.
> pr_err("Truncated TX packet: "
> " len %d != %zd\n", err, len);
> vhost_add_used_and_signal(&net->dev, vq, &head, 1);
> @@ -207,14 +225,8 @@
> .msg_flags = MSG_DONTWAIT,
> };
>
> - struct virtio_net_hdr hdr = {
> - .flags = 0,
> - .gso_type = VIRTIO_NET_HDR_GSO_NONE
> - };
> -
> size_t len, total_len = 0;
> int err, headcount, datalen;
> - size_t hdr_size;
> struct socket *sock = rcu_dereference(vq->private_data);
>
> if (!sock || !skb_head_len(&sock->sk->sk_receive_queue))
> @@ -223,7 +235,6 @@
> use_mm(net->dev.mm);
> mutex_lock(&vq->mutex);
> vhost_disable_notify(vq);
> - hdr_size = vq->hdr_size;
>
> vq_log = unlikely(vhost_has_feature(&net->dev, VHOST_F_LOG_ALL)) ?
> vq->log : NULL;
> @@ -232,25 +243,18 @@
> headcount = vhost_get_heads(vq, datalen, &in, vq_log,
> &log);
> /* OK, now we need to know about added descriptors. */
> if (!headcount) {
> - if (unlikely(vhost_enable_notify(vq))) {
> - /* They have slipped one in as we were
> - * doing that: check again. */
> - vhost_disable_notify(vq);
> - continue;
> - }
> - /* Nothing new? Wait for eventfd to tell us
> - * they refilled. */
> + vhost_enable_notify(vq);
You don't think this race can happen?
> break;
> }
> /* Skip header. TODO: support TSO/mergeable rx buffers. */
> - s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, in);
> msg.msg_iovlen = in;
> len = iov_length(vq->iov, in);
> +
> /* Sanity check */
> if (!len) {
> vq_err(vq, "Unexpected header len for RX: "
> "%zd expected %zd\n",
> - iov_length(vq->hdr, s), hdr_size);
> + len, vq->guest_hlen);
> break;
> }
> err = sock->ops->recvmsg(NULL, sock, &msg,
> @@ -268,13 +272,7 @@
> continue;
> }
> len = err;
> - err = memcpy_toiovec(vq->hdr, (unsigned char *)&hdr,
> hdr_size);
> - if (err) {
> - vq_err(vq, "Unable to write vnet_hdr at addr %p:
> %d\n",
> - vq->iov->iov_base, err);
> - break;
> - }
> - len += hdr_size;
> + len += vq->guest_hlen - vq->sock_hlen;
> vhost_add_used_and_signal(&net->dev, vq, vq->heads,
> headcount);
> if (unlikely(vq_log))
> vhost_log_write(vq, vq_log, log, len);
> @@ -483,6 +481,13 @@
> return ERR_PTR(-ENOTSOCK);
> }
>
> +static int vhost_sock_is_raw(struct socket *sock)
> +{
> + if (!sock || !sock->sk)
> + return 0;
> + return sock->sk->sk_type == SOCK_RAW;
> +}
> +
> static long vhost_net_set_backend(struct vhost_net *n, unsigned index,
> int fd)
> {
> struct socket *sock, *oldsock;
> @@ -519,6 +524,20 @@
>
> vhost_net_disable_vq(n, vq);
> rcu_assign_pointer(vq->private_data, sock);
> +
> + if (sock && sock->sk) {
> + if (!vhost_sock_is_raw(sock) ||
I dislike this backend-specific code, it ties us with specifics of
backend implementations, which change without notice. Ideally all
backend-specific stuff should live in userspace, userspace programs
vhost device appropriately.
> + vhost_has_feature(&n->dev,
> VHOST_NET_F_VIRTIO_NET_HDR)) {
> + vq->sock_hlen = sizeof(struct virtio_net_hdr);
> + if (vhost_has_feature(&n->dev,
> VIRTIO_NET_F_MRG_RXBUF))
> + vq->guest_hlen =
> + sizeof(struct
> virtio_net_hdr_mrg_rxbuf);
> + else
> + vq->guest_hlen = sizeof(struct
> virtio_net_hdr);
> + } else
> + vq->guest_hlen = vq->sock_hlen = 0;
> + } else
> + vq_err(vq, "vhost_net_set_backend: sock->sk is NULL");
As proposed above, IMO it would be nicer to add an ioctl in tap
that let us program header size.
> vhost_net_enable_vq(n, vq);
> mutex_unlock(&vq->mutex);
> done:
> @@ -566,8 +585,17 @@
> n->dev.acked_features = features;
> smp_wmb();
> for (i = 0; i < VHOST_NET_VQ_MAX; ++i) {
> - mutex_lock(&n->vqs[i].mutex);
> - n->vqs[i].hdr_size = hdr_size;
> + struct vhost_virtqueue *vq = n->vqs + i;
> + struct socket *sock = vq->private_data;
> +
> + mutex_lock(&vq->mutex);
> + if (features & (1 << VIRTIO_NET_F_MRG_RXBUF))
> + vq->sock_hlen = sizeof(struct
> virtio_net_hdr_mrg_rxbuf);
> + else if (features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ||
> + !vhost_sock_is_raw(sock))
> + vq->sock_hlen = sizeof(struct virtio_net_hdr);
> + else
> + vq->sock_hlen = 0;
> mutex_unlock(&n->vqs[i].mutex);
> }
> vhost_net_flush(n);
> diff -ruN net-next-p1/drivers/vhost/vhost.c
> net-next-p2/drivers/vhost/vhost.c
> --- net-next-p1/drivers/vhost/vhost.c 2010-03-01 11:44:06.000000000
> -0800
> +++ net-next-p2/drivers/vhost/vhost.c 2010-03-02 12:53:02.000000000
> -0800
> @@ -113,7 +113,8 @@
> vq->used_flags = 0;
> vq->log_used = false;
> vq->log_addr = -1ull;
> - vq->hdr_size = 0;
> + vq->guest_hlen = 0;
> + vq->sock_hlen = 0;
> vq->private_data = NULL;
> vq->log_base = NULL;
> vq->error_ctx = NULL;
> @@ -848,20 +849,85 @@
> return 0;
> }
>
> +static int
> +vhost_get_hdr(struct vhost_virtqueue *vq, int *in, struct vhost_log *log,
> + int *log_num)
> +{
> + struct iovec *heads = vq->heads;
> + struct iovec *iov = vq->iov;
> + int out;
> +
> + *in = 0;
> + iov[0].iov_len = 0;
> +
> + /* get buffer, starting from iov[1] */
> + heads[0].iov_base = (void *)vhost_get_vq_desc(vq->dev, vq,
> + vq->iov+1, ARRAY_SIZE(vq->iov)-1, &out, in, log, log_num);
> + if (out || *in <= 0) {
> + vq_err(vq, "unexpected descriptor format for RX: out %d, "
> + "in %d\n", out, *in);
> + return 0;
> + }
> + if (heads[0].iov_base == (void *)vq->num)
> + return 0;
> +
> + /* make iov[0] the header */
> + if (!vq->guest_hlen) {
> + if (vq->sock_hlen) {
> + static struct virtio_net_hdr junk; /* bit bucket
> */
> +
> + iov[0].iov_base = &junk;
> + iov[0].iov_len = sizeof(junk);
> + } else
> + iov[0].iov_len = 0;
> + }
> + if (vq->sock_hlen < vq->guest_hlen) {
> + iov[0].iov_base = iov[1].iov_base;
> + iov[0].iov_len = vq->sock_hlen;
> +
> + if (iov[1].iov_len < vq->sock_hlen) {
> + vq_err(vq, "can't fit header in one buffer!");
> + vhost_discard(vq, 1);
> + return 0;
> + }
> + if (!vq->sock_hlen) {
> + static const struct virtio_net_hdr_mrg_rxbuf hdr =
> {
> + .hdr.flags = 0,
> + .hdr.gso_type = VIRTIO_NET_HDR_GSO_NONE
> + };
> + memcpy(iov[0].iov_base, &hdr, vq->guest_hlen);
> + }
> + iov[1].iov_base += vq->guest_hlen;
> + iov[1].iov_len -= vq->guest_hlen;
> + }
> + return 1;
The above looks kind of scary, lots of special-casing. I'll send a
patch for tap to set vnet header size, let's see if it makes life easier
for us.
> +}
> +
> unsigned vhost_get_heads(struct vhost_virtqueue *vq, int datalen, int
> *iovcount,
> struct vhost_log *log, unsigned int *log_num)
> {
> struct iovec *heads = vq->heads;
> - int out, in;
> + int out, in = 0;
> + int seg = 0;
> int hc = 0;
I think it's better to stick to simple names like i
for index variables, alternatively give it a meaningful
name like "count".
>
> + if (vq->guest_hlen != vq->sock_hlen) {
Sticking guest_hlen/sock_hlen in vhost is somewhat ugly.
> + seg = vhost_get_hdr(vq, &in, log, log_num);
> + if (!seg)
> + return 0;
> + hc++;
> + datalen -= iov_length(vq->iov+seg, in);
> + seg += in;
> + }
> +
> while (datalen > 0) {
> if (hc >= VHOST_NET_MAX_SG) {
> vhost_discard(vq, hc);
> return 0;
> }
> heads[hc].iov_base = (void *)vhost_get_vq_desc(vq->dev,
> vq,
> - vq->iov, ARRAY_SIZE(vq->iov), &out, &in, log,
> log_num);
> + vq->iov+seg, ARRAY_SIZE(vq->iov)-seg, &out, &in,
> + log, log_num);
> if (heads[hc].iov_base == (void *)vq->num) {
> vhost_discard(vq, hc);
> return 0;
> @@ -872,11 +938,12 @@
> vhost_discard(vq, hc);
> return 0;
> }
> - heads[hc].iov_len = iov_length(vq->iov, in);
> - hc++;
> + heads[hc].iov_len = iov_length(vq->iov+seg, in);
> datalen -= heads[hc].iov_len;
> + hc++;
> + seg += in;
> }
> - *iovcount = in;
> + *iovcount = seg;
> return hc;
> }
>
> diff -ruN net-next-p1/drivers/vhost/vhost.h
> net-next-p2/drivers/vhost/vhost.h
> --- net-next-p1/drivers/vhost/vhost.h 2010-03-01 11:42:18.000000000
> -0800
> +++ net-next-p2/drivers/vhost/vhost.h 2010-03-02 13:02:03.000000000
> -0800
> @@ -82,10 +82,9 @@
> u64 log_addr;
>
> struct iovec indirect[VHOST_NET_MAX_SG];
> - struct iovec iov[VHOST_NET_MAX_SG];
> - struct iovec hdr[VHOST_NET_MAX_SG];
> + struct iovec iov[VHOST_NET_MAX_SG+1]; /* an extra for vnet hdr */
> struct iovec heads[VHOST_NET_MAX_SG];
> - size_t hdr_size;
> + size_t guest_hlen, sock_hlen;
> /* We use a kind of RCU to access private pointer.
> * All readers access it from workqueue, which makes it possible
> to
> * flush the workqueue instead of synchronize_rcu. Therefore
> readers do
>
^ permalink raw reply
* Re: 2.6.33 dies on modprobe
From: Américo Wang @ 2010-03-07 15:41 UTC (permalink / raw)
To: M G Berberich
Cc: Américo Wang, linux-kernel, Linux Kernel Network Developers,
Greg Kroah-Hartman
In-Reply-To: <20100304203803.GA3267@invalid>
On Thu, Mar 04, 2010 at 09:38:03PM +0100, M G Berberich wrote:
>Am Donnerstag, den 04. März schrieb Américo Wang:
>> On Thu, Mar 04, 2010 at 12:04:38AM +0100, M G Berberich wrote:
>> >Am Mittwoch, den 03. März schrieb Américo Wang:
>> >> On Wed, Mar 3, 2010 at 5:08 PM, Américo Wang <xiyou.wangcong@gmail.com> wrote:
>> >> > On Wed, Mar 3, 2010 at 4:58 PM, Américo Wang <xiyou.wangcong@gmail.com> wrote:
>> >> >> Ok, below is my patch, I am not sure it could fix the BUG for you,
>> >> >> but it could fix the WARNING. But perhaps they are related.
>> >> >>
>> >> >> Please give it a try.
>> >> >>
>> >> >
>> >> > Oops! Ignore the patch, it should not be correct, I will send a
>> >> > correct version soon.
>> >> > Sorry.
>> >> >
>> >>
>> >> Here we go:
>> >
>> >Seems to fix the warning, but not the modprobe-Oops.
>> >
>>
>> Thanks much for your testing!
>>
>> Could you please try to turn on DEBUG_KOBJECT and boot your kernel
>> with "initcall_debug"? And then show us your dmesg.
>
>If I'm doing this (enabled CONFIG_DEBUG_KERNEL and
>CONFIG_DEBUG_KOBJECT), the kernel dies immediately - no output.
>
Ouch!
CONFIG_DEBUG_KOBJECT will only enable some printk's, so I think
probably it's CONFIG_DEBUG_KERNEL which breaks your booting.
Could you please provide your .config?
Thanks!
^ permalink raw reply
* Re: [RFC][ PATCH 1/3] vhost-net: support multiple buffer heads in receiver
From: Michael S. Tsirkin @ 2010-03-07 15:31 UTC (permalink / raw)
To: David Stevens; +Cc: rusty, netdev, kvm, virtualization
In-Reply-To: <OF201C1B9E.C89ADF9F-ON882576DB.00006AD7-882576DB.0001DA4E@us.ibm.com>
On Tue, Mar 02, 2010 at 05:20:15PM -0700, David Stevens wrote:
> This patch generalizes buffer handling functions to
> support multiple buffer heads.
>
> In-line for viewing, attached for applying.
> Signed-off-by: David L Stevens <dlstevens@us.ibm.com>
> diff -ruN net-next-p0/drivers/vhost/net.c net-next-p1/drivers/vhost/net.c
> --- net-next-p0/drivers/vhost/net.c 2010-02-24 12:59:24.000000000
> -0800
> +++ net-next-p1/drivers/vhost/net.c 2010-03-01 11:44:22.000000000
> -0800
> @@ -97,7 +97,8 @@
> static void handle_tx(struct vhost_net *net)
> {
> struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
> - unsigned head, out, in, s;
> + unsigned out, in, s;
> + struct iovec head;
> struct msghdr msg = {
> .msg_name = NULL,
> .msg_namelen = 0,
> @@ -126,12 +127,10 @@
> hdr_size = vq->hdr_size;
>
> for (;;) {
> - head = vhost_get_vq_desc(&net->dev, vq, vq->iov,
> - ARRAY_SIZE(vq->iov),
> - &out, &in,
> - NULL, NULL);
> + head.iov_base = (void *)vhost_get_vq_desc(&net->dev, vq,
> + vq->iov, ARRAY_SIZE(vq->iov), &out, &in, NULL,
> NULL);
Should type for head be changed so that we do not need to cast?
I also prefer aligning descendants on the opening (.
> /* Nothing new? Wait for eventfd to tell us they
> refilled. */
> - if (head == vq->num) {
> + if (head.iov_base == (void *)vq->num) {
> wmem = atomic_read(&sock->sk->sk_wmem_alloc);
> if (wmem >= sock->sk->sk_sndbuf * 3 / 4) {
> tx_poll_start(net, sock);
> @@ -152,7 +151,7 @@
> /* Skip header. TODO: support TSO. */
> s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, out);
> msg.msg_iovlen = out;
> - len = iov_length(vq->iov, out);
> + head.iov_len = len = iov_length(vq->iov, out);
> /* Sanity check */
> if (!len) {
> vq_err(vq, "Unexpected header len for TX: "
> @@ -163,14 +162,14 @@
> /* TODO: Check specific error and bomb out unless ENOBUFS?
> */
> err = sock->ops->sendmsg(NULL, sock, &msg, len);
> if (unlikely(err < 0)) {
> - vhost_discard_vq_desc(vq);
> + vhost_discard(vq, 1);
Isn't the original name a bit more descriptive?
> tx_poll_start(net, sock);
> break;
> }
> if (err != len)
> pr_err("Truncated TX packet: "
> " len %d != %zd\n", err, len);
> - vhost_add_used_and_signal(&net->dev, vq, head, 0);
> + vhost_add_used_and_signal(&net->dev, vq, &head, 1);
> total_len += len;
> if (unlikely(total_len >= VHOST_NET_WEIGHT)) {
> vhost_poll_queue(&vq->poll);
> @@ -182,12 +181,22 @@
> unuse_mm(net->dev.mm);
> }
>
> +static int skb_head_len(struct sk_buff_head *skq)
> +{
> + struct sk_buff *head;
> +
> + head = skb_peek(skq);
> + if (head)
> + return head->len;
> + return 0;
> +}
> +
This is done without locking, which I think can crash
if skb is consumed after we peek it but before we read the
length.
> /* Expects to be always run from workqueue - which acts as
> * read-size critical section for our kind of RCU. */
> static void handle_rx(struct vhost_net *net)
> {
> struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_RX];
> - unsigned head, out, in, log, s;
> + unsigned in, log, s;
> struct vhost_log *vq_log;
> struct msghdr msg = {
> .msg_name = NULL,
> @@ -204,10 +213,11 @@
> };
>
> size_t len, total_len = 0;
> - int err;
> + int err, headcount, datalen;
> size_t hdr_size;
> struct socket *sock = rcu_dereference(vq->private_data);
> - if (!sock || skb_queue_empty(&sock->sk->sk_receive_queue))
> +
> + if (!sock || !skb_head_len(&sock->sk->sk_receive_queue))
> return;
>
Isn't this equivalent? Do you expect zero len skbs in socket?
If yes, maybe we should discard these, not stop processing ...
> use_mm(net->dev.mm);
> @@ -218,13 +228,10 @@
> vq_log = unlikely(vhost_has_feature(&net->dev, VHOST_F_LOG_ALL)) ?
> vq->log : NULL;
>
> - for (;;) {
> - head = vhost_get_vq_desc(&net->dev, vq, vq->iov,
> - ARRAY_SIZE(vq->iov),
> - &out, &in,
> - vq_log, &log);
> + while ((datalen = skb_head_len(&sock->sk->sk_receive_queue))) {
This peeks in the queue to figure out how much data we have.
It's a neat trick, but it does introduce new failure modes
where an skb is consumed after we did the peek:
- new skb could be shorter, we should return the unconsumed part
- new skb could be longer, this will drop a packet.
maybe this last is not an issue as the race should be rare in practice
> + headcount = vhost_get_heads(vq, datalen, &in, vq_log,
> &log);
> /* OK, now we need to know about added descriptors. */
> - if (head == vq->num) {
> + if (!headcount) {
> if (unlikely(vhost_enable_notify(vq))) {
> /* They have slipped one in as we were
> * doing that: check again. */
> @@ -235,13 +242,6 @@
> * they refilled. */
> break;
> }
> - /* We don't need to be notified again. */
you find this comment unhelpful?
> - if (out) {
> - vq_err(vq, "Unexpected descriptor format for RX: "
> - "out %d, int %d\n",
> - out, in);
> - break;
> - }
we still need this check, don't we?
> /* Skip header. TODO: support TSO/mergeable rx buffers. */
> s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, in);
> msg.msg_iovlen = in;
> @@ -257,14 +257,14 @@
> len, MSG_DONTWAIT | MSG_TRUNC);
> /* TODO: Check specific error and bomb out unless EAGAIN?
> */
> if (err < 0) {
> - vhost_discard_vq_desc(vq);
> + vhost_discard(vq, 1);
> break;
> }
> /* TODO: Should check and handle checksum. */
> if (err > len) {
> pr_err("Discarded truncated rx packet: "
> " len %d > %zd\n", err, len);
> - vhost_discard_vq_desc(vq);
> + vhost_discard(vq, 1);
> continue;
> }
> len = err;
> @@ -275,7 +275,7 @@
> break;
> }
> len += hdr_size;
> - vhost_add_used_and_signal(&net->dev, vq, head, len);
> + vhost_add_used_and_signal(&net->dev, vq, vq->heads,
> headcount);
> if (unlikely(vq_log))
> vhost_log_write(vq, vq_log, log, len);
> total_len += len;
> diff -ruN net-next-p0/drivers/vhost/vhost.c
> net-next-p1/drivers/vhost/vhost.c
> --- net-next-p0/drivers/vhost/vhost.c 2010-02-15 20:08:35.000000000
> -0800
> +++ net-next-p1/drivers/vhost/vhost.c 2010-03-01 11:44:06.000000000
> -0800
> @@ -848,6 +848,38 @@
> return 0;
> }
>
> +unsigned vhost_get_heads(struct vhost_virtqueue *vq, int datalen, int
> *iovcount,
> + struct vhost_log *log, unsigned int *log_num)
Could you please document this function's parameters? It's not obvious
what iovcount, log, log_num are.
> +{
> + struct iovec *heads = vq->heads;
> + int out, in;
> + int hc = 0;
> +
> + while (datalen > 0) {
> + if (hc >= VHOST_NET_MAX_SG) {
> + vhost_discard(vq, hc);
> + return 0;
> + }
> + heads[hc].iov_base = (void *)vhost_get_vq_desc(vq->dev,
> vq,
> + vq->iov, ARRAY_SIZE(vq->iov), &out, &in, log,
> log_num);
> + if (heads[hc].iov_base == (void *)vq->num) {
> + vhost_discard(vq, hc);
> + return 0;
> + }
> + if (out || in <= 0) {
> + vq_err(vq, "unexpected descriptor format for RX: "
> + "out %d, in %d\n", out, in);
> + vhost_discard(vq, hc);
> + return 0;
> + }
> + heads[hc].iov_len = iov_length(vq->iov, in);
> + hc++;
> + datalen -= heads[hc].iov_len;
> + }
> + *iovcount = in;
Only the last value?
> + return hc;
> +}
> +
> /* This looks in the virtqueue and for the first available buffer, and
> converts
> * it to an iovec for convenient access. Since descriptors consist of
> some
> * number of output then some number of input descriptors, it's actually
> two
> @@ -973,31 +1005,32 @@
> }
>
> /* Reverse the effect of vhost_get_vq_desc. Useful for error handling. */
> -void vhost_discard_vq_desc(struct vhost_virtqueue *vq)
> +void vhost_discard(struct vhost_virtqueue *vq, int n)
> {
> - vq->last_avail_idx--;
> + vq->last_avail_idx -= n;
> }
>
> /* After we've used one of their buffers, we tell them about it. We'll
> then
> * want to notify the guest, using eventfd. */
> -int vhost_add_used(struct vhost_virtqueue *vq, unsigned int head, int
> len)
> +int vhost_add_used(struct vhost_virtqueue *vq, struct iovec *heads, int
> count)
> {
> struct vring_used_elem *used;
> + int i;
>
> - /* The virtqueue contains a ring of used buffers. Get a pointer
> to the
> - * next entry in that used ring. */
> - used = &vq->used->ring[vq->last_used_idx % vq->num];
> - if (put_user(head, &used->id)) {
> - vq_err(vq, "Failed to write used id");
> - return -EFAULT;
> - }
> - if (put_user(len, &used->len)) {
> - vq_err(vq, "Failed to write used len");
> - return -EFAULT;
> + for (i=0; i<count; i++, vq->last_used_idx++) {
whitespace damage: I prefer space around =, <.
I also use ++i, etc in this driver, better be consistent?
Also for clarity, I prefer to put vq->last_used_idx inside the loop.
> + used = &vq->used->ring[vq->last_used_idx % vq->num];
> + if (put_user((unsigned)heads[i].iov_base, &used->id)) {
> + vq_err(vq, "Failed to write used id");
> + return -EFAULT;
> + }
> + if (put_user(heads[i].iov_len, &used->len)) {
> + vq_err(vq, "Failed to write used len");
> + return -EFAULT;
> + }
If this fails, last_used_idx will still be incremented, which I think is wrong.
> }
> /* Make sure buffer is written before we update index. */
> smp_wmb();
> - if (put_user(vq->last_used_idx + 1, &vq->used->idx)) {
> + if (put_user(vq->last_used_idx, &vq->used->idx)) {
here, too
> vq_err(vq, "Failed to increment used idx");
> return -EFAULT;
> }
> @@ -1011,7 +1044,6 @@
> if (vq->log_ctx)
> eventfd_signal(vq->log_ctx, 1);
> }
> - vq->last_used_idx++;
> return 0;
> }
>
> @@ -1038,9 +1070,9 @@
> /* And here's the combo meal deal. Supersize me! */
> void vhost_add_used_and_signal(struct vhost_dev *dev,
> struct vhost_virtqueue *vq,
> - unsigned int head, int len)
> + struct iovec *heads, int count)
> {
> - vhost_add_used(vq, head, len);
> + vhost_add_used(vq, heads, count);
> vhost_signal(dev, vq);
> }
>
> diff -ruN net-next-p0/drivers/vhost/vhost.h
> net-next-p1/drivers/vhost/vhost.h
> --- net-next-p0/drivers/vhost/vhost.h 2010-02-15 20:08:35.000000000
> -0800
> +++ net-next-p1/drivers/vhost/vhost.h 2010-03-01 11:42:18.000000000
> -0800
> @@ -84,6 +84,7 @@
> struct iovec indirect[VHOST_NET_MAX_SG];
> struct iovec iov[VHOST_NET_MAX_SG];
> struct iovec hdr[VHOST_NET_MAX_SG];
> + struct iovec heads[VHOST_NET_MAX_SG];
> size_t hdr_size;
> /* We use a kind of RCU to access private pointer.
> * All readers access it from workqueue, which makes it possible
> to
> @@ -120,16 +121,18 @@
> int vhost_vq_access_ok(struct vhost_virtqueue *vq);
> int vhost_log_access_ok(struct vhost_dev *);
>
> +unsigned vhost_get_heads(struct vhost_virtqueue *, int datalen, int
> *iovcount,
> + struct vhost_log *log, unsigned int *log_num);
> unsigned vhost_get_vq_desc(struct vhost_dev *, struct vhost_virtqueue *,
> struct iovec iov[], unsigned int iov_count,
> unsigned int *out_num, unsigned int *in_num,
> struct vhost_log *log, unsigned int *log_num);
> -void vhost_discard_vq_desc(struct vhost_virtqueue *);
> +void vhost_discard(struct vhost_virtqueue *, int);
>
> -int vhost_add_used(struct vhost_virtqueue *, unsigned int head, int len);
> -void vhost_signal(struct vhost_dev *, struct vhost_virtqueue *);
> +int vhost_add_used(struct vhost_virtqueue *, struct iovec *heads, int
> count);
> void vhost_add_used_and_signal(struct vhost_dev *, struct vhost_virtqueue
> *,
> - unsigned int head, int len);
> + struct iovec *heads, int count);
> +void vhost_signal(struct vhost_dev *, struct vhost_virtqueue *);
> void vhost_disable_notify(struct vhost_virtqueue *);
> bool vhost_enable_notify(struct vhost_virtqueue *);
>
^ permalink raw reply
* [patch] irda-usb: add error handling and fix leak
From: Dan Carpenter @ 2010-03-07 12:35 UTC (permalink / raw)
To: Samuel Ortiz
Cc: David S. Miller, Stephen Hemminger, Patrick McHardy, Tadashi Abe,
Julia Lawall, netdev, kernel-janitors
If the call to kcalloc() fails then we should return -ENOMEM.
Signed-off-by: Dan Carpenter <error27@gmail.com>
diff --git a/drivers/net/irda/irda-usb.c b/drivers/net/irda/irda-usb.c
index e8e33bb..2c9b3af 100644
--- a/drivers/net/irda/irda-usb.c
+++ b/drivers/net/irda/irda-usb.c
@@ -1651,6 +1651,8 @@ static int irda_usb_probe(struct usb_interface *intf,
self->rx_urb = kcalloc(self->max_rx_urb, sizeof(struct urb *),
GFP_KERNEL);
+ if (!self->rx_urb)
+ goto err_free_net;
for (i = 0; i < self->max_rx_urb; i++) {
self->rx_urb[i] = usb_alloc_urb(0, GFP_KERNEL);
@@ -1783,6 +1785,8 @@ err_out_2:
err_out_1:
for (i = 0; i < self->max_rx_urb; i++)
usb_free_urb(self->rx_urb[i]);
+ kfree(self->rx_urb);
+err_free_net:
free_netdev(net);
err_out:
return ret;
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox