* Re: 64-bit net_device_stats
From: Stephen Hemminger @ 2010-06-02 21:59 UTC (permalink / raw)
To: Ben Hutchings; +Cc: David Miller, junchangwang, romieu, netdev
In-Reply-To: <1275514469.2115.70.camel@achroite.uk.solarflarecom.com>
On Wed, 02 Jun 2010 22:34:29 +0100
Ben Hutchings <bhutchings@solarflare.com> wrote:
> Changing the counter types to u64 for 32-bit architectures would remove
> atomicity and expose half-updated counters to userland. Changing the
> driver interface significantly so that atomicity is not needed would
> require changes to hundreds of drivers.
Another big issue is maintaining ABI compatibility for /proc and ioctl
interfaces. So bigger values would only be available through netlink,
and most applications using counters don't use netlink.
--
^ permalink raw reply
* Re: [PATCH 1/8] ath9k: Determine Firmware on probe
From: Luis R. Rodriguez @ 2010-06-02 21:54 UTC (permalink / raw)
To: Sujith, Marcel Holtmann; +Cc: linville, linux-wireless, linux-kernel, netdev
In-Reply-To: <19462.12578.143627.398866@gargle.gargle.HOWL>
On Wed, Jun 2, 2010 at 3:23 AM, Sujith <Sujith.Manoharan@atheros.com> wrote:
> Do not assign the FW name to driver_info but determine
> it dynamically on device probe. This facilitates adding new
> firmware.
>
> Signed-off-by: Sujith <Sujith.Manoharan@atheros.com>
> ---
> drivers/net/wireless/ath/ath9k/hif_usb.c | 39 ++++++++++++++++++++----------
> drivers/net/wireless/ath/ath9k/hif_usb.h | 1 +
> 2 files changed, 27 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c b/drivers/net/wireless/ath/ath9k/hif_usb.c
> index 77b3591..7da55eb 100644
> --- a/drivers/net/wireless/ath/ath9k/hif_usb.c
> +++ b/drivers/net/wireless/ath/ath9k/hif_usb.c
> @@ -16,12 +16,9 @@
>
> #include "htc.h"
>
> -#define ATH9K_FW_USB_DEV(devid, fw) \
> - { USB_DEVICE(0x0cf3, devid), .driver_info = (unsigned long) fw }
> -
> static struct usb_device_id ath9k_hif_usb_ids[] = {
> - ATH9K_FW_USB_DEV(0x9271, "ar9271.fw"),
> - ATH9K_FW_USB_DEV(0x1006, "ar9271.fw"),
> + { USB_DEVICE(0x0cf3, 0x9271) },
> + { USB_DEVICE(0x0cf3, 0x1006) },
> { },
> };
>
> @@ -790,21 +787,21 @@ static int ath9k_hif_usb_download_fw(struct hif_device_usb *hif_dev)
> return -EIO;
>
> dev_info(&hif_dev->udev->dev, "ath9k_htc: Transferred FW: %s, size: %ld\n",
> - "ar9271.fw", (unsigned long) hif_dev->firmware->size);
> + hif_dev->fw_name, (unsigned long) hif_dev->firmware->size);
>
> return 0;
> }
>
> -static int ath9k_hif_usb_dev_init(struct hif_device_usb *hif_dev,
> - const char *fw_name)
> +static int ath9k_hif_usb_dev_init(struct hif_device_usb *hif_dev)
> {
> int ret;
>
> /* Request firmware */
> - ret = request_firmware(&hif_dev->firmware, fw_name, &hif_dev->udev->dev);
> + ret = request_firmware(&hif_dev->firmware, hif_dev->fw_name,
> + &hif_dev->udev->dev);
> if (ret) {
> dev_err(&hif_dev->udev->dev,
> - "ath9k_htc: Firmware - %s not found\n", fw_name);
> + "ath9k_htc: Firmware - %s not found\n", hif_dev->fw_name);
> goto err_fw_req;
> }
>
> @@ -820,7 +817,8 @@ static int ath9k_hif_usb_dev_init(struct hif_device_usb *hif_dev,
> ret = ath9k_hif_usb_download_fw(hif_dev);
> if (ret) {
> dev_err(&hif_dev->udev->dev,
> - "ath9k_htc: Firmware - %s download failed\n", fw_name);
> + "ath9k_htc: Firmware - %s download failed\n",
> + hif_dev->fw_name);
> goto err_fw_download;
> }
>
> @@ -847,7 +845,6 @@ static int ath9k_hif_usb_probe(struct usb_interface *interface,
> {
> struct usb_device *udev = interface_to_usbdev(interface);
> struct hif_device_usb *hif_dev;
> - const char *fw_name = (const char *) id->driver_info;
> int ret = 0;
>
> hif_dev = kzalloc(sizeof(struct hif_device_usb), GFP_KERNEL);
> @@ -872,7 +869,23 @@ static int ath9k_hif_usb_probe(struct usb_interface *interface,
> goto err_htc_hw_alloc;
> }
>
> - ret = ath9k_hif_usb_dev_init(hif_dev, fw_name);
> + /* Find out which firmware to load */
> +
> + switch(hif_dev->device_id) {
> + case 0x9271:
> + case 0x1006:
> + hif_dev->fw_name = "ar9271.fw";
> + break;
> + default:
> + break;
> + }
> +
> + if (!hif_dev->fw_name) {
> + dev_err(&udev->dev, "Can't determine firmware !\n");
> + goto err_htc_hw_alloc;
> + }
> +
> + ret = ath9k_hif_usb_dev_init(hif_dev);
> if (ret) {
> ret = -EINVAL;
> goto err_hif_init_usb;
> diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.h b/drivers/net/wireless/ath/ath9k/hif_usb.h
> index 0aca49b..b2647e8 100644
> --- a/drivers/net/wireless/ath/ath9k/hif_usb.h
> +++ b/drivers/net/wireless/ath/ath9k/hif_usb.h
> @@ -90,6 +90,7 @@ struct hif_device_usb {
> struct usb_anchor regout_submitted;
> struct usb_anchor rx_submitted;
> struct sk_buff *remain_skb;
> + const char *fw_name;
> int rx_remain_len;
> int rx_pkt_len;
> int rx_transfer_len;
I had done some something similar for ar9170 a while back [1] when
adding AVM FRITZ support to ar9170 and then got complaints from Marcel
on this approach since it requires synching the PCI device IDs in two
places. I frankly don't care how we do this but we should try to at
least stick to one way of doing this.
[1] http://osdir.com/ml/linux-wireless/2009-05/msg01219.html
Luis
^ permalink raw reply
* 64-bit net_device_stats
From: Ben Hutchings @ 2010-06-02 21:34 UTC (permalink / raw)
To: David Miller; +Cc: junchangwang, romieu, netdev
In-Reply-To: <20100525.161539.104072714.davem@davemloft.net>
On Tue, 2010-05-25 at 16:15 -0700, David Miller wrote:
[...]
> If the problem is that people want 64-bit counters available for core
> statistics on 32-bit systems, we do not fix that problem by hacking
> every single driver to provide them side-band via ethtool.
>
> First of all, we now have "struct rtnl_link_stats64" in
> linux/if_link.h, it's there to start combating this problem
> generically, for every device, rather than the way you are trying
> handle it only for one specific driver at a time.
>
> So that's the area where you should start looking to solve these kinds
> of problem.
My understand of the current situation is as follows; correct me if any
of this is wrong:
The standard counters in struct net_device_stats have type unsigned long
which is the native word size and so can be read and updated
automatically. Net drivers can update counters from the data path
without any interlocking with their ndo_get_stats implementation or the
networking core code which reads them.
The values returned by ndo_get_stats (by reference) are exposed:
- Through procfs (/proc/net/dev) as columns of numbers
- Through sysfs (/sys/class/net/*/stats/*) as single numeric strings
- Through netlink (IFLA_STATS and IFLA_STATS64) as 32-bit or 64-bit
values in binary structures
Changing the counter types to u64 for 32-bit architectures would remove
atomicity and expose half-updated counters to userland. Changing the
driver interface significantly so that atomicity is not needed would
require changes to hundreds of drivers.
Assuming the above is all correct, I think we can only solve this with a
gradual change (as for net_device_ops). The following might work:
1. a. Define struct net_device_stats64 identically to rtnl_link_stats64.
b. Add net_device_ops::ndo_get_stats64, the implementation of which
will return a pointer to such a structure. The referenced
structure must only be updated atomically, except within the
call to ndo_get_stats64.
(For 64-bit architectures, these could be macro aliases to the
existing structure and operations.)
c. On 32-bit architectures, insert unsigned long padding after each
member of struct net_device_stats.
d. Add an anonymous union in net_device; move stats into the union
and add struct net_device_stats64 stats64.
e. Change dev_get_stats() to return a pointer to struct
net_device_stats64, and to call ndo_get_stats64 if available in
preference to ndo_get_stats. Adjust callers accordingly.
f. Either change dev_get_stats() to clear the padding members, or
change drivers to ensure that the padding is cleared.
2. Change drivers to define ndo_get_stats64, following the rule
stated in 1b.
3. Much later, remove net_device_stats.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH UPDATED 1/3] vhost: replace vhost_workqueue with per-vhost kthread
From: Sridhar Samudrala @ 2010-06-02 21:34 UTC (permalink / raw)
To: Tejun Heo
Cc: Michael S. Tsirkin, Oleg Nesterov, netdev, lkml,
kvm@vger.kernel.org, Andrew Morton, Dmitri Vorobiev, Jiri Kosina,
Thomas Gleixner, Ingo Molnar, Andi Kleen
In-Reply-To: <4C06A580.9060300@kernel.org>
On 6/2/2010 11:40 AM, Tejun Heo wrote:
> Replace vhost_workqueue with per-vhost kthread. Other than callback
> argument change from struct work_struct * to struct vhost_work *,
> there's no visible change to vhost_poll_*() interface.
>
> This conversion is to make each vhost use a dedicated kthread so that
> resource control via cgroup can be applied.
>
> Partially based on Sridhar Samudrala's patch.
>
> * Updated to use sub structure vhost_work instead of directly using
> vhost_poll at Michael's suggestion.
>
> * Added flusher wake_up() optimization at Michael's suggestion.
>
> Signed-off-by: Tejun Heo<tj@kernel.org>
> Cc: Michael S. Tsirkin<mst@redhat.com>
> Cc: Sridhar Samudrala<samudrala.sridhar@gmail.com>
> ---
> Okay, just tested it. dev->work_lock had to be updated to use irq
> operations but other than that it worked just fine. Copied a large
> file using scp and it seems to perform pretty well although I don't
> have any reference of comparison. So, here's the updated version with
> the sign off.
>
I tested this with 4 VMs running netperf TCP stream tests from guest to
host and i am seeing similar
level of scalability in throughput i saw with the multi-thread workqueue
patch.
11200Mb/s - default (host cpu utilization: 40%)
21600Mb/s - multi-thread (host cpu utilization: 86%)
Signed-off-by: Sridhar Samudrala <sri@us.ibm.com>
Thanks
Sridhar
> Thanks.
>
> drivers/vhost/net.c | 56 ++++++++++---------------
> drivers/vhost/vhost.c | 111 ++++++++++++++++++++++++++++++++++++++------------
> drivers/vhost/vhost.h | 38 +++++++++++------
> 3 files changed, 134 insertions(+), 71 deletions(-)
>
> Index: work/drivers/vhost/net.c
> ===================================================================
> --- work.orig/drivers/vhost/net.c
> +++ work/drivers/vhost/net.c
> @@ -294,54 +294,58 @@ static void handle_rx(struct vhost_net *
> unuse_mm(net->dev.mm);
> }
>
> -static void handle_tx_kick(struct work_struct *work)
> +static void handle_tx_kick(struct vhost_work *work)
> {
> - struct vhost_virtqueue *vq;
> - struct vhost_net *net;
> - vq = container_of(work, struct vhost_virtqueue, poll.work);
> - net = container_of(vq->dev, struct vhost_net, dev);
> + struct vhost_virtqueue *vq = container_of(work, struct vhost_virtqueue,
> + poll.work);
> + struct vhost_net *net = container_of(vq->dev, struct vhost_net, dev);
> +
> handle_tx(net);
> }
>
> -static void handle_rx_kick(struct work_struct *work)
> +static void handle_rx_kick(struct vhost_work *work)
> {
> - struct vhost_virtqueue *vq;
> - struct vhost_net *net;
> - vq = container_of(work, struct vhost_virtqueue, poll.work);
> - net = container_of(vq->dev, struct vhost_net, dev);
> + struct vhost_virtqueue *vq = container_of(work, struct vhost_virtqueue,
> + poll.work);
> + struct vhost_net *net = container_of(vq->dev, struct vhost_net, dev);
> +
> handle_rx(net);
> }
>
> -static void handle_tx_net(struct work_struct *work)
> +static void handle_tx_net(struct vhost_work *work)
> {
> - struct vhost_net *net;
> - net = container_of(work, struct vhost_net, poll[VHOST_NET_VQ_TX].work);
> + struct vhost_net *net = container_of(work, struct vhost_net,
> + poll[VHOST_NET_VQ_TX].work);
> handle_tx(net);
> }
>
> -static void handle_rx_net(struct work_struct *work)
> +static void handle_rx_net(struct vhost_work *work)
> {
> - struct vhost_net *net;
> - net = container_of(work, struct vhost_net, poll[VHOST_NET_VQ_RX].work);
> + struct vhost_net *net = container_of(work, struct vhost_net,
> + poll[VHOST_NET_VQ_RX].work);
> handle_rx(net);
> }
>
> static int vhost_net_open(struct inode *inode, struct file *f)
> {
> struct vhost_net *n = kmalloc(sizeof *n, GFP_KERNEL);
> + struct vhost_dev *dev;
> int r;
> +
> if (!n)
> return -ENOMEM;
> +
> + dev =&n->dev;
> n->vqs[VHOST_NET_VQ_TX].handle_kick = handle_tx_kick;
> n->vqs[VHOST_NET_VQ_RX].handle_kick = handle_rx_kick;
> - r = vhost_dev_init(&n->dev, n->vqs, VHOST_NET_VQ_MAX);
> + r = vhost_dev_init(dev, n->vqs, VHOST_NET_VQ_MAX);
> if (r< 0) {
> kfree(n);
> return r;
> }
>
> - vhost_poll_init(n->poll + VHOST_NET_VQ_TX, handle_tx_net, POLLOUT);
> - vhost_poll_init(n->poll + VHOST_NET_VQ_RX, handle_rx_net, POLLIN);
> + vhost_poll_init(n->poll + VHOST_NET_VQ_TX, handle_tx_net, POLLOUT, dev);
> + vhost_poll_init(n->poll + VHOST_NET_VQ_RX, handle_rx_net, POLLIN, dev);
> n->tx_poll_state = VHOST_NET_POLL_DISABLED;
>
> f->private_data = n;
> @@ -644,25 +648,13 @@ static struct miscdevice vhost_net_misc
>
> static int vhost_net_init(void)
> {
> - int r = vhost_init();
> - if (r)
> - goto err_init;
> - r = misc_register(&vhost_net_misc);
> - if (r)
> - goto err_reg;
> - return 0;
> -err_reg:
> - vhost_cleanup();
> -err_init:
> - return r;
> -
> + return misc_register(&vhost_net_misc);
> }
> module_init(vhost_net_init);
>
> static void vhost_net_exit(void)
> {
> misc_deregister(&vhost_net_misc);
> - vhost_cleanup();
> }
> module_exit(vhost_net_exit);
>
> Index: work/drivers/vhost/vhost.c
> ===================================================================
> --- work.orig/drivers/vhost/vhost.c
> +++ work/drivers/vhost/vhost.c
> @@ -17,12 +17,12 @@
> #include<linux/mm.h>
> #include<linux/miscdevice.h>
> #include<linux/mutex.h>
> -#include<linux/workqueue.h>
> #include<linux/rcupdate.h>
> #include<linux/poll.h>
> #include<linux/file.h>
> #include<linux/highmem.h>
> #include<linux/slab.h>
> +#include<linux/kthread.h>
>
> #include<linux/net.h>
> #include<linux/if_packet.h>
> @@ -37,8 +37,6 @@ enum {
> VHOST_MEMORY_F_LOG = 0x1,
> };
>
> -static struct workqueue_struct *vhost_workqueue;
> -
> static void vhost_poll_func(struct file *file, wait_queue_head_t *wqh,
> poll_table *pt)
> {
> @@ -52,23 +50,31 @@ static void vhost_poll_func(struct file
> static int vhost_poll_wakeup(wait_queue_t *wait, unsigned mode, int sync,
> void *key)
> {
> - struct vhost_poll *poll;
> - poll = container_of(wait, struct vhost_poll, wait);
> + struct vhost_poll *poll = container_of(wait, struct vhost_poll, wait);
> +
> if (!((unsigned long)key& poll->mask))
> return 0;
>
> - queue_work(vhost_workqueue,&poll->work);
> + vhost_poll_queue(poll);
> return 0;
> }
>
> /* Init poll structure */
> -void vhost_poll_init(struct vhost_poll *poll, work_func_t func,
> - unsigned long mask)
> +void vhost_poll_init(struct vhost_poll *poll, vhost_work_fn_t fn,
> + unsigned long mask, struct vhost_dev *dev)
> {
> - INIT_WORK(&poll->work, func);
> + struct vhost_work *work =&poll->work;
> +
> init_waitqueue_func_entry(&poll->wait, vhost_poll_wakeup);
> init_poll_funcptr(&poll->table, vhost_poll_func);
> poll->mask = mask;
> + poll->dev = dev;
> +
> + INIT_LIST_HEAD(&work->node);
> + work->fn = fn;
> + init_waitqueue_head(&work->done);
> + atomic_set(&work->flushing, 0);
> + work->queue_seq = work->done_seq = 0;
> }
>
> /* Start polling a file. We add ourselves to file's wait queue. The caller must
> @@ -92,12 +98,29 @@ void vhost_poll_stop(struct vhost_poll *
> * locks that are also used by the callback. */
> void vhost_poll_flush(struct vhost_poll *poll)
> {
> - flush_work(&poll->work);
> + struct vhost_work *work =&poll->work;
> + int seq = work->queue_seq;
> +
> + atomic_inc(&work->flushing);
> + smp_mb__after_atomic_inc(); /* mb flush-b0 paired with worker-b1 */
> + wait_event(work->done, seq - work->done_seq<= 0);
> + atomic_dec(&work->flushing);
> + smp_mb__after_atomic_dec(); /* rmb flush-b1 paired with worker-b0 */
> }
>
> void vhost_poll_queue(struct vhost_poll *poll)
> {
> - queue_work(vhost_workqueue,&poll->work);
> + struct vhost_dev *dev = poll->dev;
> + struct vhost_work *work =&poll->work;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&dev->work_lock, flags);
> + if (list_empty(&work->node)) {
> + list_add_tail(&work->node,&dev->work_list);
> + work->queue_seq++;
> + wake_up_process(dev->worker);
> + }
> + spin_unlock_irqrestore(&dev->work_lock, flags);
> }
>
> static void vhost_vq_reset(struct vhost_dev *dev,
> @@ -125,10 +148,52 @@ static void vhost_vq_reset(struct vhost_
> vq->log_ctx = NULL;
> }
>
> +static int vhost_worker(void *data)
> +{
> + struct vhost_dev *dev = data;
> + struct vhost_work *work;
> +
> +repeat:
> + set_current_state(TASK_INTERRUPTIBLE); /* mb paired w/ kthread_stop */
> +
> + if (kthread_should_stop()) {
> + __set_current_state(TASK_RUNNING);
> + return 0;
> + }
> +
> + work = NULL;
> + spin_lock_irq(&dev->work_lock);
> + if (!list_empty(&dev->work_list)) {
> + work = list_first_entry(&dev->work_list,
> + struct vhost_work, node);
> + list_del_init(&work->node);
> + }
> + spin_unlock_irq(&dev->work_lock);
> +
> + if (work) {
> + __set_current_state(TASK_RUNNING);
> + work->fn(work);
> + smp_wmb(); /* wmb worker-b0 paired with flush-b1 */
> + work->done_seq = work->queue_seq;
> + smp_mb(); /* mb worker-b1 paired with flush-b0 */
> + if (atomic_read(&work->flushing))
> + wake_up_all(&work->done);
> + } else
> + schedule();
> +
> + goto repeat;
> +}
> +
> long vhost_dev_init(struct vhost_dev *dev,
> struct vhost_virtqueue *vqs, int nvqs)
> {
> + struct task_struct *worker;
> int i;
> +
> + worker = kthread_create(vhost_worker, dev, "vhost-%d", current->pid);
> + if (IS_ERR(worker))
> + return PTR_ERR(worker);
> +
> dev->vqs = vqs;
> dev->nvqs = nvqs;
> mutex_init(&dev->mutex);
> @@ -136,6 +201,9 @@ long vhost_dev_init(struct vhost_dev *de
> dev->log_file = NULL;
> dev->memory = NULL;
> dev->mm = NULL;
> + spin_lock_init(&dev->work_lock);
> + INIT_LIST_HEAD(&dev->work_list);
> + dev->worker = worker;
>
> for (i = 0; i< dev->nvqs; ++i) {
> dev->vqs[i].dev = dev;
> @@ -143,9 +211,10 @@ long vhost_dev_init(struct vhost_dev *de
> vhost_vq_reset(dev, dev->vqs + i);
> if (dev->vqs[i].handle_kick)
> vhost_poll_init(&dev->vqs[i].poll,
> - dev->vqs[i].handle_kick,
> - POLLIN);
> + dev->vqs[i].handle_kick, POLLIN, dev);
> }
> +
> + wake_up_process(worker); /* avoid contributing to loadavg */
> return 0;
> }
>
> @@ -217,6 +286,9 @@ void vhost_dev_cleanup(struct vhost_dev
> if (dev->mm)
> mmput(dev->mm);
> dev->mm = NULL;
> +
> + WARN_ON(!list_empty(&dev->work_list));
> + kthread_stop(dev->worker);
> }
>
> static int log_access_ok(void __user *log_base, u64 addr, unsigned long sz)
> @@ -1113,16 +1185,3 @@ void vhost_disable_notify(struct vhost_v
> vq_err(vq, "Failed to enable notification at %p: %d\n",
> &vq->used->flags, r);
> }
> -
> -int vhost_init(void)
> -{
> - vhost_workqueue = create_singlethread_workqueue("vhost");
> - if (!vhost_workqueue)
> - return -ENOMEM;
> - return 0;
> -}
> -
> -void vhost_cleanup(void)
> -{
> - destroy_workqueue(vhost_workqueue);
> -}
> Index: work/drivers/vhost/vhost.h
> ===================================================================
> --- work.orig/drivers/vhost/vhost.h
> +++ work/drivers/vhost/vhost.h
> @@ -5,13 +5,13 @@
> #include<linux/vhost.h>
> #include<linux/mm.h>
> #include<linux/mutex.h>
> -#include<linux/workqueue.h>
> #include<linux/poll.h>
> #include<linux/file.h>
> #include<linux/skbuff.h>
> #include<linux/uio.h>
> #include<linux/virtio_config.h>
> #include<linux/virtio_ring.h>
> +#include<asm/atomic.h>
>
> struct vhost_device;
>
> @@ -20,19 +20,31 @@ enum {
> VHOST_NET_MAX_SG = MAX_SKB_FRAGS + 2,
> };
>
> +struct vhost_work;
> +typedef void (*vhost_work_fn_t)(struct vhost_work *work);
> +
> +struct vhost_work {
> + struct list_head node;
> + vhost_work_fn_t fn;
> + wait_queue_head_t done;
> + atomic_t flushing;
> + int queue_seq;
> + int done_seq;
> +};
> +
> /* Poll a file (eventfd or socket) */
> /* Note: there's nothing vhost specific about this structure. */
> struct vhost_poll {
> poll_table table;
> wait_queue_head_t *wqh;
> wait_queue_t wait;
> - /* struct which will handle all actual work. */
> - struct work_struct work;
> + struct vhost_work work;
> unsigned long mask;
> + struct vhost_dev *dev;
> };
>
> -void vhost_poll_init(struct vhost_poll *poll, work_func_t func,
> - unsigned long mask);
> +void vhost_poll_init(struct vhost_poll *poll, vhost_work_fn_t fn,
> + unsigned long mask, struct vhost_dev *dev);
> void vhost_poll_start(struct vhost_poll *poll, struct file *file);
> void vhost_poll_stop(struct vhost_poll *poll);
> void vhost_poll_flush(struct vhost_poll *poll);
> @@ -63,7 +75,7 @@ struct vhost_virtqueue {
> struct vhost_poll poll;
>
> /* The routine to call when the Guest pings us, or timeout. */
> - work_func_t handle_kick;
> + vhost_work_fn_t handle_kick;
>
> /* Last available index we saw. */
> u16 last_avail_idx;
> @@ -86,11 +98,11 @@ struct vhost_virtqueue {
> struct iovec hdr[VHOST_NET_MAX_SG];
> size_t hdr_size;
> /* We use a kind of RCU to access private pointer.
> - * All readers access it from workqueue, which makes it possible to
> - * flush the workqueue instead of synchronize_rcu. Therefore readers do
> + * All readers access it from worker, which makes it possible to
> + * flush the vhost_work instead of synchronize_rcu. Therefore readers do
> * not need to call rcu_read_lock/rcu_read_unlock: the beginning of
> - * work item execution acts instead of rcu_read_lock() and the end of
> - * work item execution acts instead of rcu_read_lock().
> + * vhost_work execution acts instead of rcu_read_lock() and the end of
> + * vhost_work execution acts instead of rcu_read_lock().
> * Writers use virtqueue mutex. */
> void *private_data;
> /* Log write descriptors */
> @@ -110,6 +122,9 @@ struct vhost_dev {
> int nvqs;
> struct file *log_file;
> struct eventfd_ctx *log_ctx;
> + spinlock_t work_lock;
> + struct list_head work_list;
> + struct task_struct *worker;
> };
>
> long vhost_dev_init(struct vhost_dev *, struct vhost_virtqueue *vqs, int nvqs);
> @@ -136,9 +151,6 @@ bool vhost_enable_notify(struct vhost_vi
> int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
> unsigned int log_num, u64 len);
>
> -int vhost_init(void);
> -void vhost_cleanup(void);
> -
> #define vq_err(vq, fmt, ...) do { \
> pr_debug(pr_fmt(fmt), ##__VA_ARGS__); \
> if ((vq)->error_ctx) \
>
^ permalink raw reply
* Re: [PATCH v2] netfilter: Xtables: idletimer target implementation
From: Luciano Coelho @ 2010-06-02 21:01 UTC (permalink / raw)
To: ext Jan Engelhardt
Cc: netdev@vger.kernel.org, netfilter-devel@vger.kernel.org,
kaber@trash.net, Timo Teras
In-Reply-To: <1275509088.2797.29.camel@powerslave>
On Wed, 2010-06-02 at 22:04 +0200, Coelho Luciano (Nokia-D/Helsinki)
wrote:
> What causes printk to appear under /sys/module even when compiled in, is
> that it uses a module param. This line:
>
> module_param_named(time, printk_time, bool, S_IRUGO | S_IWUSR);
>
> ...is what triggers the printk directory to be created in sysfs. If I
> add a similar line in my module, it shows up there too.
>
> I still don't know if there is an actual kobject associated with it,
> I'll check that next.
Okay, so here is how it goes: if the module is linked into the kernel
and it has module parameters, the kernel creates a kobj for it as a
module_ktype without parent, which will cause it to show up
in /sys/modules.
I could do the same in the module initialization when THIS_MODULE ==
NULL, but I don't see any other module doing this. In fact, I only see
the kernel itself creating kobjects of module_ktype (in load_module()
and in the case I just described). Smells like a terrible hack to do
that in the module itself... :(
Adding bogus parameters to the module just to trig the kernel to create
the kobject also seems to be too hacky...
--
Cheers,
Luca.
^ permalink raw reply
* Re: [PATCH net-next-2.6] net: replace hooks in __netif_receive_skb V5
From: Paul E. McKenney @ 2010-06-02 20:43 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Stephen Hemminger, Jiri Pirko, netdev, davem, kaber
In-Reply-To: <1275491722.2725.184.camel@edumazet-laptop>
On Wed, Jun 02, 2010 at 05:15:22PM +0200, Eric Dumazet wrote:
> Le mercredi 02 juin 2010 à 08:07 -0700, Stephen Hemminger a écrit :
> > On Wed, 2 Jun 2010 09:52:08 +0200
> > Jiri Pirko <jpirko@redhat.com> wrote:
> >
> > > +
> > > + err = netdev_rx_handler_register(dev, macvlan_handle_frame);
> > > + if (err) {
> > > + rcu_assign_pointer(dev->macvlan_port, NULL);
> > > + kfree(port);
> > > + }
> > > +
> > > + return err;
> > > }
> >
> > Rcu assign is not necessary here for because the hook didn't
> > get registered so there is no way for other CPU to see it.
> >
>
> Thats a valid point, but we should use it, and not care of this litle
> detail. Compiler generates same code anyway, since NULL value is tested
> by rcu_assign_pointer().
>
> If we dont use rcu_assign_pointer() ourself, Paul or Arnd will put it
> one day or another :)
>
> http://lkml.org/lkml/2010/6/1/290
Some year soon, I hope. ;-)
Thanx, Paul
^ permalink raw reply
* [PATCH] sfc: Store port number in net_device::dev_id
From: Ben Hutchings @ 2010-06-02 20:39 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-net-drivers
This exposes the port number to userland through sysfs.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
Following on from the earlier discussion of dev_id, here's the necessary
change for sfc. This depends on commit dd8f61d "sfc: Get port number
from CS_PORT_NUM, not PCI function number". Please consider including
both of these in net-2.6.
Ben.
drivers/net/sfc/net_driver.h | 4 +---
drivers/net/sfc/siena.c | 2 +-
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/net/sfc/net_driver.h b/drivers/net/sfc/net_driver.h
index 6b2e440..a8783b4 100644
--- a/drivers/net/sfc/net_driver.h
+++ b/drivers/net/sfc/net_driver.h
@@ -649,7 +649,6 @@ union efx_multicast_hash {
* struct efx_nic - an Efx NIC
* @name: Device name (net device name or bus id before net device registered)
* @pci_dev: The PCI device
- * @port_num: Index of this host port within the controller
* @type: Controller type attributes
* @legacy_irq: IRQ number
* @workqueue: Workqueue for port reconfigures and the HW monitor.
@@ -733,7 +732,6 @@ union efx_multicast_hash {
struct efx_nic {
char name[IFNAMSIZ];
struct pci_dev *pci_dev;
- unsigned port_num;
const struct efx_nic_type *type;
int legacy_irq;
struct workqueue_struct *workqueue;
@@ -836,7 +834,7 @@ static inline const char *efx_dev_name(struct efx_nic *efx)
static inline unsigned int efx_port_num(struct efx_nic *efx)
{
- return efx->port_num;
+ return efx->net_dev->dev_id;
}
/**
diff --git a/drivers/net/sfc/siena.c b/drivers/net/sfc/siena.c
index 7ecd255..f2b1e61 100644
--- a/drivers/net/sfc/siena.c
+++ b/drivers/net/sfc/siena.c
@@ -222,7 +222,7 @@ static int siena_probe_nic(struct efx_nic *efx)
}
efx_reado(efx, ®, FR_AZ_CS_DEBUG);
- efx->port_num = EFX_OWORD_FIELD(reg, FRF_CZ_CS_PORT_NUM) - 1;
+ efx->net_dev->dev_id = EFX_OWORD_FIELD(reg, FRF_CZ_CS_PORT_NUM) - 1;
efx_mcdi_init(efx);
--
1.6.2.5
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply related
* [PATCH] epic100: Test __BIG_ENDIAN instead of (non-existent) CONFIG_BIG_ENDIAN
From: Roland Dreier @ 2010-06-02 20:36 UTC (permalink / raw)
To: David S. Miller, netdev
Probably no one has used this driver on big-endian systems, since it was
setting up descriptor swapping if CONFIG_BIG_ENDIAN is set, which it
never is, since that symbol is not mentioned anywhere else in the kernel
source. Switch this test to a check for __BIG_ENDIAN so it has a chance
at working.
Signed-off-by: Roland Dreier <rolandd@cisco.com>
---
drivers/net/epic100.c | 7 ++++---
1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/net/epic100.c b/drivers/net/epic100.c
index 6838dfc..4c27465 100644
--- a/drivers/net/epic100.c
+++ b/drivers/net/epic100.c
@@ -87,6 +87,7 @@ static int rx_copybreak;
#include <linux/bitops.h>
#include <asm/io.h>
#include <asm/uaccess.h>
+#include <asm/byteorder.h>
/* These identify the driver base version and may not be removed. */
static char version[] __devinitdata =
@@ -230,7 +231,7 @@ static const u16 media2miictl[16] = {
* The EPIC100 Rx and Tx buffer descriptors. Note that these
* really ARE host-endian; it's not a misannotation. We tell
* the card to byteswap them internally on big-endian hosts -
- * look for #ifdef CONFIG_BIG_ENDIAN in epic_open().
+ * look for #ifdef __BIG_ENDIAN in epic_open().
*/
struct epic_tx_desc {
@@ -690,7 +691,7 @@ static int epic_open(struct net_device *dev)
outl((inl(ioaddr + NVCTL) & ~0x003C) | 0x4800, ioaddr + NVCTL);
/* Tell the chip to byteswap descriptors on big-endian hosts */
-#ifdef CONFIG_BIG_ENDIAN
+#ifdef __BIG_ENDIAN
outl(0x4432 | (RX_FIFO_THRESH<<8), ioaddr + GENCTL);
inl(ioaddr + GENCTL);
outl(0x0432 | (RX_FIFO_THRESH<<8), ioaddr + GENCTL);
@@ -806,7 +807,7 @@ static void epic_restart(struct net_device *dev)
for (i = 16; i > 0; i--)
outl(0x0008, ioaddr + TEST1);
-#ifdef CONFIG_BIG_ENDIAN
+#ifdef __BIG_ENDIAN
outl(0x0432 | (RX_FIFO_THRESH<<8), ioaddr + GENCTL);
#else
outl(0x0412 | (RX_FIFO_THRESH<<8), ioaddr + GENCTL);
--
Roland Dreier <rolandd@cisco.com> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html
^ permalink raw reply related
* Re: [PATCH] IP: Increment INADDRERRORS if routing for a packet is not successful
From: Christoph Lameter @ 2010-06-02 20:11 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, shemminger
In-Reply-To: <1275506732.2519.23.camel@edumazet-laptop>
On Wed, 2 Jun 2010, Eric Dumazet wrote:
>
> Say we have eth0 on 192.168.0.1/24 and eth1 on 192.168.0.2/24
>
> Then we cannot use rp_filter = 1, even with unicast trafic.
>
> I really dont understand why you would setup rp_filter in such a
> situation. This wont work.
rp_filter was setup in the past and it worked. Stephen fixed it in 2.6.31
for multicast and thus suddenly multicast stopped working on secondary
interfaces when we moved to 2.6.32. rp_filter having to be off is okay but
it does not feel correct.
> Now, I agree we should have a counter somewhere to help admins to
> understand their error ;)
Ah. Good.
> Here is patch I am currently testing.
>
> I finaly created a new counter, because its a linux specific check.
Looks good which does not say too much given my limited networking
knowledge.
Reviewed-by: Christoph Lameter <cl@linux-foundation.org>
^ permalink raw reply
* Re: [PATCH v2] netfilter: Xtables: idletimer target implementation
From: Luciano Coelho @ 2010-06-02 20:04 UTC (permalink / raw)
To: ext Jan Engelhardt
Cc: netdev@vger.kernel.org, netfilter-devel@vger.kernel.org,
kaber@trash.net, Timo Teras
In-Reply-To: <1275508348.2797.26.camel@powerslave>
On Wed, 2010-06-02 at 21:52 +0200, Coelho Luciano (Nokia-D/Helsinki)
wrote:
> On Wed, 2010-06-02 at 21:29 +0200, ext Jan Engelhardt wrote:
> > On Wednesday 2010-06-02 21:05, Luciano Coelho wrote:
> > >> > >+ idletimer_tg_kobj = kobject_create_and_add("idletimer",
> > >> > >+ &THIS_MODULE->mkobj.kobj);
> > >> >
> > >> > Isn't this going to oops when you compile this module as =y?
> > >>
> > >> Damn, that's true. :(
> > >>
> > >> I'll investigate how to fix this.
> > >
> > >Would it be too hacky to force it to be a module (ie. add "depends on m"
> > >in Kconfig)?
> > >
> > >Besides /sys/module/xt_IDLETIMER and /sys/class/net, which we have
> > >already discarded, I can't find any other place that would make sense to
> > >add the idletimer in the kernel object hierarchy...
> >
> > While THIS_MODULE is NULL in =y mode, /sys/module/<xyz> can still exist
> > (cf. /sys/module/printk). I just don't know how to get at the kobj for
> > it, but the existence of it must mean it's there somewhere. Might ask
> > sysfs authors.
>
> Okay, good to know. My initial theory was that /sys/module/xt_IDLETIMER
> (pointed to by THIS_MODULE) would exist even if it was linked into the
> kernel itself, but now it's obvious that it doesn't.
>
> I'll investigate how printk does that.
What causes printk to appear under /sys/module even when compiled in, is
that it uses a module param. This line:
module_param_named(time, printk_time, bool, S_IRUGO | S_IWUSR);
...is what triggers the printk directory to be created in sysfs. If I
add a similar line in my module, it shows up there too.
I still don't know if there is an actual kobject associated with it,
I'll check that next.
--
Cheers,
Luca.
^ permalink raw reply
* Re: [PATCH 3/3] net: deliver skbs on inactive slaves to exact matches
From: Jay Vosburgh @ 2010-06-02 20:01 UTC (permalink / raw)
To: David Miller; +Cc: john.r.fastabend, andy, nhorman, bonding-devel, netdev
In-Reply-To: <20100602.033623.84369970.davem@davemloft.net>
David Miller <davem@davemloft.net> wrote:
>From: John Fastabend <john.r.fastabend@intel.com>
>Date: Wed, 26 May 2010 21:52:58 -0700
>
>> I know you were looking over this series at one point. Did you have
>> any comments? Sorry for the ping just wanted to keep this on my
>> radar.
>
>I'll hold this last one until Jay has a chance to comment on it.
I've looked at it, and was initially hoping to combine this with
Andy/Neil's vaguely similar changes, but I don't see a reasonable way to
do that.
I think the functionality is reasonable, i.e., adding a facility
to implement "direct bind" delivery of packets on inactive bonding
slaves (where direct bind means that the struct packet_type has a
non-NULL dev).
I have a couple of concerns about the patch itself:
>From: John Fastabend <john.r.fastabend@intel.com>
>Subject: [PATCH 3/3] net: deliver skbs on inactive slaves to exact matches
>To: andy@greyhouse.net, fubar@us.ibm.com, nhorman@tuxdriver.com,
> bonding-devel@lists.sourceforge.net, netdev@vger.kernel.org
>Cc: john.r.fastabend@intel.com
>Date: Thu, 13 May 2010 00:31:17 -0700
>
>Currently, the accelerated receive path for VLAN's will
>drop packets if the real device is an inactive slave and
>is not one of the special pkts tested for in
>skb_bond_should_drop(). This behavior is different then
>the non-accelerated path and for pkts over a bonded vlan.
>
>For example,
>
>vlanx -> bond0 -> ethx
>
>will be dropped in the vlan path and not delivered to any
>packet handlers at all. However,
>
>bond0 -> vlanx -> ethx
>
>and
>
>bond0 -> ethx
>
>will be delivered to handlers that match the exact dev,
>because the VLAN path checks the real_dev which is not a
>slave and netif_recv_skb() doesn't drop frames but only
>delivers them to exact matches.
>
>This patch adds a sk_buff flag which is used for tagging
>skbs that would previously been dropped and allows the
>skb to continue to skb_netif_recv(). Here we add
>logic to check for the bond_should_drop flag and if it
>is set only deliver to handlers that match exactly. This
>makes both paths above consistent and gives pkt handlers
>a way to identify skbs that come from inactive slaves.
>Without this patch in some configurations skbs will be
>delivered to handlers with exact matches and in others
>be dropped out right in the vlan path.
>
>I have tested the following 4 configurations in failover modes
>and load balancing modes.
>
># bond0 -> ethx
>
># vlanx -> bond0 -> ethx
>
># bond0 -> vlanx -> ethx
>
># bond0 -> ethx
> |
> vlanx -> --
>
>Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
>---
>
> include/linux/skbuff.h | 8 +++++++-
> net/8021q/vlan_core.c | 8 ++++++--
> net/core/dev.c | 23 +++++++++++++++++++----
> 3 files changed, 32 insertions(+), 7 deletions(-)
>
>diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>index c9525bc..5ba4fd5 100644
>--- a/include/linux/skbuff.h
>+++ b/include/linux/skbuff.h
>@@ -379,8 +379,14 @@ struct sk_buff {
>
> kmemcheck_bitfield_begin(flags2);
> __u16 queue_mapping:16;
>-#ifdef CONFIG_IPV6_NDISC_NODETYPE
>+#if defined(CONFIG_IPV6_NDISC_NODETYPE) && \
>+ (defined(CONFIG_BONDING) || defined(CONFIG_BONDING_MODULE))
>+ __u8 ndisc_nodetype:2,
>+ bond_should_drop:1;
>+#elif defined(CONFIG_IPV6_NDISC_NODETYPE)
> __u8 ndisc_nodetype:2;
>+#elif defined(CONFIG_BONDING) || defined(CONFIG_BONDING_MODULE)
>+ __u8 bond_should_drop:1;
> #endif
First, this looks like too much #ifdef soup here. None of the
bonding-specific code in the packet receive processing path has
historically been #ifdefed out; there are more examples further down in
the patch.
Second, should the field be named something generic, e.g.,
"deliver_no_wcard" to indicate its function? "bond_should_drop" doesn't
really describe what the field is used for (which is to specify that the
packet should be delivered only to non-wildcard packet handlers). With
this change, packets are never dropped outright as the result of what
the bonding "should drop" logic says.
I'm open to alternatives to using a field in the sk_buff; John's
original submission added a PACKET_DROP (to supplant PACKET_LOCAL,
PACKET_BROADCAST, etc), but that seemed like a loss of information to
me. I haven't thought of a way to efficiently accomplish John's goal
without putting state into the skb somewhere.
-J
> kmemcheck_bitfield_end(flags2);
>
>diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
>index c584a0a..57ac2d3 100644
>--- a/net/8021q/vlan_core.c
>+++ b/net/8021q/vlan_core.c
>@@ -11,8 +11,10 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp,
> if (netpoll_rx(skb))
> return NET_RX_DROP;
>
>+#if defined(CONFIG_BONDING) || defined(CONFIG_BONDING_MODULE)
> if (skb_bond_should_drop(skb, ACCESS_ONCE(skb->dev->master)))
>- goto drop;
>+ skb->bond_should_drop = 1;
>+#endif
> skb->skb_iif = skb->dev->ifindex;
> __vlan_hwaccel_put_tag(skb, vlan_tci);
>@@ -83,8 +85,10 @@ vlan_gro_common(struct napi_struct *napi, struct vlan_group *grp,
> {
> struct sk_buff *p;
>
>+#if defined(CONFIG_BONDING) || defined(CONFIG_BONDING_MODULE)
> if (skb_bond_should_drop(skb, ACCESS_ONCE(skb->dev->master)))
>- goto drop;
>+ skb->bond_should_drop = 1;
>+#endif
>
> skb->skb_iif = skb->dev->ifindex;
> __vlan_hwaccel_put_tag(skb, vlan_tci);
>diff --git a/net/core/dev.c b/net/core/dev.c
>index 3dc691d..92fdff4 100644
>--- a/net/core/dev.c
>+++ b/net/core/dev.c
>@@ -2782,11 +2782,13 @@ static int __netif_receive_skb(struct sk_buff *skb)
> {
> struct packet_type *ptype, *pt_prev;
> struct net_device *orig_dev;
>- struct net_device *master;
> struct net_device *null_or_orig;
> struct net_device *orig_or_bond;
> int ret = NET_RX_DROP;
> __be16 type;
>+#if defined(CONFIG_BONDING) || defined(CONFIG_BONDING_MODULE)
>+ struct net_device *master;
>+#endif
>
> if (!skb->tstamp.tv64)
> net_timestamp(skb);
>@@ -2801,15 +2803,28 @@ static int __netif_receive_skb(struct sk_buff *skb)
> if (!skb->skb_iif)
> skb->skb_iif = skb->dev->ifindex;
>
>+ /*
>+ * bonding note: skbs received on inactive slaves should only
>+ * be delivered to pkt handlers that are exact matches. Also
>+ * the bond_should_drop flag will be set. If packet handlers
>+ * are sensitive to duplicate packets these skbs will need to
>+ * be dropped at the handler. The vlan accel path may have
>+ * already set the bond_should_drop flag.
>+ */
> null_or_orig = NULL;
> orig_dev = skb->dev;
>+#if defined(CONFIG_BONDING) || defined(CONFIG_BONDING_MODULE)
> master = ACCESS_ONCE(orig_dev->master);
>- if (master) {
>- if (skb_bond_should_drop(skb, master))
>+ if (skb->bond_should_drop)
>+ null_or_orig = orig_dev;
>+ else if (master) {
>+ if (skb_bond_should_drop(skb, master)) {
>+ skb->bond_should_drop = 1;
> null_or_orig = orig_dev; /* deliver only exact match */
>- else
>+ } else
> skb->dev = master;
> }
>+#endif
>
> __get_cpu_var(softnet_data).processed++;
>
>
---
-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
^ permalink raw reply
* Re: [PATCH v2] netfilter: Xtables: idletimer target implementation
From: Luciano Coelho @ 2010-06-02 19:52 UTC (permalink / raw)
To: ext Jan Engelhardt
Cc: netdev@vger.kernel.org, netfilter-devel@vger.kernel.org,
kaber@trash.net, Timo Teras
In-Reply-To: <alpine.LSU.2.01.1006022127430.32028@obet.zrqbmnf.qr>
On Wed, 2010-06-02 at 21:29 +0200, ext Jan Engelhardt wrote:
> On Wednesday 2010-06-02 21:05, Luciano Coelho wrote:
> >> > >+ idletimer_tg_kobj = kobject_create_and_add("idletimer",
> >> > >+ &THIS_MODULE->mkobj.kobj);
> >> >
> >> > Isn't this going to oops when you compile this module as =y?
> >>
> >> Damn, that's true. :(
> >>
> >> I'll investigate how to fix this.
> >
> >Would it be too hacky to force it to be a module (ie. add "depends on m"
> >in Kconfig)?
> >
> >Besides /sys/module/xt_IDLETIMER and /sys/class/net, which we have
> >already discarded, I can't find any other place that would make sense to
> >add the idletimer in the kernel object hierarchy...
>
> While THIS_MODULE is NULL in =y mode, /sys/module/<xyz> can still exist
> (cf. /sys/module/printk). I just don't know how to get at the kobj for
> it, but the existence of it must mean it's there somewhere. Might ask
> sysfs authors.
Okay, good to know. My initial theory was that /sys/module/xt_IDLETIMER
(pointed to by THIS_MODULE) would exist even if it was linked into the
kernel itself, but now it's obvious that it doesn't.
I'll investigate how printk does that.
--
Cheers,
Luca.
^ permalink raw reply
* Re: [PATCH] phylib: Add support for the LXT973 phy.
From: Andy Fleming @ 2010-06-02 19:32 UTC (permalink / raw)
To: David Miller; +Cc: richardcochran, netdev
In-Reply-To: <20100602.065017.139108801.davem@davemloft.net>
On Wed, Jun 2, 2010 at 8:50 AM, David Miller <davem@davemloft.net> wrote:
> From: Richard Cochran <richardcochran@gmail.com>
> Date: Wed, 2 Jun 2010 14:55:27 +0200
>
>> On Tue, Jun 01, 2010 at 05:39:22PM -0500, Andy Fleming wrote:
>>> That's a bit hacky. There is a dev_flags field, which could be used
>>> for this. Probably, we should add a more general way of saying what
>>> sort of port this is. But don't use the presence and absence of
>>> "priv", as it could one day get used for a different purpose, and this
>>> seems like it would leave us open to strange bugs.
>>
>> Okay, I changed it.
>>
>> At first, I was worried about using 'dev_flags' because I couldn't
>> tell exactly who may write to this field. Looking at tg.c and
>> broadcom.c, it appears that the MAC drivers may also write this
>> field. In contrast, the 'priv' field is surely private.
>
> No, I think using dev_flags is absolutely the wrong way to do about
> this.
Yeah, I was clearly not thinking clearly. dev_flags will be
overwritten, and is not meant for this. I believe, what we should do
is add a "port" field to the PHY device, and if PCR_FIBER_SELECT is
set, then set the port field to PORT_FIBRE. I'm not entirely clear on
the semantics of that field in the ethtool cmd, but it seems like the
right idea.
>
> phy_device->priv "could one day get used for a different purpose"?
> What in the world are you smoking Andy?
>
> It's clearly a private state pointer for the PHY driver to use,
> full stop. There is absolutely no ambiguity of what this value
> is and what it is used for and who owns it. The comments in the
> layout of struct phy_device state this clearly as well.
Yes, it's private, and I would be fine with using priv, I just didn't
think that it was correct to assign priv to point at the probe
function as a mechanism for indicating that fiber is being used. I
believe that code should be more explicit than that. priv is meant to
point at private data, not to indicate an arbitrary attribute of the
link by virtue of being non-null.
Andy
^ permalink raw reply
* Re: [PATCH v2] netfilter: Xtables: idletimer target implementation
From: Jan Engelhardt @ 2010-06-02 19:29 UTC (permalink / raw)
To: Luciano Coelho
Cc: netdev@vger.kernel.org, netfilter-devel@vger.kernel.org,
kaber@trash.net, Timo Teras
In-Reply-To: <1275505553.2797.2.camel@powerslave>
On Wednesday 2010-06-02 21:05, Luciano Coelho wrote:
>> > >+ idletimer_tg_kobj = kobject_create_and_add("idletimer",
>> > >+ &THIS_MODULE->mkobj.kobj);
>> >
>> > Isn't this going to oops when you compile this module as =y?
>>
>> Damn, that's true. :(
>>
>> I'll investigate how to fix this.
>
>Would it be too hacky to force it to be a module (ie. add "depends on m"
>in Kconfig)?
>
>Besides /sys/module/xt_IDLETIMER and /sys/class/net, which we have
>already discarded, I can't find any other place that would make sense to
>add the idletimer in the kernel object hierarchy...
While THIS_MODULE is NULL in =y mode, /sys/module/<xyz> can still exist
(cf. /sys/module/printk). I just don't know how to get at the kobj for
it, but the existence of it must mean it's there somewhere. Might ask
sysfs authors.
^ permalink raw reply
* [PATCH] fec: convert legacy PM hooks to dem_pm_ops
From: Denis Kirjanov @ 2010-06-02 19:27 UTC (permalink / raw)
To: davem; +Cc: s.hauer, eric, gerg, linux-arm-kernel, netdev
This patch compile tested only.
Convert legacy PM hooks to dev_pm_ops
Signed-off-by: Denis Kirjanov <dkirjanov@kernel.org>
---
drivers/net/fec.c | 24 +++++++++++++++++++++---
1 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/drivers/net/fec.c b/drivers/net/fec.c
index 42d9ac9..6990791 100644
--- a/drivers/net/fec.c
+++ b/drivers/net/fec.c
@@ -1339,6 +1339,8 @@ fec_drv_remove(struct platform_device *pdev)
return 0;
}
+#ifdef CONFIG_PM
+
static int
fec_suspend(struct platform_device *dev, pm_message_t state)
{
@@ -1369,15 +1371,31 @@ fec_resume(struct platform_device *dev)
return 0;
}
+static const struct dev_pm_ops fec_pm_ops = {
+ .suspend = fec_suspend,
+ .resume = fec_resume,
+ .freeze = fec_suspend,
+ .thaw = fec_resume,
+ .poweroff = fec_suspend,
+ .restore = fec_resume,
+};
+
+#define FEC_PM_OPS (&fec_pm_ops)
+
+#else /* !CONFIG_PM */
+
+#define FEC_PM_OPS NULL
+
+#endif /* !CONFIG_PM */
+
static struct platform_driver fec_driver = {
.driver = {
.name = "fec",
.owner = THIS_MODULE,
+ .pm = FEC_PM_OPS,
},
.probe = fec_probe,
.remove = __devexit_p(fec_drv_remove),
- .suspend = fec_suspend,
- .resume = fec_resume,
};
static int __init
^ permalink raw reply related
* Re: [PATCH] IP: Increment INADDRERRORS if routing for a packet is not successful
From: Eric Dumazet @ 2010-06-02 19:25 UTC (permalink / raw)
To: Christoph Lameter; +Cc: David Miller, netdev, shemminger
In-Reply-To: <alpine.DEB.2.00.1006021355040.32431@router.home>
Le mercredi 02 juin 2010 à 13:59 -0500, Christoph Lameter a écrit :
> The rp_filter is rejecting traffic coming into a NIC for which the kernel
> has a multicast join list that indicates that this traffic is expected on
> this NIC. You could consult the MC subscription list to verify that the
> traffic is coming into the right NIC.
>
> In the MC case the user can explicitly specify through which NIC the
> traffic is expected. See IP_ADD_MEMBERSHIP.
This has litle to do with MC. We certainly are not going to check MC
membership in fib_validate_source() !
Say we have eth0 on 192.168.0.1/24 and eth1 on 192.168.0.2/24
Then we cannot use rp_filter = 1, even with unicast trafic.
I really dont understand why you would setup rp_filter in such a
situation. This wont work.
Now, I agree we should have a counter somewhere to help admins to
understand their error ;)
Here is patch I am currently testing.
I finaly created a new counter, because its a linux specific check.
diff --git a/include/linux/snmp.h b/include/linux/snmp.h
index 5279771..ebb0c80 100644
--- a/include/linux/snmp.h
+++ b/include/linux/snmp.h
@@ -229,6 +229,7 @@ enum
LINUX_MIB_TCPBACKLOGDROP,
LINUX_MIB_TCPMINTTLDROP, /* RFC 5082 */
LINUX_MIB_TCPDEFERACCEPTDROP,
+ LINUX_MIB_IPRPFILTER, /* IP Reverse Path Filter (rp_filter) */
__LINUX_MIB_MAX
};
diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
index 4f0ed45..e830f7a 100644
--- a/net/ipv4/fib_frontend.c
+++ b/net/ipv4/fib_frontend.c
@@ -284,7 +284,7 @@ int fib_validate_source(__be32 src, __be32 dst, u8 tos, int oif,
if (no_addr)
goto last_resort;
if (rpf == 1)
- goto e_inval;
+ goto e_rpf;
fl.oif = dev->ifindex;
ret = 0;
@@ -299,7 +299,7 @@ int fib_validate_source(__be32 src, __be32 dst, u8 tos, int oif,
last_resort:
if (rpf)
- goto e_inval;
+ goto e_rpf;
*spec_dst = inet_select_addr(dev, 0, RT_SCOPE_UNIVERSE);
*itag = 0;
return 0;
@@ -308,6 +308,8 @@ e_inval_res:
fib_res_put(&res);
e_inval:
return -EINVAL;
+e_rpf:
+ return -EXDEV;
}
static inline __be32 sk_extract_addr(struct sockaddr *addr)
diff --git a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c
index d930dc5..d52c9da 100644
--- a/net/ipv4/ip_input.c
+++ b/net/ipv4/ip_input.c
@@ -340,6 +340,9 @@ static int ip_rcv_finish(struct sk_buff *skb)
else if (err == -ENETUNREACH)
IP_INC_STATS_BH(dev_net(skb->dev),
IPSTATS_MIB_INNOROUTES);
+ else if (err == -EXDEV)
+ NET_INC_STATS_BH(dev_net(skb->dev),
+ LINUX_MIB_IPRPFILTER);
goto drop;
}
}
diff --git a/net/ipv4/proc.c b/net/ipv4/proc.c
index 3dc9914..e320ca6 100644
--- a/net/ipv4/proc.c
+++ b/net/ipv4/proc.c
@@ -252,6 +252,7 @@ static const struct snmp_mib snmp4_net_list[] = {
SNMP_MIB_ITEM("TCPBacklogDrop", LINUX_MIB_TCPBACKLOGDROP),
SNMP_MIB_ITEM("TCPMinTTLDrop", LINUX_MIB_TCPMINTTLDROP),
SNMP_MIB_ITEM("TCPDeferAcceptDrop", LINUX_MIB_TCPDEFERACCEPTDROP),
+ SNMP_MIB_ITEM("IPReversePathFilter", LINUX_MIB_IPRPFILTER),
SNMP_MIB_SENTINEL
};
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 8495bce..3a264f7 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1851,6 +1851,7 @@ static int ip_route_input_mc(struct sk_buff *skb, __be32 daddr, __be32 saddr,
__be32 spec_dst;
struct in_device *in_dev = in_dev_get(dev);
u32 itag = 0;
+ int err;
/* Primary sanity checks. */
@@ -1865,10 +1866,12 @@ static int ip_route_input_mc(struct sk_buff *skb, __be32 daddr, __be32 saddr,
if (!ipv4_is_local_multicast(daddr))
goto e_inval;
spec_dst = inet_select_addr(dev, 0, RT_SCOPE_LINK);
- } else if (fib_validate_source(saddr, 0, tos, 0,
- dev, &spec_dst, &itag, 0) < 0)
- goto e_inval;
-
+ } else {
+ err = fib_validate_source(saddr, 0, tos, 0, dev, &spec_dst,
+ &itag, 0);
+ if (err < 0)
+ goto e_err;
+ }
rth = dst_alloc(&ipv4_dst_ops);
if (!rth)
goto e_nobufs;
@@ -1922,6 +1925,9 @@ e_nobufs:
e_inval:
in_dev_put(in_dev);
return -EINVAL;
+e_err:
+ in_dev_put(in_dev);
+ return err;
}
@@ -1985,7 +1991,6 @@ static int __mkroute_input(struct sk_buff *skb,
ip_handle_martian_source(in_dev->dev, in_dev, skb, daddr,
saddr);
- err = -EINVAL;
goto cleanup;
}
@@ -2191,7 +2196,7 @@ brd_input:
err = fib_validate_source(saddr, 0, tos, 0, dev, &spec_dst,
&itag, skb->mark);
if (err < 0)
- goto martian_source;
+ goto martian_source_keep_err;
if (err)
flags |= RTCF_DIRECTSRC;
}
@@ -2272,8 +2277,10 @@ e_nobufs:
goto done;
martian_source:
+ err = -EINVAL;
+martian_source_keep_err:
ip_handle_martian_source(dev, in_dev, skb, daddr, saddr);
- goto e_inval;
+ goto done;
}
int ip_route_input_common(struct sk_buff *skb, __be32 daddr, __be32 saddr,
^ permalink raw reply related
* Re: sysfs class/net/ problem
From: Johannes Berg @ 2010-06-02 19:25 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: Greg KH, netdev
In-Reply-To: <m1y6exux6x.fsf@fess.ebiederm.org>
On Wed, 2010-06-02 at 11:05 -0700, Eric W. Biederman wrote:
> My current hypothesis is something is causing us to try and delete
> the symlink from the wrong namespace, so we just skip that part of it.
Hmm... ok:
[ 70.338274] create link wlan2 ns=(null)
...
[ 71.881775] delete link wlan2 ns=(null)
[ 71.881777] hash_and_remove ffff88001f9563c0, (null), wlan2
[ 71.881782] sd=ffff88001ce2d9c0, sdns=ffffffff8271c260
and thus we skip sysfs_remove_one() in sysfs_hash_and_remove() because
sysfs_find_dirent() return an sd with a different ns than we passed in.
Why is the ns we pass in NULL, shouldn't it be init_ns?
johannes
^ permalink raw reply
* [PATCH 2/2] fec: Cleanup PHY probing
From: Denis Kirjanov @ 2010-06-02 19:17 UTC (permalink / raw)
To: davem; +Cc: netdev
Cleanup PHY probing: use helpers from phylib
Signed-off-by: Denis Kirjanov <dkirjanov@kernel.org>
---
drivers/net/fec.c | 16 +++++-----------
1 files changed, 5 insertions(+), 11 deletions(-)
diff --git a/drivers/net/fec.c b/drivers/net/fec.c
index edfff92..c107d8e 100644
--- a/drivers/net/fec.c
+++ b/drivers/net/fec.c
@@ -679,30 +679,24 @@ static int fec_enet_mii_probe(struct net_device *dev)
{
struct fec_enet_private *fep = netdev_priv(dev);
struct phy_device *phy_dev = NULL;
- int phy_addr;
+ int ret;
fep->phy_dev = NULL;
/* find the first phy */
- for (phy_addr = 0; phy_addr < PHY_MAX_ADDR; phy_addr++) {
- if (fep->mii_bus->phy_map[phy_addr]) {
- phy_dev = fep->mii_bus->phy_map[phy_addr];
- break;
- }
- }
-
+ phy_dev = phy_find_first(fep->mii_bus);
if (!phy_dev) {
printk(KERN_ERR "%s: no PHY found\n", dev->name);
return -ENODEV;
}
/* attach the mac to the phy */
- phy_dev = phy_connect(dev, dev_name(&phy_dev->dev),
+ ret = phy_connect_direct(dev, phy_dev,
&fec_enet_adjust_link, 0,
PHY_INTERFACE_MODE_MII);
- if (IS_ERR(phy_dev)) {
+ if (ret) {
printk(KERN_ERR "%s: Could not attach to PHY\n", dev->name);
- return PTR_ERR(phy_dev);
+ return ret;
}
/* mask with MAC supported features */
^ permalink raw reply related
* [PATCH 1/2] fec: convert TX hook to netdev_tx_t
From: Denis Kirjanov @ 2010-06-02 19:15 UTC (permalink / raw)
To: davem; +Cc: netdev
Convert TX hook return value to netdev_tx_t
Signed-off-by: Denis Kirjanov <dkirjanov@kernel.org>
---
drivers/net/fec.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/net/fec.c b/drivers/net/fec.c
index 42d9ac9..64afed0 100644
--- a/drivers/net/fec.c
+++ b/drivers/net/fec.c
@@ -208,7 +208,7 @@ static void fec_stop(struct net_device *dev);
/* Transmitter timeout */
#define TX_TIMEOUT (2 * HZ)
-static int
+static netdev_tx_t
fec_enet_start_xmit(struct sk_buff *skb, struct net_device *dev)
{
struct fec_enet_private *fep = netdev_priv(dev);
^ permalink raw reply related
* Re: sysfs class/net/ problem
From: Johannes Berg @ 2010-06-02 19:12 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: Greg KH, netdev
In-Reply-To: <1275504953.3915.31.camel@jlt3.sipsolutions.net>
On Wed, 2010-06-02 at 20:55 +0200, Johannes Berg wrote:
> On Wed, 2010-06-02 at 11:05 -0700, Eric W. Biederman wrote:
>
> > If you want to dig into this look at sysfs_delete_link. instrument
> > it so that you can see if it is called for wlan{0,1,2} and see what
> > ns it is called for.
> >
> > My current hypothesis is something is causing us to try and delete
> > the symlink from the wrong namespace, so we just skip that part of it.
>
> [ 78.253128] create link wlan0 ns=ffff88001ce1e600
> ...
> [ 93.462268] delete link wlan0 ns=ffff88001ce1e600
No that was the sd, but sd->s_ns is NULL in both cases.
johannes
^ permalink raw reply
* Re: [PATCH v2] netfilter: Xtables: idletimer target implementation
From: Luciano Coelho @ 2010-06-02 19:05 UTC (permalink / raw)
To: ext Jan Engelhardt
Cc: netdev@vger.kernel.org, netfilter-devel@vger.kernel.org,
kaber@trash.net, Timo Teras
In-Reply-To: <1275503835.1574.0.camel@powerslave>
On Wed, 2010-06-02 at 20:37 +0200, Coelho Luciano (Nokia-D/Helsinki)
wrote:
> On Wed, 2010-06-02 at 17:16 +0200, ext Jan Engelhardt wrote:
> > On Wednesday 2010-06-02 15:41, Luciano Coelho wrote:
> >
> > >+static int __init idletimer_tg_init(void)
> > >+{
> > >+ int ret;
> > >+
> > >+ idletimer_tg_kobj = kobject_create_and_add("idletimer",
> > >+ &THIS_MODULE->mkobj.kobj);
> >
> > Isn't this going to oops when you compile this module as =y?
>
> Damn, that's true. :(
>
> I'll investigate how to fix this.
Would it be too hacky to force it to be a module (ie. add "depends on m"
in Kconfig)?
Besides /sys/module/xt_IDLETIMER and /sys/class/net, which we have
already discarded, I can't find any other place that would make sense to
add the idletimer in the kernel object hierarchy...
--
Cheers,
Luca.
^ permalink raw reply
* Re: [PATCH] IP: Increment INADDRERRORS if routing for a packet is not successful
From: Christoph Lameter @ 2010-06-02 18:59 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, shemminger
In-Reply-To: <1275504070.2519.12.camel@edumazet-laptop>
On Wed, 2 Jun 2010, Eric Dumazet wrote:
> > In my particular case it is a weird corner case for the rp_filter.
> >
> > Two NICs are on the same subnet. Different multicast groups are joined
> > on each (Using two NICs to balance the MC load since the drivers have
> > some multicast limitations and having different interrupt lines for each
> > NIC is also beneficial).
> >
>
> yeah, I know about this problem, and am working on it too...
>
> > The rp_filter rejects all multicast traffic to the subscriptions on the
> > second NIC. I guess this is because the source address of the MC traffic
> > (on the same subnet) is also reachable via the first NIC.
> >
>
> Its clearly a case were rp_filter should be set to 2, dont you think ?
The rp_filter is rejecting traffic coming into a NIC for which the kernel
has a multicast join list that indicates that this traffic is expected on
this NIC. You could consult the MC subscription list to verify that the
traffic is coming into the right NIC.
In the MC case the user can explicitly specify through which NIC the
traffic is expected. See IP_ADD_MEMBERSHIP.
^ permalink raw reply
* Re: [PATCH] ipconfig: send host-name in DHCP requests
From: Andi Kleen @ 2010-06-02 18:56 UTC (permalink / raw)
To: David Miller; +Cc: fengguang.wu, netdev, linux-kernel, andi
In-Reply-To: <20100602.070531.73368509.davem@davemloft.net>
On Wed, Jun 02, 2010 at 07:05:31AM -0700, David Miller wrote:
> From: Wu Fengguang <fengguang.wu@intel.com>
> Date: Mon, 31 May 2010 11:19:53 +0800
>
> > Normally dhclient can be configured to send the "host-name" option
> > in DHCP requests to update the client's DNS record. However for an
> > NFSROOT system, dhclient shall never be called (which may change the
> > IP addr and therefore lose your root NFS mount connection).
> >
> > So enable updating the DNS record with kernel parameter
> >
> > ip=::::$HOST_NAME::dhcp
> >
> > Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
>
> Applied, thanks.
Small nit: Fengguang, please document the new option in
Documentation/kernel-parameters.txt That could be done in a follow-on patch.
-Andi
>
--
ak@linux.intel.com -- Speaking for myself only.
^ permalink raw reply
* Re: sysfs class/net/ problem
From: Johannes Berg @ 2010-06-02 18:55 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: Greg KH, netdev
In-Reply-To: <m1y6exux6x.fsf@fess.ebiederm.org>
On Wed, 2010-06-02 at 11:05 -0700, Eric W. Biederman wrote:
> If you want to dig into this look at sysfs_delete_link. instrument
> it so that you can see if it is called for wlan{0,1,2} and see what
> ns it is called for.
>
> My current hypothesis is something is causing us to try and delete
> the symlink from the wrong namespace, so we just skip that part of it.
[ 78.253128] create link wlan0 ns=ffff88001ce1e600
...
[ 93.462268] delete link wlan0 ns=ffff88001ce1e600
looks the same ...
Also note
[ 109.872488] netconsole: network logging stopped, interface wlan0 unregistered
[ 109.872910] PM: Removing info for No Bus:wlan0
[ 109.872941] delete link wlan0 ns=ffff88001e9bd600
[ 110.130563] PM: Removing info for No Bus:rfkill0
[ 110.130599] delete link rfkill0 ns=ffff88001b61ea80
[ 110.131135] PM: Removing info for No Bus:phy0
[ 110.131161] delete link phy0 ns=ffff88001b61e240
[ 110.131424] PM: Removing info for No Bus:hwsimdev0
[ 110.131445] delete link hwsimdev0 ns=ffff88001b67ed80
(I changed the struct device thing in hwsim to be hwsimdev%d rather than
hwsim%d to tell the difference to hwsim0, the monitor netdev) so it's
getting removed from PM way after the wlan0 that links into it...
johannes
^ permalink raw reply
* Re: [PATCH] IP: Increment INADDRERRORS if routing for a packet is not successful
From: Eric Dumazet @ 2010-06-02 18:41 UTC (permalink / raw)
To: Christoph Lameter; +Cc: David Miller, netdev, shemminger
In-Reply-To: <alpine.DEB.2.00.1006021255001.32431@router.home>
Le mercredi 02 juin 2010 à 13:01 -0500, Christoph Lameter a écrit :
> On Wed, 2 Jun 2010, Eric Dumazet wrote:
>
> > Here is the patch I cooked to account for RP_FILTER errors in multicast
> > path.
> >
> > I will complete it to also do the unicast part before official
> > submission.
> >
> > Christoph, the official counter would be IPSTATS_MIB_INNOROUTES
>
> Great. Thanks.
>
> > ipSystemStatsInNoRoutes OBJECT-TYPE
> > SYNTAX Counter32
> > MAX-ACCESS read-only
> > STATUS current
> > DESCRIPTION
> > "The number of input IP datagrams discarded because no route
> > could be found to transmit them to their destination.
>
> add "or because the rp_filter rejected the packet"? In the case of MC
> traffic you dont really need a route.
>
Unicast trafic dont need a reverse route, if you only receive packets.
rp_filter is an optional check, not covered by standard MIBS, so its
borderline.
> In my particular case it is a weird corner case for the rp_filter.
>
> Two NICs are on the same subnet. Different multicast groups are joined
> on each (Using two NICs to balance the MC load since the drivers have
> some multicast limitations and having different interrupt lines for each
> NIC is also beneficial).
>
yeah, I know about this problem, and am working on it too...
> The rp_filter rejects all multicast traffic to the subscriptions on the
> second NIC. I guess this is because the source address of the MC traffic
> (on the same subnet) is also reachable via the first NIC.
>
Its clearly a case were rp_filter should be set to 2, dont you think ?
> So you could add also "because of breakage in the rp_filter (rp_filter
> ignores the multicast subscription tables when determining the correct
> reverse path of the packet)"
>
In standard RFC ? I wont change it :)
^ 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