* 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 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
* [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
* [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
* 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
* 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 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
* [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 1/3] vhost-net: support multiple buffer heads in receiver
From: Michael S. Tsirkin @ 2010-03-08 7:45 UTC (permalink / raw)
To: David Stevens; +Cc: kvm, netdev, rusty, virtualization
In-Reply-To: <OFEEE8B4AB.0C708EB8-ON882576DF.00831519-882576E0.000329C4@us.ibm.com>
On Sun, Mar 07, 2010 at 04:34:32PM -0800, David Stevens wrote:
> "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
Yes, we stop on error, but host can fix uo the vq and redo a kick.
That's why there's errorfd.
--
MST
^ permalink raw reply
* Re: [RFC][ PATCH 2/3] vhost-net: handle vnet_hdr processing for MRG_RX_BUF
From: Michael S. Tsirkin @ 2010-03-08 7:54 UTC (permalink / raw)
To: David Stevens; +Cc: kvm, netdev, rusty, virtualization
In-Reply-To: <OF8A4807F2.DE42C086-ON882576E0.00033A96-882576E0.00080FCA@us.ibm.com>
On Sun, Mar 07, 2010 at 05:28:02PM -0800, David Stevens wrote:
> "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).
You mean already full? Well, this one triggers even if queue
on guest side is not full. TCP might be ok with it, but I suspect
packet drops would be very bad for UDP.
> 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. :-)
>
Hmm, need to think about it as well.
> > > @@ -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.
qemu tells us how much of the header to skip before passing
it to socket.
> > > + 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?
I'l post a patch, and we'll see.
> > > +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.
Let's see about tap patch then.
> >
> > > +}
> > > +
> > > 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?
I mean hc. Let's call it count and live with line wraps if you don't
like i.
> >
> > >
> > > + 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.
Yes. hdrsize is kind of generic, socket is obviously net specific ...
> 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
--
MST
^ permalink raw reply
* Re: [RFC][ PATCH 3/3] vhost-net: Add mergeable RX buffer support to vhost-net
From: Michael S. Tsirkin @ 2010-03-08 8:07 UTC (permalink / raw)
To: David Stevens; +Cc: kvm, netdev, rusty, virtualization
In-Reply-To: <OF8D4A9381.FF5A483F-ON882576E0.0008221D-882576E0.000B9D4A@us.ibm.com>
On Sun, Mar 07, 2010 at 06:06:51PM -0800, David Stevens wrote:
> "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,
I'm not sure why. Can't we just call memcpy_from_iovec
and then read the structure as usual?
> 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 don't really see how you can find this out from just the number of
heads. A head can address buffer of any size. Need to think
about this code some more.
> I can think about changing the comment to reflect this more
> clearly.
Also, this is all for rx only, so names should reflect this I think.
> >
> > > !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
Hmm, yes. One of the horrors of the mergeable buffer hack.
Not sure all this is worth the complexity though: I don't think this
covers all cases whether the size would be < 64K, anyway: you don't get
notified when user disables GRO on physical NIC, for example. So maybe
just go ahead with hard-coding 64K.
In any case, number of heads does not necessarily tell us much either,
does it?
Would interrupt when we actually don't have room on RX work better? I'll
think about it some more.
--
MST
^ permalink raw reply
* Re: [tcpdump-workers] Current wireless-testing breaks libpcap: mr_alen should be set
From: Jiri Pirko @ 2010-03-08 8:11 UTC (permalink / raw)
To: Guy Harris; +Cc: tcpdump-workers, linux-wireless, netdev
In-Reply-To: <7D621661-59E7-46C4-9E86-1D9654A626E7@alum.mit.edu>
Sat, Mar 06, 2010 at 10:23:12PM CET, guy@alum.mit.edu wrote:
>
>On Mar 2, 2010, at 5:00 PM, Pavel Roskin wrote:
>
>> This patch to libpcap helps:
>>
>> --- a/pcap-linux.c
>> +++ b/pcap-linux.c
>> @@ -1563,6 +1563,7 @@ live_open_new(pcap_t *handle, const char
>> memset(&mr, 0, sizeof(mr));
>> mr.mr_ifindex = handle->md.ifindex;
>> mr.mr_type = PACKET_MR_PROMISC;
>> + mr.mr_alen = 6;
>
>If there are any network types that support promiscuous mode and have link-layer addresses that aren't 6 octets long, that would still fail.
>
>It sounds as if the fix is not to care about the address length if the address isn't used, so you don't need to get the length right for PACKET_MR_PROMISC or PACKET_MR_ALLMULTI, so libpcap, and other clients setting promiscuous or "show me all multicast packets" mode, don't need to change. Is that the case?
This should be fixed in kernel (net-2.6
1162563f82b434e3099c9e6c1bbdba846d792f0d)
Jirka
^ permalink raw reply
* Re: [PATCH net-2.6] netfilter: ebt_ip6: Use ipv6_masked_addr_cmp().
From: Bart De Schuymer @ 2010-03-08 8:25 UTC (permalink / raw)
To: YOSHIFUJI Hideaki
Cc: bart.de.schuymer, kaber, davem, ebtables-devel, netfilter-devel,
netdev
In-Reply-To: <201003070918.o279IXtq029570@94.43.138.210.xn.2iij.net>
YOSHIFUJI Hideaki schreef:
> Signed-off-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
> ---
> net/bridge/netfilter/ebt_ip6.c | 18 ++++--------------
> 1 files changed, 4 insertions(+), 14 deletions(-)
>
> diff --git a/net/bridge/netfilter/ebt_ip6.c b/net/bridge/netfilter/ebt_ip6.c
> index bbf2534..4644cc9 100644
> --- a/net/bridge/netfilter/ebt_ip6.c
> +++ b/net/bridge/netfilter/ebt_ip6.c
> @@ -35,8 +35,6 @@ ebt_ip6_mt(const struct sk_buff *skb, const struct xt_match_param *par)
> struct ipv6hdr _ip6h;
> const struct tcpudphdr *pptr;
> struct tcpudphdr _ports;
> - struct in6_addr tmp_addr;
> - int i;
>
> ih6 = skb_header_pointer(skb, 0, sizeof(_ip6h), &_ip6h);
> if (ih6 == NULL)
> @@ -44,18 +42,10 @@ ebt_ip6_mt(const struct sk_buff *skb, const struct xt_match_param *par)
> if (info->bitmask & EBT_IP6_TCLASS &&
> FWINV(info->tclass != ipv6_get_dsfield(ih6), EBT_IP6_TCLASS))
> return false;
> - for (i = 0; i < 4; i++)
> - tmp_addr.in6_u.u6_addr32[i] = ih6->saddr.in6_u.u6_addr32[i] &
> - info->smsk.in6_u.u6_addr32[i];
> - if (info->bitmask & EBT_IP6_SOURCE &&
> - FWINV((ipv6_addr_cmp(&tmp_addr, &info->saddr) != 0),
> - EBT_IP6_SOURCE))
> - return false;
> - for (i = 0; i < 4; i++)
> - tmp_addr.in6_u.u6_addr32[i] = ih6->daddr.in6_u.u6_addr32[i] &
> - info->dmsk.in6_u.u6_addr32[i];
> - if (info->bitmask & EBT_IP6_DEST &&
> - FWINV((ipv6_addr_cmp(&tmp_addr, &info->daddr) != 0), EBT_IP6_DEST))
> + if (FWINV(ipv6_masked_addr_cmp(&ih6->saddr, &info->smsk,
> + &info->saddr), EBT_IP6_SOURCE) ||
> + FWINV(ipv6_masked_addr_cmp(&ih6->daddr, &info->dmsk,
> + &info->daddr), EBT_IP6_DEST))
> return false;
> if (info->bitmask & EBT_IP6_PROTO) {
> uint8_t nexthdr = ih6->nexthdr;
>
Signed-off-by: Bart De Schuymer <bdschuym@pandora.be>
Looks OK to me.
cheers,
Bart
--
Bart De Schuymer
www.artinalgorithms.be
^ permalink raw reply
* Re: [RFC][PATCH] ns: Syscalls for better namespace sharing control.
From: Eric W. Biederman @ 2010-03-08 8:32 UTC (permalink / raw)
To: Daniel Lezcano
Cc: Pavel Emelyanov, Sukadev Bhattiprolu, Serge Hallyn,
Linux Netdev List, containers, Netfilter Development Mailinglist,
Ben Greear
In-Reply-To: <4B92C886.9020507@free.fr>
I have take an snapshot of my development tree and placed it at.
git://git.kernel.org/pub/scm/linux/people/ebiederm/linux-2.6.33-nsfd-v5.git
>> I am going to explore a bit more. Given that nsfd is using the same
>> permission checks as a proc file, I think I can just make it a proc
>> file. Something like "/proc/<pid>/ns/net". With a little luck that
>> won't suck too badly.
>>
> Ah ! yes. Good idea.
It is a hair more code to use proc files but nothing worth counting.
Probably the biggest thing I am aware of right now in my development
tree is in getting uids to pass properly between unix domain sockets
I would up writing this cred_to_ucred function.
Serge can you take a look and check my logic, and do you have
any idea of where we should place something like pid_vnr but
for the uid namespace?
void cred_to_ucred(struct pid *pid, const struct cred *cred,
struct ucred *ucred)
{
ucred->pid = pid_vnr(pid);
ucred->uid = ucred->gid = -1;
if (cred) {
struct user_namespace *cred_ns = cred->user->user_ns;
struct user_namespace *current_ns = current_user_ns();
struct user_namespace *tmp;
if (likely(cred_ns == current_ns)) {
ucred->uid = cred->euid;
ucred->gid = cred->egid;
} else {
/* Is cred in a child user namespace */
tmp = cred_ns;
do {
tmp = tmp->creator->user_ns;
if (tmp == current_ns) {
ucred->uid = tmp->creator->uid;
ucred->gid = overflowgid;
return;
}
} while (tmp != &init_user_ns);
/* Is cred the creator of my user namespace,
* or the creator of one of it's parents?
*/
for( tmp = current_ns; tmp != &init_user_ns;
tmp = tmp->creator->user_ns) {
if (cred->user == tmp->creator) {
ucred->uid = 0;
ucred->gid = 0;
return;
}
}
/* No user namespace relationship so no mapping */
ucred->uid = overflowuid;
ucred->gid = overflowgid;
}
}
}
Eric
^ permalink raw reply
* Re: KS8695: possible NAPI issue
From: figo zhang @ 2010-03-08 9:04 UTC (permalink / raw)
To: Yegor Yefremov; +Cc: Dick Hollenbeck, netdev, zealcook
In-Reply-To: <f69abfc31003050700y57718a6aje3c1ff41d04748ae@mail.gmail.com>
2010/3/5 Yegor Yefremov <yegorslists@googlemail.com>:
>>> My tests look like following:
>>>
>>> 1. system start
>>> 2. ping one host: O.K.
>>> 3. nc -l -p 5000 > /var/test (about 1Mb): O.K.
>>> 4. ping the same host: failed
>> 1. type "arp" command, see the arp is exit or expire?
>
> debian:~# nc -l -p 5000 > /var/zImage
>
> debian:~# ping -c 1 192.168.1.38
>
> PING 192.168.1.38 (192.168.1.38) 56(84) bytes of data.
>
> From 192.168.1.66 icmp_seq=1 Destination Host Unreachable
>
>
>
> --- 192.168.1.38 ping statistics ---
>
> 1 packets transmitted, 0 received, +1 errors, 100% packet loss, time 0ms
>
>
>
> debian:~# arp
>
> Address HWtype HWaddress Flags Mask Iface
> 192.168.1.38 (incomplete) eth0
>
> 192.168.1.36 ether 00:10:18:39:19:aa C eth0
>
>
>> 2. at this point, see is it still have RX interrpt and receive packet in
>> ks8695_rx()? (in some watchpoint add printk).
>
> I've inserted some printks and I can see that interrupts are coming
> and the received count will be increased. I can also see my system
> sending arp requests. Even this ping request is visible in wireshark
> and its reply, but the ping utility sees nothing of it.
>
when you netcat finished, it cannot ping, right? at this point, would
you like to add some printk at RX and TX?
i want to see the target board have send arp packet, or have receive
arp reply packet.
you can add such funtion to print the packet buffer:
void print_mem(unsigned char *p,u32 len, u8 * s)
{
u32 i;
printk(" %s ram addr = %x , data len = %x",s, p, len);
for (i=0;i<len;i++) {
if (0==i%16) {
printk("\n 0x%02x : ",(p+i));
}
printk("%02x",*(u8 *)(p+i));
printk(" ");
}
printk("\n\n");
}
for TX: ks8695_start_xmit(struct sk_buff *skb, struct net_device *ndev)
=>
ksp->tx_ring[buff_n].status =
cpu_to_le32(TDES_IC | TDES_FS | TDES_LS |
(skb->len & TDES_TBS));
print_mem(skb->data, skb->len, "tx");
wmb();
for RX: static int ks8695_rx(struct ks8695_priv *ksp, int budget)
=>
/* Retrieve the sk_buff */
skb = ksp->rx_buffers[buff_n].skb;
print_mem(skb->data, skb->len, "rx");
/* Clear it from the ring */
ksp->rx_buffers[buff_n].skb = NULL;
^ permalink raw reply
* Re: [PATCH V3 2/8] tcp: use limited socket backlog
From: Eric Dumazet @ 2010-03-08 9:21 UTC (permalink / raw)
To: Zhu Yi, David Miller; +Cc: netdev
In-Reply-To: <1267769972.2867.1.camel@edumazet-laptop>
Le vendredi 05 mars 2010 à 07:19 +0100, Eric Dumazet a écrit :
> I'll submit a followup patch to add a MIB counter if your patch gets in.
>
As promised, here it is.
[PATCH] tcp: Add SNMP counters for backlog and min_ttl drops
Commit 6b03a53a (tcp: use limited socket backlog) added the possibility
of dropping frames when backlog queue is full.
Commit d218d111 (tcp: Generalized TTL Security Mechanism) added the
possibility of dropping frames when TTL is under a given limit.
This patch adds new SNMP MIB entries, named TCPBacklogDrop and
TCPMinTTLDrop, published in /proc/net/netstat in TcpExt: line
netstat -s | egrep "TCPBacklogDrop|TCPMinTTLDrop"
TCPBacklogDrop: 0
TCPMinTTLDrop: 0
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
include/linux/snmp.h | 2 ++
net/ipv4/proc.c | 2 ++
net/ipv4/tcp_ipv4.c | 7 +++++--
net/ipv6/tcp_ipv6.c | 3 ++-
4 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/include/linux/snmp.h b/include/linux/snmp.h
index e28f5a0..4435d10 100644
--- a/include/linux/snmp.h
+++ b/include/linux/snmp.h
@@ -225,6 +225,8 @@ enum
LINUX_MIB_SACKSHIFTED,
LINUX_MIB_SACKMERGED,
LINUX_MIB_SACKSHIFTFALLBACK,
+ LINUX_MIB_TCPBACKLOGDROP,
+ LINUX_MIB_TCPMINTTLDROP, /* RFC 5082 */
__LINUX_MIB_MAX
};
diff --git a/net/ipv4/proc.c b/net/ipv4/proc.c
index 242ed23..4f1f337 100644
--- a/net/ipv4/proc.c
+++ b/net/ipv4/proc.c
@@ -249,6 +249,8 @@ static const struct snmp_mib snmp4_net_list[] = {
SNMP_MIB_ITEM("TCPSackShifted", LINUX_MIB_SACKSHIFTED),
SNMP_MIB_ITEM("TCPSackMerged", LINUX_MIB_SACKMERGED),
SNMP_MIB_ITEM("TCPSackShiftFallback", LINUX_MIB_SACKSHIFTFALLBACK),
+ SNMP_MIB_ITEM("TCPBacklogDrop", LINUX_MIB_TCPBACKLOGDROP),
+ SNMP_MIB_ITEM("TCPMinTTLDrop", LINUX_MIB_TCPMINTTLDROP),
SNMP_MIB_SENTINEL
};
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 1915f7d..8d51d39 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1651,8 +1651,10 @@ int tcp_v4_rcv(struct sk_buff *skb)
if (!sk)
goto no_tcp_socket;
- if (iph->ttl < inet_sk(sk)->min_ttl)
+ if (unlikely(iph->ttl < inet_sk(sk)->min_ttl)) {
+ NET_INC_STATS_BH(net, LINUX_MIB_TCPMINTTLDROP);
goto discard_and_relse;
+ }
process:
if (sk->sk_state == TCP_TIME_WAIT)
@@ -1682,8 +1684,9 @@ process:
if (!tcp_prequeue(sk, skb))
ret = tcp_v4_do_rcv(sk, skb);
}
- } else if (sk_add_backlog(sk, skb)) {
+ } else if (unlikely(sk_add_backlog(sk, skb))) {
bh_unlock_sock(sk);
+ NET_INC_STATS_BH(net, LINUX_MIB_TCPBACKLOGDROP);
goto discard_and_relse;
}
bh_unlock_sock(sk);
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 2c378b1..9b6dbba 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1740,8 +1740,9 @@ process:
if (!tcp_prequeue(sk, skb))
ret = tcp_v6_do_rcv(sk, skb);
}
- } else if (sk_add_backlog(sk, skb)) {
+ } else if (unlikely(sk_add_backlog(sk, skb))) {
bh_unlock_sock(sk);
+ NET_INC_STATS_BH(net, LINUX_MIB_TCPBACKLOGDROP);
goto discard_and_relse;
}
bh_unlock_sock(sk);
^ permalink raw reply related
* Re: [PATCH 00/13] TProxy IPv6 support 2nd round
From: Amos Jeffries @ 2010-03-08 9:38 UTC (permalink / raw)
To: Balazs Scheidler; +Cc: Harald Welte, netfilter-devel, netdev
In-Reply-To: <1256547636.11361.18.camel@bzorp.balabit>
Balazs Scheidler wrote:
> On Sun, 2009-10-25 at 11:16 +0100, Harald Welte wrote:
>> Dear Balazs,
>>
>> as you might have read from other mails (and by the long period of silence),
>> Patrick McHardy is currently unavailable to perform his usual maintainer
>> role.
>>
>> I personally am too much out of touch with recent developments in
>> netfitler-land to be able to confidently review your patches...
>>
>> So unless somebody else from the team (Jozsef?, Pablo?) feels confident in
>> ACKing your patchset, I will have to ask for your patience until Patrick is
>> back and can do it by himself.
>
> Thanks for letting me know, hopefully Patrick gets better soon. I've
> planned another round of the TProxy patches as I got some comments at
> the previous round I'm yet to address.
>
> So no need to hurry.
>
Just bumping this topic up again.
What is the current status of these patches?
Our release which might make use of them goes into production sites in a
few weeks and I'm starting to see a little more interest in them from
our users.
AYJ
Squid Project
^ permalink raw reply
* [PATCH 3/6] qlcnic: fix multicast handling
From: Amit Kumar Salecha @ 2010-03-08 10:14 UTC (permalink / raw)
To: davem; +Cc: netdev, dhananjay.phadke, ameen.rahman, Sucheta Chakraborty
In-Reply-To: <1268043290-19929-1-git-send-email-amit.salecha@qlogic.com>
From: Sucheta Chakraborty <sucheta.chakraborty@qlogic.com>
For promiscuous mode, driver send request to device for deleting
multicast addresses and again it send request for adding them back
while exiting from this mode, this is bad for performance.
Just setting device in promiscuous mode is enough, no need to del/add
multicast addresses.
Signed-off-by: Sucheta Chakraborty <sucheta.chakraborty@qlogic.com>
Signed-off-by: Amit Kumar Salecha <amit.salecha@qlogic.com>
---
drivers/net/qlcnic/qlcnic_hw.c | 31 ++++++-------------------------
1 files changed, 6 insertions(+), 25 deletions(-)
diff --git a/drivers/net/qlcnic/qlcnic_hw.c b/drivers/net/qlcnic/qlcnic_hw.c
index e95646b..da00e16 100644
--- a/drivers/net/qlcnic/qlcnic_hw.c
+++ b/drivers/net/qlcnic/qlcnic_hw.c
@@ -398,20 +398,16 @@ qlcnic_sre_macaddr_change(struct qlcnic_adapter *adapter, u8 *addr,
return qlcnic_send_cmd_descs(adapter, (struct cmd_desc_type0 *)&req, 1);
}
-static int qlcnic_nic_add_mac(struct qlcnic_adapter *adapter,
- u8 *addr, struct list_head *del_list)
+static int qlcnic_nic_add_mac(struct qlcnic_adapter *adapter, u8 *addr)
{
struct list_head *head;
struct qlcnic_mac_list_s *cur;
/* look up if already exists */
- list_for_each(head, del_list) {
+ list_for_each(head, &adapter->mac_list) {
cur = list_entry(head, struct qlcnic_mac_list_s, list);
-
- if (memcmp(addr, cur->mac_addr, ETH_ALEN) == 0) {
- list_move_tail(head, &adapter->mac_list);
+ if (memcmp(addr, cur->mac_addr, ETH_ALEN) == 0)
return 0;
- }
}
cur = kzalloc(sizeof(struct qlcnic_mac_list_s), GFP_ATOMIC);
@@ -433,14 +429,9 @@ void qlcnic_set_multi(struct net_device *netdev)
struct dev_mc_list *mc_ptr;
u8 bcast_addr[ETH_ALEN] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
u32 mode = VPORT_MISS_MODE_DROP;
- LIST_HEAD(del_list);
- struct list_head *head;
- struct qlcnic_mac_list_s *cur;
- list_splice_tail_init(&adapter->mac_list, &del_list);
-
- qlcnic_nic_add_mac(adapter, adapter->mac_addr, &del_list);
- qlcnic_nic_add_mac(adapter, bcast_addr, &del_list);
+ qlcnic_nic_add_mac(adapter, adapter->mac_addr);
+ qlcnic_nic_add_mac(adapter, bcast_addr);
if (netdev->flags & IFF_PROMISC) {
mode = VPORT_MISS_MODE_ACCEPT_ALL;
@@ -455,22 +446,12 @@ void qlcnic_set_multi(struct net_device *netdev)
if (!netdev_mc_empty(netdev)) {
netdev_for_each_mc_addr(mc_ptr, netdev) {
- qlcnic_nic_add_mac(adapter, mc_ptr->dmi_addr,
- &del_list);
+ qlcnic_nic_add_mac(adapter, mc_ptr->dmi_addr);
}
}
send_fw_cmd:
qlcnic_nic_set_promisc(adapter, mode);
- head = &del_list;
- while (!list_empty(head)) {
- cur = list_entry(head->next, struct qlcnic_mac_list_s, list);
-
- qlcnic_sre_macaddr_change(adapter,
- cur->mac_addr, QLCNIC_MAC_DEL);
- list_del(&cur->list);
- kfree(cur);
- }
}
int qlcnic_nic_set_promisc(struct qlcnic_adapter *adapter, u32 mode)
--
1.6.0.2
^ permalink raw reply related
* [PATCH 2/6] qlcnic: additional driver statistics.
From: Amit Kumar Salecha @ 2010-03-08 10:14 UTC (permalink / raw)
To: davem; +Cc: netdev, dhananjay.phadke, ameen.rahman, Sucheta Chakraborty
In-Reply-To: <1268043290-19929-1-git-send-email-amit.salecha@qlogic.com>
From: Sucheta Chakraborty <sucheta.chakraborty@qlogic.com>
Statistics added for lro/lso bytes, count for tx stop queue and
wake queue and skb alloc failure count.
Signed-off-by: Sucheta Chakraborty <sucheta.chakraborty@qlogic.com>
Signed-off-by: Amit Kumar Salecha <amit.salecha@qlogic.com>
---
drivers/net/qlcnic/qlcnic.h | 5 +++++
drivers/net/qlcnic/qlcnic_ethtool.c | 11 +++++++++++
drivers/net/qlcnic/qlcnic_hw.c | 1 +
drivers/net/qlcnic/qlcnic_init.c | 8 ++++++--
drivers/net/qlcnic/qlcnic_main.c | 5 +++++
5 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/drivers/net/qlcnic/qlcnic.h b/drivers/net/qlcnic/qlcnic.h
index b40a851..9897b69 100644
--- a/drivers/net/qlcnic/qlcnic.h
+++ b/drivers/net/qlcnic/qlcnic.h
@@ -423,6 +423,11 @@ struct qlcnic_adapter_stats {
u64 lro_pkts;
u64 rxbytes;
u64 txbytes;
+ u64 lrobytes;
+ u64 lso_frames;
+ u64 xmit_on;
+ u64 xmit_off;
+ u64 skb_alloc_failure;
};
/*
diff --git a/drivers/net/qlcnic/qlcnic_ethtool.c b/drivers/net/qlcnic/qlcnic_ethtool.c
index ef12792..f83e15f 100644
--- a/drivers/net/qlcnic/qlcnic_ethtool.c
+++ b/drivers/net/qlcnic/qlcnic_ethtool.c
@@ -59,6 +59,17 @@ static const struct qlcnic_stats qlcnic_gstrings_stats[] = {
QLC_SIZEOF(stats.rxbytes), QLC_OFF(stats.rxbytes)},
{"tx_bytes",
QLC_SIZEOF(stats.txbytes), QLC_OFF(stats.txbytes)},
+ {"lrobytes",
+ QLC_SIZEOF(stats.lrobytes), QLC_OFF(stats.lrobytes)},
+ {"lso_frames",
+ QLC_SIZEOF(stats.lso_frames), QLC_OFF(stats.lso_frames)},
+ {"xmit_on",
+ QLC_SIZEOF(stats.xmit_on), QLC_OFF(stats.xmit_on)},
+ {"xmit_off",
+ QLC_SIZEOF(stats.xmit_off), QLC_OFF(stats.xmit_off)},
+ {"skb_alloc_failure", QLC_SIZEOF(stats.skb_alloc_failure),
+ QLC_OFF(stats.skb_alloc_failure)},
+
};
#define QLCNIC_STATS_LEN ARRAY_SIZE(qlcnic_gstrings_stats)
diff --git a/drivers/net/qlcnic/qlcnic_hw.c b/drivers/net/qlcnic/qlcnic_hw.c
index 99a4d13..e95646b 100644
--- a/drivers/net/qlcnic/qlcnic_hw.c
+++ b/drivers/net/qlcnic/qlcnic_hw.c
@@ -349,6 +349,7 @@ qlcnic_send_cmd_descs(struct qlcnic_adapter *adapter,
if (nr_desc >= qlcnic_tx_avail(tx_ring)) {
netif_tx_stop_queue(tx_ring->txq);
__netif_tx_unlock_bh(tx_ring->txq);
+ adapter->stats.xmit_off++;
return -EBUSY;
}
diff --git a/drivers/net/qlcnic/qlcnic_init.c b/drivers/net/qlcnic/qlcnic_init.c
index ea00ab4..f0df971 100644
--- a/drivers/net/qlcnic/qlcnic_init.c
+++ b/drivers/net/qlcnic/qlcnic_init.c
@@ -1114,8 +1114,10 @@ qlcnic_alloc_rx_skb(struct qlcnic_adapter *adapter,
struct pci_dev *pdev = adapter->pdev;
buffer->skb = dev_alloc_skb(rds_ring->skb_size);
- if (!buffer->skb)
+ if (!buffer->skb) {
+ adapter->stats.skb_alloc_failure++;
return -ENOMEM;
+ }
skb = buffer->skb;
@@ -1289,7 +1291,7 @@ qlcnic_process_lro(struct qlcnic_adapter *adapter,
netif_receive_skb(skb);
adapter->stats.lro_pkts++;
- adapter->stats.rxbytes += length;
+ adapter->stats.lrobytes += length;
return buffer;
}
@@ -1505,6 +1507,8 @@ qlcnic_process_rcv_diag(struct qlcnic_adapter *adapter,
adapter->diag_cnt++;
dev_kfree_skb_any(skb);
+ adapter->stats.rx_pkts++;
+ adapter->stats.rxbytes += length;
return buffer;
}
diff --git a/drivers/net/qlcnic/qlcnic_main.c b/drivers/net/qlcnic/qlcnic_main.c
index 665e8e5..fc72156 100644
--- a/drivers/net/qlcnic/qlcnic_main.c
+++ b/drivers/net/qlcnic/qlcnic_main.c
@@ -118,6 +118,7 @@ qlcnic_update_cmd_producer(struct qlcnic_adapter *adapter,
if (qlcnic_tx_avail(tx_ring) <= TX_STOP_THRESH) {
netif_stop_queue(adapter->netdev);
smp_mb();
+ adapter->stats.xmit_off++;
}
}
@@ -1385,6 +1386,7 @@ qlcnic_tso_check(struct net_device *netdev,
int copied, offset, copy_len, hdr_len = 0, tso = 0, vlan_oob = 0;
struct cmd_desc_type0 *hwdesc;
struct vlan_ethhdr *vh;
+ struct qlcnic_adapter *adapter = netdev_priv(netdev);
if (protocol == cpu_to_be16(ETH_P_8021Q)) {
@@ -1494,6 +1496,7 @@ qlcnic_tso_check(struct net_device *netdev,
tx_ring->producer = producer;
barrier();
+ adapter->stats.lso_frames++;
}
static int
@@ -1573,6 +1576,7 @@ qlcnic_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
if (unlikely(no_of_desc + 2 > qlcnic_tx_avail(tx_ring))) {
netif_stop_queue(netdev);
+ adapter->stats.xmit_off++;
return NETDEV_TX_BUSY;
}
@@ -1880,6 +1884,7 @@ static int qlcnic_process_cmd_ring(struct qlcnic_adapter *adapter)
if (qlcnic_tx_avail(tx_ring) > TX_STOP_THRESH) {
netif_wake_queue(netdev);
adapter->tx_timeo_cnt = 0;
+ adapter->stats.xmit_on++;
}
__netif_tx_unlock(tx_ring->txq);
}
--
1.6.0.2
^ permalink raw reply related
* [PATCH 4/6] qlcnic: validate unified fw image
From: Amit Kumar Salecha @ 2010-03-08 10:14 UTC (permalink / raw)
To: davem; +Cc: netdev, dhananjay.phadke, ameen.rahman, Sucheta Chakraborty
In-Reply-To: <1268043290-19929-1-git-send-email-amit.salecha@qlogic.com>
From: Sucheta Chakraborty <sucheta@dut6195.unminc.com>
Validate all sections of unified fw image, before accessing them,
to avoid seg fault.
Signed-off-by: Sucheta Chakraborty <sucheta@dut6195.unminc.com>
Signed-off-by: Amit Kumar Salecha <amit.salecha@qlogic.com>
---
drivers/net/qlcnic/qlcnic_init.c | 146 ++++++++++++++++++++++++++++++++++++--
1 files changed, 139 insertions(+), 7 deletions(-)
diff --git a/drivers/net/qlcnic/qlcnic_init.c b/drivers/net/qlcnic/qlcnic_init.c
index f0df971..21a6e9f 100644
--- a/drivers/net/qlcnic/qlcnic_init.c
+++ b/drivers/net/qlcnic/qlcnic_init.c
@@ -568,21 +568,123 @@ struct uni_table_desc *qlcnic_get_table_desc(const u8 *unirom, int section)
return NULL;
}
+#define FILEHEADER_SIZE (14 * 4)
+
static int
-qlcnic_set_product_offs(struct qlcnic_adapter *adapter)
+qlcnic_validate_header(struct qlcnic_adapter *adapter)
{
- struct uni_table_desc *ptab_descr;
const u8 *unirom = adapter->fw->data;
- u32 i;
+ struct uni_table_desc *directory = (struct uni_table_desc *) &unirom[0];
+ __le32 fw_file_size = adapter->fw->size;
__le32 entries;
+ __le32 entry_size;
+ __le32 tab_size;
+
+ if (fw_file_size < FILEHEADER_SIZE)
+ return -EINVAL;
+
+ entries = cpu_to_le32(directory->num_entries);
+ entry_size = cpu_to_le32(directory->entry_size);
+ tab_size = cpu_to_le32(directory->findex) + (entries * entry_size);
+
+ if (fw_file_size < tab_size)
+ return -EINVAL;
+
+ return 0;
+}
+
+static int
+qlcnic_validate_bootld(struct qlcnic_adapter *adapter)
+{
+ struct uni_table_desc *tab_desc;
+ struct uni_data_desc *descr;
+ const u8 *unirom = adapter->fw->data;
+ int idx = cpu_to_le32(*((int *)&unirom[adapter->file_prd_off] +
+ QLCNIC_UNI_BOOTLD_IDX_OFF));
+ __le32 offs;
+ __le32 tab_size;
+ __le32 data_size;
+
+ tab_desc = qlcnic_get_table_desc(unirom, QLCNIC_UNI_DIR_SECT_BOOTLD);
+
+ if (!tab_desc)
+ return -EINVAL;
+
+ tab_size = cpu_to_le32(tab_desc->findex) +
+ (cpu_to_le32(tab_desc->entry_size * (idx + 1)));
+
+ if (adapter->fw->size < tab_size)
+ return -EINVAL;
+
+ offs = cpu_to_le32(tab_desc->findex) +
+ (cpu_to_le32(tab_desc->entry_size) * (idx));
+ descr = (struct uni_data_desc *)&unirom[offs];
+
+ data_size = descr->findex + cpu_to_le32(descr->size);
+
+ if (adapter->fw->size < data_size)
+ return -EINVAL;
+
+ return 0;
+}
+
+static int
+qlcnic_validate_fw(struct qlcnic_adapter *adapter)
+{
+ struct uni_table_desc *tab_desc;
+ struct uni_data_desc *descr;
+ const u8 *unirom = adapter->fw->data;
+ int idx = cpu_to_le32(*((int *)&unirom[adapter->file_prd_off] +
+ QLCNIC_UNI_FIRMWARE_IDX_OFF));
+ __le32 offs;
+ __le32 tab_size;
+ __le32 data_size;
+
+ tab_desc = qlcnic_get_table_desc(unirom, QLCNIC_UNI_DIR_SECT_FW);
+
+ if (!tab_desc)
+ return -EINVAL;
+
+ tab_size = cpu_to_le32(tab_desc->findex) +
+ (cpu_to_le32(tab_desc->entry_size * (idx + 1)));
+
+ if (adapter->fw->size < tab_size)
+ return -EINVAL;
+
+ offs = cpu_to_le32(tab_desc->findex) +
+ (cpu_to_le32(tab_desc->entry_size) * (idx));
+ descr = (struct uni_data_desc *)&unirom[offs];
+ data_size = descr->findex + cpu_to_le32(descr->size);
+
+ if (adapter->fw->size < data_size)
+ return -EINVAL;
+
+ return 0;
+}
+
+static int
+qlcnic_validate_product_offs(struct qlcnic_adapter *adapter)
+{
+ struct uni_table_desc *ptab_descr;
+ const u8 *unirom = adapter->fw->data;
int mn_present = qlcnic_has_mn(adapter);
+ __le32 entries;
+ __le32 entry_size;
+ __le32 tab_size;
+ u32 i;
ptab_descr = qlcnic_get_table_desc(unirom,
QLCNIC_UNI_DIR_SECT_PRODUCT_TBL);
- if (ptab_descr == NULL)
- return -1;
+ if (!ptab_descr)
+ return -EINVAL;
entries = cpu_to_le32(ptab_descr->num_entries);
+ entry_size = cpu_to_le32(ptab_descr->entry_size);
+ tab_size = cpu_to_le32(ptab_descr->findex) + (entries * entry_size);
+
+ if (adapter->fw->size < tab_size)
+ return -EINVAL;
+
nomn:
for (i = 0; i < entries; i++) {
@@ -609,7 +711,37 @@ nomn:
mn_present = 0;
goto nomn;
}
- return -1;
+ return -EINVAL;
+}
+
+static int
+qlcnic_validate_unified_romimage(struct qlcnic_adapter *adapter)
+{
+ if (qlcnic_validate_header(adapter)) {
+ dev_err(&adapter->pdev->dev,
+ "unified image: header validation failed\n");
+ return -EINVAL;
+ }
+
+ if (qlcnic_validate_product_offs(adapter)) {
+ dev_err(&adapter->pdev->dev,
+ "unified image: product validation failed\n");
+ return -EINVAL;
+ }
+
+ if (qlcnic_validate_bootld(adapter)) {
+ dev_err(&adapter->pdev->dev,
+ "unified image: bootld validation failed\n");
+ return -EINVAL;
+ }
+
+ if (qlcnic_validate_fw(adapter)) {
+ dev_err(&adapter->pdev->dev,
+ "unified image: firmware validation failed\n");
+ return -EINVAL;
+ }
+
+ return 0;
}
static
@@ -858,7 +990,7 @@ qlcnic_validate_firmware(struct qlcnic_adapter *adapter)
u8 fw_type = adapter->fw_type;
if (fw_type == QLCNIC_UNIFIED_ROMIMAGE) {
- if (qlcnic_set_product_offs(adapter))
+ if (qlcnic_validate_unified_romimage(adapter))
return -EINVAL;
min_size = QLCNIC_UNI_FW_MIN_SIZE;
--
1.6.0.2
^ permalink raw reply related
* [PATCH 1/6] qlcnic: fix tx csum status
From: Amit Kumar Salecha @ 2010-03-08 10:14 UTC (permalink / raw)
To: davem; +Cc: netdev, dhananjay.phadke, ameen.rahman, Sucheta Chakraborty
In-Reply-To: <1268043290-19929-1-git-send-email-amit.salecha@qlogic.com>
From: Sucheta Chakraborty <sucheta.chakraborty@qlogic.com>
Kernel default tx csum function (ethtool_op_get_tx_csum) doesn't show
correct csum status. It takes various FLAGS (NETIF_F_ALL_CSUM) in account
to show tx csum status, which driver doesn't set while disabling tx csum.
Signed-off-by: Sucheta Chakraborty <sucheta.chakraborty@qlogic.com>
Signed-off-by: Amit Kumar Salecha <amit.salecha@qlogic.com>
---
drivers/net/qlcnic/qlcnic_ethtool.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/drivers/net/qlcnic/qlcnic_ethtool.c b/drivers/net/qlcnic/qlcnic_ethtool.c
index 8da6ec8..ef12792 100644
--- a/drivers/net/qlcnic/qlcnic_ethtool.c
+++ b/drivers/net/qlcnic/qlcnic_ethtool.c
@@ -785,6 +785,11 @@ qlcnic_get_ethtool_stats(struct net_device *dev,
}
}
+static u32 qlcnic_get_tx_csum(struct net_device *dev)
+{
+ return dev->features & NETIF_F_IP_CSUM;
+}
+
static u32 qlcnic_get_rx_csum(struct net_device *dev)
{
struct qlcnic_adapter *adapter = netdev_priv(dev);
@@ -995,6 +1000,7 @@ const struct ethtool_ops qlcnic_ethtool_ops = {
.set_ringparam = qlcnic_set_ringparam,
.get_pauseparam = qlcnic_get_pauseparam,
.set_pauseparam = qlcnic_set_pauseparam,
+ .get_tx_csum = qlcnic_get_tx_csum,
.set_tx_csum = ethtool_op_set_tx_csum,
.set_sg = ethtool_op_set_sg,
.get_tso = qlcnic_get_tso,
--
1.6.0.2
^ permalink raw reply related
* [PATCH 0/6]qlcnic: bug fixes
From: Amit Kumar Salecha @ 2010-03-08 10:14 UTC (permalink / raw)
To: davem; +Cc: netdev, dhananjay.phadke, ameen.rahman
Hi
Series of 6 patches to fix minor bugs and to enhance driver statistics.
Please apply them in net-2.6 tree.
-Amit Salecha
^ permalink raw reply
* [PATCH 5/6] qlcnic: fix bios version check
From: Amit Kumar Salecha @ 2010-03-08 10:14 UTC (permalink / raw)
To: davem; +Cc: netdev, dhananjay.phadke, ameen.rahman
In-Reply-To: <1268043290-19929-1-git-send-email-amit.salecha@qlogic.com>
Bios sub version from unified fw image is calculated incorrect.
Signed-off-by: Amit Kumar Salecha <amit.salecha@qlogic.com>
---
drivers/net/qlcnic/qlcnic_init.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/net/qlcnic/qlcnic_init.c b/drivers/net/qlcnic/qlcnic_init.c
index 21a6e9f..7c34e4e 100644
--- a/drivers/net/qlcnic/qlcnic_init.c
+++ b/drivers/net/qlcnic/qlcnic_init.c
@@ -847,7 +847,7 @@ qlcnic_get_bios_version(struct qlcnic_adapter *adapter)
bios_ver = cpu_to_le32(*((u32 *) (&fw->data[prd_off])
+ QLCNIC_UNI_BIOS_VERSION_OFF));
- return (bios_ver << 24) + ((bios_ver >> 8) & 0xff00) + (bios_ver >> 24);
+ return (bios_ver << 16) + ((bios_ver >> 8) & 0xff00) + (bios_ver >> 24);
}
int
--
1.6.0.2
^ permalink raw reply related
* [PATCH 6/6] qlcnic: remove extra space from board names
From: Amit Kumar Salecha @ 2010-03-08 10:14 UTC (permalink / raw)
To: davem; +Cc: netdev, dhananjay.phadke, ameen.rahman
In-Reply-To: <1268043290-19929-1-git-send-email-amit.salecha@qlogic.com>
Signed-off-by: Amit Kumar Salecha <amit.salecha@qlogic.com>
---
drivers/net/qlcnic/qlcnic.h | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/qlcnic/qlcnic.h b/drivers/net/qlcnic/qlcnic.h
index 9897b69..0da94b2 100644
--- a/drivers/net/qlcnic/qlcnic.h
+++ b/drivers/net/qlcnic/qlcnic.h
@@ -1100,11 +1100,11 @@ struct qlcnic_brdinfo {
static const struct qlcnic_brdinfo qlcnic_boards[] = {
{0x1077, 0x8020, 0x1077, 0x203,
- "8200 Series Single Port 10GbE Converged Network Adapter \
- (TCP/IP Networking)"},
+ "8200 Series Single Port 10GbE Converged Network Adapter "
+ "(TCP/IP Networking)"},
{0x1077, 0x8020, 0x1077, 0x207,
- "8200 Series Dual Port 10GbE Converged Network Adapter \
- (TCP/IP Networking)"},
+ "8200 Series Dual Port 10GbE Converged Network Adapter "
+ "(TCP/IP Networking)"},
{0x1077, 0x8020, 0x1077, 0x20b,
"3200 Series Dual Port 10Gb Intelligent Ethernet Adapter"},
{0x1077, 0x8020, 0x1077, 0x20c,
--
1.6.0.2
^ permalink raw reply related
* RE: bnx2x crash dump on high network load
From: Fischer, Anna @ 2010-03-08 11:02 UTC (permalink / raw)
To: Eilon Greenstein; +Cc: eliezert, Michael Chan, netdev@vger.kernel.org
In-Reply-To: <BD3F7F1EFBA6D54DB056C4FFA451400803A1F8F2F7@SJEXCHCCR01.corp.ad.broadcom.com>
I have this option set in my /etc/modprobe.conf.local. The problem must be something else.
> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> On Behalf Of Eilon Greenstein
> Sent: 05 March 2010 17:32
> To: Fischer, Anna
> Cc: eliezert; Michael Chan; netdev@vger.kernel.org
> Subject: RE: bnx2x crash dump on high network load
>
> Hi Anna,
>
> On this kernel, enabling bridging (like you do on virtualization) does
> not turn off LRO automatically. Please try to load the bnx2x with
> disable_tpa=1 (TPA is the HW based LRO feature).
>
> Regards,
> Eilon
>
> -----Original Message-----
> From: Fischer, Anna [mailto:anna.fischer@hp.com]
> Sent: Friday, March 05, 2010 7:08 PM
> To: netdev@vger.kernel.org
> Cc: eliezert; Eilon Greenstein; Michael Chan
> Subject: bnx2x crash dump on high network load
>
> I am seeing driver crashes with a bnx2x version 1.50.16. I have attached
> two crash dumps that I have captured.
>
> I run Linux with the following kernel:
>
> # uname -a
> Linux sup-prj-441106 2.6.16.60-0.54.5-debug #1 SMP Fri Sep 4 01:28:03
> UTC 2009 x86_64 x86_64 x86_64 GNU/Linux
>
> I am seeing these driver crashes when I run virtual machines (VMware
> Server) and those VMs do high bandwidth network I/O, so there are a lot
> of packets going through the NIC.
>
> Can you give me any hints for what the problem here could be?
>
> Thanks,
> Anna
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
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