* Re: [PATCHv8 3/3] vhost_net: a kernel-level virtio server
From: Michael S. Tsirkin @ 2009-11-08 11:35 UTC (permalink / raw)
To: Rusty Russell
Cc: netdev, virtualization, kvm, linux-kernel, mingo, linux-mm, akpm,
hpa, gregory.haskins, s.hetze, Daniel Walker, Eric Dumazet,
Paul E. McKenney
In-Reply-To: <200911061529.17500.rusty@rustcorp.com.au>
On Fri, Nov 06, 2009 at 03:29:17PM +1030, Rusty Russell wrote:
> On Thu, 5 Nov 2009 02:27:24 am Michael S. Tsirkin wrote:
> > What it is: vhost net is a character device that can be used to reduce
> > the number of system calls involved in virtio networking.
>
> Hi Michael,
>
> Now everyone else has finally kicked all the tires and it seems to pass,
> I've done a fairly complete review. Generally, it's really nice; just one
> bug and a few minor suggestions for polishing.
Thanks for the review! I'll add more polishing and repost.
Answers to some questions below.
> > +/* Caller must have TX VQ lock */
> > +static void tx_poll_stop(struct vhost_net *net)
> > +{
> > + if (likely(net->tx_poll_state != VHOST_NET_POLL_STARTED))
> > + return;
>
> likely? Really?
Hmm ... yes. tx poll stop is called on each packet (as long as we do not
fill up 1/2 backend queue), the first call will stop polling
the rest checks state and does nothing.
This is because we normally do not care when the message has left the
queue in backend device: we tell backend to send it and forget. We only
start polling when backend tx queue fills up.
Makes sense?
> > + for (;;) {
> > + head = vhost_get_vq_desc(&net->dev, vq, vq->iov, &out, &in,
> > + NULL, NULL);
>
> Danger! You need an arg to vhost_get_vq_desc to tell it the max desc size
> you can handle. Otherwise, it's only limited by ring size, and a malicious
> guest can overflow you here, and below:
In fact, I think this is not a bug. This happens to work correctly
(even with malicious guests) because vhost_get_vq_desc is hard-coded to
check VHOST_NET_MAX_SG, so in fact no overflow is possible. I agree
that it's mich nicer to pass iovec size to vhost_get_vq_desc.
>
> > + /* Skip header. TODO: support TSO. */
> > + s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, out);
> ...
> > +
> > + use_mm(net->dev.mm);
> > + mutex_lock(&vq->mutex);
> > + vhost_no_notify(vq);
>
> I prefer a name like "vhost_disable_notify()".
Good idea.
> > + /* OK, now we need to know about added descriptors. */
> > + if (head == vq->num && vhost_notify(vq))
> > + /* They could have slipped one in as we were doing that:
> > + * check again. */
> > + continue;
> > + /* Nothing new? Wait for eventfd to tell us they refilled. */
> > + if (head == vq->num)
> > + break;
> > + /* We don't need to be notified again. */
> > + vhost_no_notify(vq);
>
> Similarly, vhost_enable_notify. This one is particularly misleading since
> it doesn't actually notify anything!
Good idea.
>
> In particular, this code would be neater as:
>
> if (head == vq->num) {
> if (vhost_enable_notify(vq)) {
> /* Try again, they could have slipped one in. */
> continue;
> }
> /* Nothing more to do. */
> break;
> }
> vhost_disable_notify(vq);
>
> Now, AFAICT vhost_notify()/enable_notify() would be better rewritten to
> return true only when there's more pending. Saves a loop around here most
> of the time.
OKay, I'll look into this. It kind of annoys me that we would do
get_user for the same value twice: once in vhost_enable_notify and once
in vhost_get_vq_desc. OTOH, all the loop does is call vhost_get_vq_desc
again.
> Also, the vhost_no_notify/vhost_disable_notify() can be moved
> out of the loop entirely.
I don't think it can, if we enabled notification and then see more
descriptors in queue, we want to disable notification again. But it can
be
> (It could be under an if (unlikely(enabled)), not
> sure if it's worth it).
if (unlikely(vhost_enable_notify(vq))) {
/* Try again, they have slipped one in. */
vhost_disable_notify(vq);
continue;
}
>
> > + len = err;
> > + err = memcpy_toiovec(vq->hdr, (unsigned char *)&hdr, hdr_size);
>
> That unsigned char * arg to memcpy_toiovec is annoying. A patch might be
> nice, separate from this effort.
Sounds good.
> > +static int vhost_net_open(struct inode *inode, struct file *f)
> > +{
> > + struct vhost_net *n = kzalloc(sizeof *n, GFP_KERNEL);
> > + int r;
> > + if (!n)
> > + return -ENOMEM;
> > + f->private_data = n;
> > + n->vqs[VHOST_NET_VQ_TX].handle_kick = handle_tx_kick;
> > + n->vqs[VHOST_NET_VQ_RX].handle_kick = handle_rx_kick;
>
> I have a personal dislike of calloc for structures.
You mean zalloc?
> In userspace, it's because valgrind can't spot uninitialized fields.
> These days a similar argument applies in the kernel, because we have
> KMEMCHECK now. If someone adds a field to the struct and forgets to
> initialize it, we can spot it.
OK.
> > +static void vhost_net_enable_vq(struct vhost_net *n, int index)
> > +{
> > + struct socket *sock = n->vqs[index].private_data;
>
> OK, I can't help but this that presenting the vqs as an array doesn't buy
> us very much. Esp. if you change vhost_dev_init to take a NULL-terminated
> varargs. I think readability would improve. It means passing a vq around
> rather than an index.
>
> Not completely sure it'll be a win tho.
Hmm, varargs sounds a bit complex. But I agree readability for
vhost_net_enable_vq and friends would benefit from passing a vq around
rather than an index. I'll try it out and do it if it's a win.
> > +static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
> > +{
> > + struct socket *sock, *oldsock = NULL;
> ...
> > + sock = get_socket(fd);
> > + if (IS_ERR(sock)) {
> > + r = PTR_ERR(sock);
> > + goto done;
> > + }
> > +
> > + /* start polling new socket */
> > + oldsock = vq->private_data;
> ...
> > +done:
> > + mutex_unlock(&n->dev.mutex);
> > + if (oldsock) {
> > + vhost_net_flush_vq(n, index);
> > + fput(oldsock->file);
>
> I dislike this style; I prefer multiple different goto points, one for when
> oldsock is set, and one for when it's not.
>
> That way, gcc warns us about uninitialized variables if we get it wrong.
OK.
> > +static long vhost_net_reset_owner(struct vhost_net *n)
> > +{
> > + struct socket *tx_sock = NULL;
> > + struct socket *rx_sock = NULL;
> > + long r;
>
> This should be called "err", since that's what it is.
OK.
> > +static void 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);
> > + n->dev.acked_features = features;
>
> Why is this called "acked_features"? Not just "features"? I expected
> to see code which exposed these back to userspace, and didn't.
Not sure how do you mean. Userspace sets them, why
does it want to get them exposed back?
> > + case VHOST_GET_FEATURES:
> > + features = VHOST_FEATURES;
> > + return put_user(features, featurep);
> > + case VHOST_ACK_FEATURES:
> > + r = get_user(features, featurep);
> > + /* No features for now */
> > + if (r < 0)
> > + return r;
> > + if (features & ~VHOST_FEATURES)
> > + return -EOPNOTSUPP;
> > + vhost_net_set_features(n, features);
>
> OK, from the userspace POV it's "get features" then "ack features". But
> I think "VHOST_SET_FEATURES" is more consistent, despite this usage.
OK.
> > + switch (ioctl) {
> > + case VHOST_SET_VRING_NUM:
>
> I haven't looked at your userspace implementation, but does a generic
> VHOST_SET_VRING_STATE & VHOST_GET_VRING_STATE with a struct make more
> sense? It'd be simpler here,
Not by much though, right?
> but not sure if it'd be simpler to use?
The problem is with VHOST_SET_VRING_BASE as well. I want it to be
separate because I want to make it possible to relocate e.g. used ring
to another address while ring is running. This would be a good debugging
tool (you look at kernel's used ring, check descriptor, then update
guest's used ring) and also possibly an extra way to do migration. And
it's nicer to have vring size separate as well, because it is
initialized by host and never changed, right?
We could merge DESC, AVAIL, USED, and it will reduce the amount of code
in userspace. With both base, size and fds separate, it seemed a bit
more symmetrical to have desc/avail/used separate as well.
What's your opinion?
> (Not the fd-setting ioctls of course)
>
> > + case VHOST_SET_VRING_LOG:
> > + r = copy_from_user(&a, argp, sizeof a);
> > + if (r < 0)
> > + break;
> > + if (a.padding) {
> > + r = -EOPNOTSUPP;
> > + break;
> > + }
> > + if (a.user_addr == VHOST_VRING_LOG_DISABLE) {
> > + vq->log_used = false;
> > + break;
> > + }
> > + if (a.user_addr & (sizeof *vq->used->ring - 1)) {
> > + r = -EINVAL;
> > + break;
> > + }
> > + vq->log_used = true;
> > + vq->log_addr = a.user_addr;
> > + break;
>
> For future reference, this is *exactly* the kind of thing which would have
> been nice as a followup patch. Easy to separate, easy to review, not critical
> to the core.
Yes. It's not too late to split it out though: should I do it yet?
> > +/* TODO: This is really inefficient. We need something like get_user()
> > + * (instruction directly accesses the data, with an exception table entry
> > + * returning -EFAULT). See Documentation/x86/exception-tables.txt.
> > + */
> > +static int set_bit_to_user(int nr, void __user *addr)
> > +{
>
> I guess we won't be dealing with many contiguous pages, otherwise we could
> get a cheap speedup making this set_bits_to_user(int nr, int num_bits...).
No idea. Let's keep it as simple as possible for now?
> > +/* Each buffer in the virtqueues is actually a chain of descriptors. This
> > + * function returns the next descriptor in the chain,
> > + * or -1 if we're at the end. */
> > +static unsigned next_desc(struct vring_desc *desc)
> > +{
> > + unsigned int next;
> > +
> > + /* If this descriptor says it doesn't chain, we're done. */
> > + if (!(desc->flags & VRING_DESC_F_NEXT))
> > + return -1;
>
> Hmm, prefer s/-1/-1U/ in comment, here, and below. Clarifies a bit.
Good idea.
> > +/* After we've used one of their buffers, we tell them about it. We'll then
> > + * want to send them an interrupt, using vq->call. */
>
> This comment has too much cut & paste:
I tried to cut and paste as many comments as possible, this
made it easy to audit the code by comparing it with lguest,
and made them much more witty. But yes, this is definitely going
overboard as we do not have vq->call at all :)
> ... want to notify the guest, using the eventfd */
>
> > +/* This actually sends the interrupt for this virtqueue */
> > +void vhost_trigger_irq(struct vhost_dev *dev, struct vhost_virtqueue *vq)
> > +{
>
> Rename vhost_notify_eventfd() or something, and fix comments?
Sounds good. Since I'm renaming vhost_notify to vhost_enable_notify,
this one can just become vhost_notify.
> > +enum {
> > + VHOST_NET_MAX_SG = MAX_SKB_FRAGS + 2,
>
> +2? Believable, but is it correct?
+ 1 is for skb head, + 1 is for virtio net header.
I'll add a comment.
> > +/* Poll a file (eventfd or socket) */
> > +/* Note: there's nothing vhost specific about this structure. */
> > +struct vhost_poll {
>
> This comment really helped while reading the code. Kudos!
>
> Thanks!
> Rusty.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH 39/75] bnx2x: declare MODULE_FIRMWARE
From: Eilon Greenstein @ 2009-11-08 11:06 UTC (permalink / raw)
To: Ben Hutchings; +Cc: David Miller, netdev
In-Reply-To: <1257630819.15927.437.camel@localhost>
On Sat, 2009-11-07 at 13:53 -0800, Ben Hutchings wrote:
> Replace run-time string formatting with preprocessor string
> manipulation.
>
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
Thanks Ben.
Acked-by: Eilon Greenstein <eilong@broadcom.com>
^ permalink raw reply
* Re: [PATCH v3 net-next-2.6] can: Driver for the Microchip MCP251x SPI CAN controllers
From: David Miller @ 2009-11-08 9:51 UTC (permalink / raw)
To: wg; +Cc: chripell, socketcan-core, spi-devel-general, netdev, pthomas8589
In-Reply-To: <4AF68EDF.60405@grandegger.com>
From: Wolfgang Grandegger <wg@grandegger.com>
Date: Sun, 08 Nov 2009 10:26:55 +0100
> David Miller wrote:
>> From: Christian Pellegrin <chripell@fsfe.org>
>> Date: Tue, 3 Nov 2009 10:07:00 +0100
>>
>>> Signed-off-by: Christian Pellegrin <chripell@fsfe.org>
>
> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
>
>> Wolfgang, others, any acks on this?
>
> From my point of view the patch is OK. I was just awaiting some testing
> feedback from Paul, but that may take more time. Please apply, we will
> fix eventual problems when they show up.
Ok, done, thanks.
^ permalink raw reply
* Re: [PATCH] can: fix WARN_ON dump in net/core/rtnetlink.c:rtmsg_ifinfo()
From: Wolfgang Grandegger @ 2009-11-08 9:30 UTC (permalink / raw)
To: David Miller
Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w,
netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20091108.004602.213063654.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
David Miller wrote:
> From: Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
> Date: Sat, 07 Nov 2009 10:53:13 +0100
>
>> On older kernels, e.g. 2.6.27, a WARN_ON dump in rtmsg_ifinfo()
>> is thrown when the CAN device is registered due to insufficient
>> skb space, as reported by various users. This patch adds the
>> rtnl_link_ops "get_size" to fix the problem. I think this patch
>> is required for more recent kernels as well, even if no WARN_ON
>> dumps are triggered. Maybe we also need "get_xstats_size" for
>> the CAN xstats.
>>
>> Signed-off-by: Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
>
> Applied to net-2.6, thanks Wolfgang.
Thanks, the commit message included some questions. What is the rule
using the rtnl_link_ops "get_size" or "get_xstats_size". Are these
mandatory if the corresponding fill functions are used?
Wolfgang.
^ permalink raw reply
* Re: [PATCH v3 net-next-2.6] can: Driver for the Microchip MCP251x SPI CAN controllers
From: Wolfgang Grandegger @ 2009-11-08 9:26 UTC (permalink / raw)
To: David Miller
Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w,
spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
chripell-VaTbYqLCNhc, netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20091108.005046.42104186.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
David Miller wrote:
> From: Christian Pellegrin <chripell-VaTbYqLCNhc@public.gmane.org>
> Date: Tue, 3 Nov 2009 10:07:00 +0100
>
>> Signed-off-by: Christian Pellegrin <chripell-VaTbYqLCNhc@public.gmane.org>
Signed-off-by: Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
> Wolfgang, others, any acks on this?
>From my point of view the patch is OK. I was just awaiting some testing
feedback from Paul, but that may take more time. Please apply, we will
fix eventual problems when they show up.
Thanks,
Wolfgang.
^ permalink raw reply
* Re: [PATCH RFC] gianfar: Do not call skb recycling with disabled IRQs
From: David Miller @ 2009-11-08 9:08 UTC (permalink / raw)
To: avorontsov
Cc: jdl, netdev, linuxppc-dev, afleming, jason.wessel, shemminger,
buytenh
In-Reply-To: <20091105165738.GA31923@oksana.dev.rtsoft.ru>
From: Anton Vorontsov <avorontsov@ru.mvista.com>
Date: Thu, 5 Nov 2009 19:57:38 +0300
> But that basically means that with skb recycling we can't safely
> use KGDBoE, though we can add something like this:
Please stop adding special logic only to your driver to handle these
things.
Either it's a non-issue, or it's going to potentially be an issue for
everyone using skb_recycle_check() in a NAPI driver, right?
So why not add the "in_interrupt()" or whatever check to
skb_recycle_check() and if the context is unsuitable return false (0)
ok?
^ permalink raw reply
* Re: [PATCH RFC] gianfar: Make polling safe with IRQs disabled
From: David Miller @ 2009-11-08 9:05 UTC (permalink / raw)
To: avorontsov; +Cc: linuxppc-dev, netdev, afleming, jason.wessel
In-Reply-To: <20091104225711.GA30844@oksana.dev.rtsoft.ru>
From: Anton Vorontsov <avorontsov@ru.mvista.com>
Date: Thu, 5 Nov 2009 01:57:11 +0300
> When using KGDBoE, gianfar driver spits 'Interrupt problem' messages,
> which appears to be a legitimate warning, i.e. we may end up calling
> netif_receive_skb() or vlan_hwaccel_receive_skb() with IRQs disabled.
>
> This patch reworks the RX path so that if netpoll is enabled (the
> only case when the driver don't know from what context the polling
> may be called), we check whether IRQs are disabled, and if so we
> fall back to safe variants of skb receiving functions.
>
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
This is bogus, I'll tell you why.
When you go into netif_receive_skb() we have a special check,
"if (netpoll_receive_skb(..." that takes care of all of the
details concerning doing a ->poll() from IRQ disabled context
via netpoll.
So this code you're adding should not be necessary.
Or, explain to me why no other driver needs special logic in their
->poll() handler like this and does not run into any kinds of netpoll
problems :-)
^ permalink raw reply
* Re: [PATCH] net: Support specifying the network namespace upon device creation.
From: David Miller @ 2009-11-08 8:54 UTC (permalink / raw)
To: ebiederm; +Cc: netdev
In-Reply-To: <m1eioggeux.fsf@fess.ebiederm.org>
From: ebiederm@xmission.com (Eric W. Biederman)
Date: Tue, 03 Nov 2009 01:03:02 -0800
>
> There is no good reason to not support userspace specifying the
> network namespace during device creation, and it makes it easier
> to create a network device and pass it to a child network namespace
> with a well known name.
>
> We have to be careful to ensure that the target network namespace
> for the new device exists through the life of the call. To keep
> that logic clear I have factored out the network namespace grabbing
> logic into rtnl_link_get_net.
>
> In addtion we need to continue to pass the source network namespace
> to the rtnl_link_ops.newlink method so that we can find the base
> device source network namespace.
>
> Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
Applied to net-next-2.6, thanks!
^ permalink raw reply
* Re: [PATCH v3 net-next-2.6] can: Driver for the Microchip MCP251x SPI CAN controllers
From: David Miller @ 2009-11-08 8:50 UTC (permalink / raw)
To: chripell; +Cc: wg, socketcan-core, spi-devel-general, netdev, pthomas8589
In-Reply-To: <1257239220-11007-1-git-send-email-chripell@fsfe.org>
From: Christian Pellegrin <chripell@fsfe.org>
Date: Tue, 3 Nov 2009 10:07:00 +0100
> Signed-off-by: Christian Pellegrin <chripell@fsfe.org>
Wolfgang, others, any acks on this?
^ permalink raw reply
* Re: [PATCH] net/fsl_pq_mdio: add module license GPL
From: David Miller @ 2009-11-08 8:49 UTC (permalink / raw)
To: bigeasy; +Cc: afleming, netdev
In-Reply-To: <20091106185028.GA5198@www.tglx.de>
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Date: Fri, 6 Nov 2009 19:50:28 +0100
> or it will taint the kernel and fail to load becuase
> of_address_to_resource() is GPL only.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Applied to net-2.6, thanks.
^ permalink raw reply
* Re: [PATCH] can: fix WARN_ON dump in net/core/rtnetlink.c:rtmsg_ifinfo()
From: David Miller @ 2009-11-08 8:46 UTC (permalink / raw)
To: wg; +Cc: netdev, socketcan-core
In-Reply-To: <4AF54389.1090309@grandegger.com>
From: Wolfgang Grandegger <wg@grandegger.com>
Date: Sat, 07 Nov 2009 10:53:13 +0100
> On older kernels, e.g. 2.6.27, a WARN_ON dump in rtmsg_ifinfo()
> is thrown when the CAN device is registered due to insufficient
> skb space, as reported by various users. This patch adds the
> rtnl_link_ops "get_size" to fix the problem. I think this patch
> is required for more recent kernels as well, even if no WARN_ON
> dumps are triggered. Maybe we also need "get_xstats_size" for
> the CAN xstats.
>
> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
Applied to net-2.6, thanks Wolfgang.
^ permalink raw reply
* Re: [PATCH] appletalk/ddp.c: Neaten checksum function
From: David Miller @ 2009-11-08 8:44 UTC (permalink / raw)
To: joe; +Cc: netdev, acme
In-Reply-To: <1257366373.18982.25.camel@Joe-Laptop.home>
From: Joe Perches <joe@perches.com>
Date: Wed, 04 Nov 2009 12:26:13 -0800
> atalk_sum_partial can now use the rol16 function in bitops.h
>
> Signed-off-by: Joe Perches <joe@perches.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH net-next2.6] ipv6: avoid dev_hold()/dev_put() in rawv6_bind()
From: David Miller @ 2009-11-08 8:44 UTC (permalink / raw)
To: eric.dumazet; +Cc: brian.haley, netdev
In-Reply-To: <4AF4565D.2070207@gmail.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 06 Nov 2009 18:01:17 +0100
> [PATCH net-next2.6] ipv6: avoid dev_hold()/dev_put() in rawv6_bind()
>
> Using RCU helps not touching device refcount in rawv6_bind()
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH 1/3] fsl_pq_mdio: Fix compiler/sparse warnings (part 1)
From: David Miller @ 2009-11-08 8:43 UTC (permalink / raw)
To: avorontsov; +Cc: linuxppc-dev, Sandeep.Kumar, afleming, netdev
In-Reply-To: <20091104225256.GA29537@oksana.dev.rtsoft.ru>
From: Anton Vorontsov <avorontsov@ru.mvista.com>
Date: Thu, 5 Nov 2009 01:52:56 +0300
> commit 1d2397d742b7a2b39b2f09dd9da3b9d1463f55e9 ("fsl_pq_mdio: Add
> Suport for etsec2.0 devices") introduced the following warnings:
...
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
Applied.
^ permalink raw reply
* Re: [PATCH 3/3] gianfar: Fix compiler and sparse warnings
From: David Miller @ 2009-11-08 8:44 UTC (permalink / raw)
To: avorontsov; +Cc: afleming, Sandeep.Kumar, netdev, linuxppc-dev
In-Reply-To: <20091104225300.GC29537@oksana.dev.rtsoft.ru>
From: Anton Vorontsov <avorontsov@ru.mvista.com>
Date: Thu, 5 Nov 2009 01:53:00 +0300
> commit fba4ed030cfae7efdb6b79a57b0c5a9d72c9de83 ("gianfar: Add Multiple
> Queue Support") introduced the following warnings:
...
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
Applied.
^ permalink raw reply
* Re: [PATCH 2/3] fsl_pq_mdio: Fix compiler/sparse warnings (part 2)
From: David Miller @ 2009-11-08 8:43 UTC (permalink / raw)
To: avorontsov; +Cc: afleming, Sandeep.Kumar, netdev, linuxppc-dev
In-Reply-To: <20091104225257.GB29537@oksana.dev.rtsoft.ru>
From: Anton Vorontsov <avorontsov@ru.mvista.com>
Date: Thu, 5 Nov 2009 01:52:57 +0300
> This patch fixes following warnings:
>
> fsl_pq_mdio.c:112:38: warning: cast adds address space to expression (<asn:2>)
> fsl_pq_mdio.c:124:38: warning: cast adds address space to expression (<asn:2>)
> fsl_pq_mdio.c:133:38: warning: cast adds address space to expression (<asn:2>)
> fsl_pq_mdio.c:414:11: warning: cast adds address space to expression (<asn:2>)
>
> Instead of adding __force all over the place, introduce convenient
> fsl_pq_mdio_get_regs() call that does the ugly casting just once.
>
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
Applied.
^ permalink raw reply
* Re: [PATCH] can: should not use __dev_get_by_index() without locks
From: David Miller @ 2009-11-08 8:34 UTC (permalink / raw)
To: socketcan; +Cc: eric.dumazet, netdev
In-Reply-To: <4AF402B0.7090202@hartkopp.net>
From: Oliver Hartkopp <socketcan@hartkopp.net>
Date: Fri, 06 Nov 2009 12:04:16 +0100
> Eric Dumazet wrote:
>> [PATCH] can: should not use __dev_get_by_index() without locks
>>
>> bcm_proc_getifname() is called with RTNL and dev_base_lock
>> not held. It calls __dev_get_by_index() without locks, and
>> this is illegal (might crash)
>>
>> Close the race by holding dev_base_lock and copying dev->name
>> in the protected section.
>>
>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
...
> Signed-off-by: Oliver Hartkopp <oliver@hartkopp.net>
Applied, thanks everyone.
^ permalink raw reply
* [PATCH] drivers/net/wireless: correct check on CCS_START_NETWORK
From: Julia Lawall @ 2009-11-08 8:23 UTC (permalink / raw)
To: Corey Thomas, John W. Linville, linux-wireless, netdev,
linux-kernel, kernel-janit
From: Julia Lawall <julia@diku.dk>
CCS_START_NETWORK is declared in drivers/net/wireless/rayctl.h with the
comment Values for cmd. status is previously compared to
CCS_COMMAND_COMPLETE, which is declared in the same file with the comment
Values for buffer_status. Finally, it is possible at this point that cmd
is CCS_START_NETWORK, because it is compared to that value in an enclosing
switch that has CCS_START_NETWORK as one of two case labels around this code.
Signed-off-by: Julia Lawall <julia@diku.dk>
---
drivers/net/wireless/ray_cs.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/net/wireless/ray_cs.c b/drivers/net/wireless/ray_cs.c
index 1c88c2e..5ee9d2a 100644
--- a/drivers/net/wireless/ray_cs.c
+++ b/drivers/net/wireless/ray_cs.c
@@ -2074,7 +2074,7 @@ static irqreturn_t ray_interrupt(int irq, void *dev_id)
del_timer(&local->timer);
local->timer.expires = jiffies + HZ * 5;
local->timer.data = (long)local;
- if (status == CCS_START_NETWORK) {
+ if (cmd == CCS_START_NETWORK) {
DEBUG(0,
"ray_cs interrupt network \"%s\" start failed\n",
local->sparm.b4.a_current_ess_id);
^ permalink raw reply related
* [PATCH iproute2] libnetlink: fix rtnl_send_check()
From: Eric Dumazet @ 2009-11-08 6:18 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: David S. Miller, Linux Netdev List
We currently cannot flush more than ~64 addresses on a device, with
strange status.
# ip -stats addr flush dev eth3
Failed to send flush request: Success
Flush terminated
Problem is in rtnl_send_check(), where a return -1; is misplaced.
After patch, we can flush 64000 addresses without any problem.
# ip -stats addr flush dev eth3
*** Round 1, deleting 32338 addresses ***
*** Round 2, deleting 16168 addresses ***
*** Round 3, deleting 8082 addresses ***
*** Round 4, deleting 4058 addresses ***
*** Round 5, deleting 2028 addresses ***
*** Round 6, deleting 918 addresses ***
*** Round 7, deleting 278 addresses ***
*** Round 8, deleting 130 addresses ***
*** Flush is complete after 8 rounds ***
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
diff --git a/lib/libnetlink.c b/lib/libnetlink.c
index b68e2fd..100dd40 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -137,8 +137,8 @@ int rtnl_send_check(struct rtnl_handle *rth, const char *buf, int len)
fprintf(stderr, "ERROR truncated\n");
else
errno = -err->error;
+ return -1;
}
- return -1;
}
return 0;
^ permalink raw reply related
* Re: RFC: ethtool support for n-tuple filter programming
From: David Miller @ 2009-11-08 4:27 UTC (permalink / raw)
To: peter.p.waskiewicz.jr; +Cc: netdev
In-Reply-To: <1257533841.2610.12.camel@ppwaskie-mobl2>
From: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
Date: Fri, 06 Nov 2009 10:57:21 -0800
> Any comments, thoughts, suggestions, or ideas are welcome.
We can use the existing datastructures and defines used for
ETHTOOL_GRX* for new ethtool commands that do filtering.
NIU can filter on these tuples too.
^ permalink raw reply
* Re: [PATCHv7 3/3] vhost_net: a kernel-level virtio server
From: Rusty Russell @ 2009-11-08 4:09 UTC (permalink / raw)
To: paulmck
Cc: Michael S. Tsirkin, Gregory Haskins, Eric Dumazet, netdev,
virtualization, kvm, linux-kernel, mingo, linux-mm, akpm, hpa,
s.hetze
In-Reply-To: <20091106163007.GC6746@linux.vnet.ibm.com>
On Sat, 7 Nov 2009 03:00:07 am Paul E. McKenney wrote:
> On Fri, Nov 06, 2009 at 03:31:20PM +1030, Rusty Russell wrote:
> > But it's still nasty to use half an API. If it were a few places I would
> > have open-coded it with a comment, or wrapped it. As it is, I don't think
> > that would be a win.
>
> So would it help to have a rcu_read_lock_workqueue() and
> rcu_read_unlock_workqueue() that checked nesting and whether they were
> actually running in the context of a workqueue item? Or did you have
> something else in mind? Or am I misjudging the level of sarcasm in
> your reply? ;-)
You read correctly. If we get a second user, creating an API makes sense.
Thanks,
Rusty.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: TC-HTB issue : low throughput
From: Jarek Poplawski @ 2009-11-07 23:35 UTC (permalink / raw)
To: jt; +Cc: netdev
In-Reply-To: <4AF60349.8000502@gmail.com>
Jarek Poplawski wrote, On 11/08/2009 12:31 AM:
> BTW, v2.6.31 should be more exact (but so much...) for above 100mbit
should be:
BTW, v2.6.31 should be more exact (but _not_ so much...) for above 100mbit
Jarek P.
^ permalink raw reply
* Re: TC-HTB issue : low throughput
From: Jarek Poplawski @ 2009-11-07 23:31 UTC (permalink / raw)
To: jt; +Cc: netdev
In-Reply-To: <20091107024305.GA32398@bougret.hpl.hp.com>
Jean Tourrilhes wrote, On 11/07/2009 03:43 AM:
> Hi,
>
> I'm playing with a TC-HTB. I'm noticing that the throughput is
> low. As the LARTC mailing list seems to be dead, I would welcome
> suggestions...
> The example below may seem contrived, but I reduced it to
> a simple testcase to make it easier to reproduce.
>
> System :
> ------
> Debian Lenny with 2.6.30.9
> Quad Core 2.5 GHz Q9300
> Intel 1Gb/s NIC, e1000e drive
...
> Topology :
> --------
> Four PCs with Linux :
> 10.10.10.32 on br0
> 10.10.10.33 connected to eth5.33 (sender)
> 10.10.10.38 connected to eth7.38 (sender)
> 10.10.10.34 connected to eth6.34 (receiver)
>
> Behaviour :
> ---------
> If I set only qdisc 1: and class 1:1, but *NOT* 1:2 and 1:3 :
> Each host independantly : ~935 Mb/s
> Both host together, for 10.10.10.38 : ~527 Mb/s ;
> Both host together, for 10.10.10.33 : ~443 Mb/s
>
> If I add classes 1:2 and 1:3 :
> Each host independantly : ~170 Mb/s.
> Both host together, for 10.10.10.38 : ~106 Mb/s ;
> Both host together, for 10.10.10.33 : ~135 Mb/s
>
> So, not only performance did drop significantely, but
> prioritisation did not happen as expected.
If these eths are vlans (or other virtuals) something like this
often happens if you forget to set dev's txqueuelen before
adding classes (or a subqdisc with some 'limit').
> Weird detail :
> ------------
> I've noticed that /sbin/tc calculates a very low burst
> value. This is due to the content of /proc/net/psched. I'm wondering
> if the burst calculation is what causes the issue here.
> However, I tried with "burst 50kb" and saw no difference...
There is (probably still) unfixed overflow in tc.
BTW, v2.6.31 should be more exact (but so much...) for above 100mbit
scheduling, especially with this patch to iproute2:
http://marc.info/?l=linux-netdev&m=124453482324409&w=2
Jarek P.
^ permalink raw reply
* Re: RFC: ethtool support for n-tuple filter programming
From: Rick Jones @ 2009-11-07 23:28 UTC (permalink / raw)
To: Bill Fink; +Cc: Peter P Waskiewicz Jr, Caitlin Bestler, netdev@vger.kernel.org
In-Reply-To: <20091107144938.82957125.billfink@mindspring.com>
Bill Fink wrote:
> On Fri, 06 Nov 2009, Peter P Waskiewicz Jr wrote:
>
>
>>On Fri, 2009-11-06 at 11:12 -0800, Caitlin Bestler wrote:
>>
>>>The approach you are proposing assumes what type of packet filters
>>>that L2 hardware could support.
>>>
>>>Why not simply use existing filtering rules that overshoot the target,
>>>such as netfilter, and ask the
>>>device specific tool to indicate what set of these rules it can support?
>>
>>Are you proposing that netfilter is modified to pass the filters down to
>>the hardware if it supports it? netfilter doesn't steer flows though to
>>queues (or flow ID's in the kernel), plus that's putting HW-specific
>>capabilities into netfilter. I'm not sure we want to do that.
>>
>>Please correct me if I'm wrong with interpreting your suggestion.
>
>
> Plus I believe using netfilter has a significant performance penalty,
> and it would be desirable to use such a feature without incurring
> this penalty when there was otherwise no need to use netfilter.
At the risk of typing words into someone's keyboard, I interpreted it as
suggesting using the filtering language of netfilter or something similar, not
necessarily netfilter itself?
rick jones
^ permalink raw reply
* Re: CTP: Ethernet configuration testing protocol
From: Mark Smith @ 2009-11-07 22:23 UTC (permalink / raw)
To: James Courtier-Dutton; +Cc: netdev
In-Reply-To: <4AF5B997.4070601@superbug.co.uk>
On Sat, 07 Nov 2009 18:16:55 +0000
James Courtier-Dutton <James@superbug.co.uk> wrote:
> Hi,
>
> How do I turn off CTP in Linux?
>
As far as I'm aware, it isn't implemented in Linux. I wrote an
implementation a while back, and it wasn't accepted for inclusion at
that time. I'm not aware if that has changed, and I don't think any
other implementations have been included.
> Kind Regards
>
> James
>
>
> --
> 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