* Re: Kernel oops on setting sky2 interfaces down
From: Rene Mayrhofer @ 2009-08-19 21:25 UTC (permalink / raw)
To: Mike McCormack; +Cc: netdev, Richard Leitner, Stephen Hemminger
In-Reply-To: <200908192307.22112.rene.mayrhofer@gibraltar.at>
Am Mittwoch, 19. August 2009 23:07:21 schrieb Rene Mayrhofer:
> In this state, rmmod sky2 / modprobe
> sky2 gives:
>
> [ 718.502717] sky2 0000:01:00.0: unsupported chip type 0xff
> [ 718.510517] sky2: probe of 0000:01:00.0 failed with error -95
> [ 718.517900] sky2 0000:02:00.0: unsupported chip type 0xff
> [ 718.524408] sky2: probe of 0000:02:00.0 failed with error -95
> [ 718.531617] sky2 0000:03:00.0: unsupported chip type 0xff
> [ 718.538104] sky2: probe of 0000:03:00.0 failed with error -95
> [ 718.545344] sky2 0000:04:00.0: unsupported chip type 0xff
> [ 718.551818] sky2: probe of 0000:04:00.0 failed with error -95
Just for the sake of completeness, I get this during bootup (i.e. before
networking restart):
[ 1.637607] sky2 driver version 1.24
[ 1.637665] sky2 0000:01:00.0: PCI INT A -> GSI 16 (level, low) -> IRQ
16iled with error -95 3:00.0: unsupported chi[ 1.637686]
sky2 0000:01:00.0: setting latency timer to 64rror -95
[ 1.637718] sky2 0000:01:00.0: Yukon-2 EC chip revision 2 0xff
[ 1.637832] sky2 0000:01:00.0: irq 28 for MSI/MSI-X
[ 1.638395] sky2 eth0: addr 00:90:0b:09:55:42
[ 1.638418] sky2 0000:02:00.0: PCI INT A -> GSI 17 (level, low) -> IRQ 17
[ 1.638431] sky2 0000:02:00.0: setting latency timer to 64
[ 1.638458] sky2 0000:02:00.0: Yukon-2 EC chip revision 2
[ 1.638567] sky2 0000:02:00.0: irq 29 for MSI/MSI-X
[ 1.639117] sky2 eth1: addr 00:90:0b:09:55:43
[ 1.639140] sky2 0000:03:00.0: PCI INT A -> GSI 18 (level, low) -> IRQ 18
[ 1.639152] sky2 0000:03:00.0: setting latency timer to 64
[ 1.639180] sky2 0000:03:00.0: Yukon-2 EC chip revision 2
[ 1.639289] sky2 0000:03:00.0: irq 30 for MSI/MSI-X
[ 1.639869] sky2 eth2: addr 00:90:0b:09:55:44
[ 1.639894] sky2 0000:04:00.0: PCI INT A -> GSI 19 (level, low) -> IRQ 19
[ 1.639906] sky2 0000:04:00.0: setting latency timer to 64
[ 1.639934] sky2 0000:04:00.0: Yukon-2 EC chip revision 2
[ 1.680096] sky2 0000:04:00.0: irq 31 for MSI/MSI-X
[ 1.680786] sky2 eth3: addr 00:90:0b:09:55:45
[ 106.751517] sky2 wan: enabling interface
[ 107.404801] sky2 gibsrv: enabling interface
[ 107.631583] sky2 dmz: enabling interface
[ 107.869878] sky2 lan: enabling interface
[ 109.225106] sky2 wan: Link is up at 1000 Mbps, full duplex, flow control
both
[ 109.232519] sky2 gibsrv: Link is up at 100 Mbps, full duplex, flow control
both
[ 109.983861] sky2 dmz: Link is up at 1000 Mbps, full duplex, flow control rx
[ 110.453276] sky2 lan: Link is up at 1000 Mbps, full duplex, flow control rx
Then, at some point the interfaces get disabled (most probably by the
networking restart):
[ 224.652146] sky2 lan: disabling interface
[ 224.684483] sky2 0000:04:00.0: PCI INT A disabled
[ 224.700164] sky2 dmz: disabling interface
[ 224.736502] sky2 0000:03:00.0: PCI INT A disabled
[ 224.760219] sky2 gibsrv: disabling interface
[ 224.812491] sky2 0000:02:00.0: PCI INT A disabled
[ 224.832221] sky2 wan: disabling interface
[ 224.904479] sky2 0000:01:00.0: PCI INT A disabled
best regards,
Rene
--
-------------------------------------------------
Gibraltar firewall http://www.gibraltar.at/
^ permalink raw reply
* Re: [PATCH] ibm_newemac: emac_close() needs to call netif_carrier_off()
From: Stephen Hemminger @ 2009-08-19 23:48 UTC (permalink / raw)
To: Petri Gynther; +Cc: David Miller, benh, netdev
In-Reply-To: <fc21faff0908191532i2de570a1g1a6674bf5fe64a05@mail.gmail.com>
On Wed, 19 Aug 2009 15:32:41 -0700
Petri Gynther <pgynther@google.com> wrote:
> Stephen,
>
> I think your suggestion of adding netif_running() check to
> bond_check_dev_link() is valid and a good fix to the bonding driver.
> We can do this in a separate patch.
>
> However, I think that the change to ibm_newemac: emac_close() is
> needed as well. ibm_newemac netdevs should not return
> netif_carrier_ok() == TRUE when they have been shut down.
I concur. Fixing the possible problems in both places is best.
^ permalink raw reply
* Re: configfs/sysfs
From: Nicholas A. Bellinger @ 2009-08-19 23:54 UTC (permalink / raw)
To: Joel Becker
Cc: Avi Kivity, Ingo Molnar, Anthony Liguori, kvm, alacrityvm-devel,
linux-kernel, netdev, Michael S. Tsirkin, Ira W. Snyder
In-Reply-To: <20090819221654.GA29503@mail.oracle.com>
On Wed, 2009-08-19 at 15:16 -0700, Joel Becker wrote:
> On Wed, Aug 19, 2009 at 11:12:43PM +0300, Avi Kivity wrote:
> > On 08/19/2009 09:23 PM, Nicholas A. Bellinger wrote:
> > >Anyways, I was wondering if you might be interesting in sharing your
> > >concerns wrt to configfs (conigfs maintainer CC'ed), at some point..?
> >
> > My concerns aren't specifically with configfs, but with all the text
> > based pseudo filesystems that the kernel exposes.
>
> Phew! It's not just me :-)
>
> > My high level concern is that we're optimizing for the active
> > sysadmin, not for libraries and management programs. configfs and
> > sysfs are easy to use from the shell, discoverable, and easily
> > scripted. But they discourage documentation, the text format is
> > ambiguous, and they require a lot of boilerplate to use in code.
>
> I don't think they "discourage documentation" anymore than any
> ioctl we've ever had. At least you can look at the names and values and
> take a good stab at it (configfs is better than sysfs at this, by virtue
> of what it does, but discoverability is certainly not as good as real
> documentation).
> With an ioctl() that isn't (well) documented, you have to go
> read the structure and probably even read the code that uses the
> structure to be sure what you are doing.
>
Good point..
>
> > You could argue that you can wrap *fs in a library that hides the
> > details of accessing it, but that's the wrong approach IMO. We
> > should make the information easy to use and manipulate for programs;
> > one of these programs can be a fuse filesystem for the active
> > sysadmin if someone thinks it's important.
>
> You are absolutely correct that they are a boon to the sysadmin,
> where in theory programs can do better with binary interfaces. Except
> what programs? I can't do an ioctl or a syscall from a shell script
> (no, using bash's network capabilities to talk to netlink does not
> count). Same with perl/python/whatever where you have to write
> boilerplate to create binary structures.
<nod>, then I suppose it then begins to get down to how easy those
boilderplates can be used to add new groups and attributes for
developers.. In my experience using the CONFIGFS_EATTR() macros with
multiple struct config_groups hanging of the same make_group() allocated
internal TCM structure, this has been very easy for me once I figured
out why I really needed the extended macro set (again, to hang multiple
differently named struct config_groups off a single internally allocated
structure). Joel, I know that you have been keeping the configfs macros
in sync with the parameters used for original matching sysfs macros (and
that I have been using my own configfs macro that can be used together
with existing code) but I really do think the extended macro set has
benefit for users of configfs who put a little bit of effort to
understand how they work.
> These interfaces have two opposing forces acting on them. They
> provide a reasonably nice way to cross the user<->kernel boundary, so
> people want to use them. Programmatic things, like a power management
> daemon for example, don't want sysadmins touching anything. It's just
> an interface for the daemon. Conversely, some things are really knobs
> for the sysadmin. There's nothing else to it. Why should they have to
> code up a C program just to turn a knob? Configfs, as its name implies,
> really does exist for that second case.
I think this is a very good point that really shows the benefits of a
configfs based design for real world admin useablility and
configurability (CLI building blocks for higher level UIs). Having the
ability to modify non compiled code to suit their needs on top of a user
defined configfs directory structure of groups/directories (assuming
config groups have some sort of project defined naming requrements in
each defined struct configfs_item_operations->make_group()) with
synchronization done on a individual configfs group context for
creation/deletion and optionally the I/O access of attributes within
said group.
> It turns out that it's quite
> nice to use for the first case too, but if folks wanted to go the
> syscall route, no worries.
> I've said it many times. We will never come up with one
> over-arching solution to all the disparate use cases. Instead, we
> should use each facility - syscalls, ioctls, sysfs, configfs, etc - as
> appropriate. Even in the same program or subsystem.
>
> > - atomicity
> >
> > One attribute per file means that, lacking userspace-visible
> > transactions, there is no way to change several attributes at once.
> > When you read attributes, there is no way to read several attributes
> > atomically so you can be sure their values correlate. Another
> > example of a problem is when an object disappears while reading its
> > attributes. Sure, openat() can mitigate this, but it's better to
> > avoid introducing problem than having a fix.
>
> configfs has some atomicity capabilities, but not full
> atomicity. It's not the right too for that sort of thing.
>
> > - ambiguity
> >
> > What format is the attribute? does it accept lowercase or uppercase
> > hex digits? is there a newline at the end? how many digits can it
> > take before the attribute overflows? All of this has to be
> > documented and checked by the OS, otherwise we risk regressions
> > later. In contrast, __u64 says everything in a binary interface.
>
> Um, is that __u64 a pointer to a userspace object? A key to a
> lookup table? A file descriptor that is padded out? It's no less
> ambiguous.
>
> > - lifetime and access control
> >
> > If a process brings an object into being (using mkdir) and then
> > dies, the object remains behind. The syscall/ioctl approach ties
> > the object into an fd, which will be destroyed when the process
> > dies, and which can be passed around using SCM_RIGHTS, allowing a
> > server process to create and configure an object before passing it
> > to an unprivileged program
>
> Most things here do *not* want to be tied to the lifetime of one
> process. We don't want our cpu_freq governor changing just because the
> power manager died.
>
>
> > You may argue, correctly, that syscalls and ioctls are not as
> > flexible. But this is because no one has invested the effort in
> > making them so. A struct passed as an argument to a syscall is not
> > extensible. But if you pass the size of the structure, and also a
> > bitmap of which attributes are present, you gain extensibility and
> > retain the atomicity property of a syscall interface. I don't think
> > a lot of effort is needed to make an extensible syscall interface
> > just as usable and a lot more efficient than configfs/sysfs. It
> > should also be simple to bolt a fuse interface on top to expose it
> > to us commandline types.
>
> Your extensible syscall still needs to be known. The
> flexibility provided by configfs and sysfs is of generic access to
> non-generic things. It's different.
> The follow-ups regarding the perf_counter call are a good
> example. If you know the perf_counter call, you can code up a C program
> that asks what attributes or things are there. But if you don't, you've
> first got to find out that there's a perf_counter call, then learn how
> to use it. With configfs/sysfs, you notice that there's now a
> perf_counter directory under a tree, and you can figure out what
> attributes and items are there.
> But this is not the be-all-end-all. Our syscalls should be more
> flexible in the perf_counter way. Not everything really needs to be
> listable by some yokel sysadmin.
>
> > configfs is more maintainable that a bunch of hand-maintained
> > ioctls. But if we put some effort into an extendable syscall
> > infrastructure (perhaps to the point of using an IDL) I'm sure we
> > can improve on that without the problems pseudo filesystems
> > introduce.
>
> Oh, boy, IDL :-) Seriously, if you can solve the "how do I just
> poke around without actually writing C code or installing a
> domain-specific binary" problem, you will probably get somewhere.
>
Also, having the configfs directory hierarchy that is based on names
provided by user that can be accessed by higher level code or directly
by the shell, 'tree' and friends is pretty nice too if you are the admin
running the box. ;-)
> > I can't really fault a project for using configfs; it's an accepted
> > and recommented (by the community) interface. I'd much prefer it
> > though if there was an effort to create a usable fd/struct based
> > alternative.
>
> Oh, and configfs was explicitly designed to be interface
> agnostic to the client. The filesystem portions, to the best of my
> ability, are not exposed to client drivers. So you can replace the
> configfs filesystem interface with a system call set that does the same
> operations, and no configfs user will actually need to change their
> code (if you want to change from text values to non-text, that would
> require changing the show/store operation prototypes, but that's about
> it).
>
Wow really..? I was wondering if something like this was possible in
terms of different client interfaces for configfs ops, and where it
would (ever..?) make sense..
--nab
> Joel
>
^ permalink raw reply
* [net-next PATCH 0/3] qlge: Change RSS ring to MSIx vector mapping.
From: Ron Mercer @ 2009-08-19 23:53 UTC (permalink / raw)
To: davem; +Cc: netdev, ron.mercer
Hello Dave and All,
Attached is a respin of the changes posted earlier this week.
The new RX buffer changes were removed from this series
and will be address them in a separate series when these changes
are accepted.
Changes are as follows:
1) Move TX completion processing from a worker thread to
the send path. This reduces overhead and causes better
locality for freeing TX resources. It also prevents
the TX completion rings from using MSIx vectors hence
allowing them all to be used for RSS rings.
2) Change RSS ring to MSIx vector mapping. Some platforms
are stingy with vectors. This change causes all vectors
to be used for RSS rings. Previously there was a dedicated
ring/vector for handling broadcast/multicast and
various events from the chip and firmware. This 'default'
functionality is now performed by the zeroth RSS ring before
waking NAPI for normal RX processing.
This is a big patch but is mostly deletions.
3) Remove worker threads and ISR that were rendered unused
by #2 above. They were left in the code for patch #2
to make it more readable.
Regards,
Ron Mercer
^ permalink raw reply
* [net-next PATCH 1/3] qlge: Move TX completion processing to send path.
From: Ron Mercer @ 2009-08-19 23:53 UTC (permalink / raw)
To: davem; +Cc: netdev, ron.mercer
In-Reply-To: <1250725991-7155-1-git-send-email-ron.mercer@qlogic.com>
Turn off interrupt generation for outbound completions. Handle them in
the send path. Use a timer as a backup that is protected by the
txq->xmit_lock.
Signed-off-by: Ron Mercer <ron.mercer@qlogic.com>
---
drivers/net/qlge/qlge.h | 5 ++-
drivers/net/qlge/qlge_main.c | 60 ++++++++++++++++++++++++++++++++++++++---
2 files changed, 58 insertions(+), 7 deletions(-)
diff --git a/drivers/net/qlge/qlge.h b/drivers/net/qlge/qlge.h
index 6ed5317..94dfba4 100644
--- a/drivers/net/qlge/qlge.h
+++ b/drivers/net/qlge/qlge.h
@@ -1205,6 +1205,7 @@ struct bq_desc {
};
#define QL_TXQ_IDX(qdev, skb) (smp_processor_id()%(qdev->tx_ring_count))
+#define TXQ_CLEAN_TIME (HZ/4)
struct tx_ring {
/*
@@ -1224,11 +1225,11 @@ struct tx_ring {
u8 wq_id; /* queue id for this entry */
u8 reserved1[3];
struct tx_ring_desc *q; /* descriptor list for the queue */
- spinlock_t lock;
atomic_t tx_count; /* counts down for every outstanding IO */
atomic_t queue_stopped; /* Turns queue off when full. */
- struct delayed_work tx_work;
+ struct netdev_queue *txq;
struct ql_adapter *qdev;
+ struct timer_list txq_clean_timer;
};
/*
diff --git a/drivers/net/qlge/qlge_main.c b/drivers/net/qlge/qlge_main.c
index 3a271af..c964066 100644
--- a/drivers/net/qlge/qlge_main.c
+++ b/drivers/net/qlge/qlge_main.c
@@ -1797,22 +1797,43 @@ static int ql_clean_outbound_rx_ring(struct rx_ring *rx_ring)
ql_update_cq(rx_ring);
prod = ql_read_sh_reg(rx_ring->prod_idx_sh_reg);
}
+ if (!count)
+ return count;
ql_write_cq_idx(rx_ring);
tx_ring = &qdev->tx_ring[net_rsp->txq_idx];
- if (__netif_subqueue_stopped(qdev->ndev, tx_ring->wq_id) &&
- net_rsp != NULL) {
+ if (netif_tx_queue_stopped(tx_ring->txq)) {
if (atomic_read(&tx_ring->queue_stopped) &&
(atomic_read(&tx_ring->tx_count) > (tx_ring->wq_len / 4)))
/*
* The queue got stopped because the tx_ring was full.
* Wake it up, because it's now at least 25% empty.
*/
- netif_wake_subqueue(qdev->ndev, tx_ring->wq_id);
+ if (netif_running(qdev->ndev)) {
+ netif_tx_wake_queue(tx_ring->txq);
+ atomic_dec(&tx_ring->queue_stopped);
+ }
}
return count;
}
+/* When the lock is free we periodically check for
+ * unhandled completions.
+ */
+static void ql_txq_clean_timer(unsigned long data)
+{
+ struct tx_ring *tx_ring = (struct tx_ring *)data;
+ struct ql_adapter *qdev = tx_ring->qdev;
+ struct rx_ring *rx_ring = &qdev->rx_ring[tx_ring->cq_id];
+
+ if (__netif_tx_trylock(tx_ring->txq)) {
+ ql_clean_outbound_rx_ring(rx_ring);
+ __netif_tx_unlock(tx_ring->txq);
+ }
+ mod_timer(&tx_ring->txq_clean_timer, jiffies + TXQ_CLEAN_TIME);
+
+}
+
static int ql_clean_inbound_rx_ring(struct rx_ring *rx_ring, int budget)
{
struct ql_adapter *qdev = rx_ring->qdev;
@@ -2039,6 +2060,8 @@ static irqreturn_t qlge_isr(int irq, void *dev_id)
rx_ring = &qdev->rx_ring[i];
if (ql_read_sh_reg(rx_ring->prod_idx_sh_reg) !=
rx_ring->cnsmr_idx) {
+ if (rx_ring->type == TX_Q)
+ continue;
QPRINTK(qdev, INTR, INFO,
"Waking handler for rx_ring[%d].\n", i);
ql_disable_completion_interrupt(qdev,
@@ -2146,11 +2169,17 @@ static int qlge_send(struct sk_buff *skb, struct net_device *ndev)
if (skb_padto(skb, ETH_ZLEN))
return NETDEV_TX_OK;
+ /* If there is at least 16 entries to clean then
+ * go do it.
+ */
+ if (tx_ring->wq_len - atomic_read(&tx_ring->tx_count) > 16)
+ ql_clean_outbound_rx_ring(&qdev->rx_ring[tx_ring->cq_id]);
+
if (unlikely(atomic_read(&tx_ring->tx_count) < 2)) {
QPRINTK(qdev, TX_QUEUED, INFO,
"%s: shutting down tx queue %d du to lack of resources.\n",
__func__, tx_ring_idx);
- netif_stop_subqueue(ndev, tx_ring->wq_id);
+ netif_tx_stop_queue(tx_ring->txq);
atomic_inc(&tx_ring->queue_stopped);
return NETDEV_TX_BUSY;
}
@@ -2167,6 +2196,8 @@ static int qlge_send(struct sk_buff *skb, struct net_device *ndev)
tx_ring_desc->skb = skb;
mac_iocb_ptr->frame_len = cpu_to_le16((u16) skb->len);
+ /* Disable completion interrupt for this packet. */
+ mac_iocb_ptr->flags1 |= OB_MAC_IOCB_REQ_I;
if (qdev->vlgrp && vlan_tx_tag_present(skb)) {
QPRINTK(qdev, TX_QUEUED, DEBUG, "Adding a vlan tag %d.\n",
@@ -2192,13 +2223,20 @@ static int qlge_send(struct sk_buff *skb, struct net_device *ndev)
tx_ring->prod_idx++;
if (tx_ring->prod_idx == tx_ring->wq_len)
tx_ring->prod_idx = 0;
+ atomic_dec(&tx_ring->tx_count);
wmb();
+ /* Run the destructor before telling the DMA engine about
+ * the packet to make sure it doesn't complete and get
+ * freed prematurely.
+ */
+ if (likely(!skb_shared(skb)))
+ skb_orphan(skb);
+
ql_write_db_reg(tx_ring->prod_idx, tx_ring->prod_idx_db_reg);
QPRINTK(qdev, TX_QUEUED, DEBUG, "tx queued, slot %d, len %d\n",
tx_ring->prod_idx, skb->len);
- atomic_dec(&tx_ring->tx_count);
return NETDEV_TX_OK;
}
@@ -2783,6 +2821,8 @@ static int ql_start_tx_ring(struct ql_adapter *qdev, struct tx_ring *tx_ring)
*/
tx_ring->cnsmr_idx_sh_reg = shadow_reg;
tx_ring->cnsmr_idx_sh_reg_dma = shadow_reg_dma;
+ *tx_ring->cnsmr_idx_sh_reg = 0;
+ tx_ring->txq = netdev_get_tx_queue(qdev->ndev, tx_ring->wq_id);
wqicb->len = cpu_to_le16(tx_ring->wq_len | Q_LEN_V | Q_LEN_CPP_CONT);
wqicb->flags = cpu_to_le16(Q_FLAGS_LC |
@@ -2802,6 +2842,7 @@ static int ql_start_tx_ring(struct ql_adapter *qdev, struct tx_ring *tx_ring)
return err;
}
QPRINTK(qdev, IFUP, DEBUG, "Successfully loaded WQICB.\n");
+ mod_timer(&tx_ring->txq_clean_timer, jiffies + TXQ_CLEAN_TIME);
return err;
}
@@ -3362,6 +3403,12 @@ static int ql_adapter_down(struct ql_adapter *qdev)
}
}
+ /* Delete the timers used for cleaning up
+ * TX completions.
+ */
+ for (i = 0; i < qdev->tx_ring_count; i++)
+ del_timer_sync(&qdev->tx_ring[i].txq_clean_timer);
+
clear_bit(QL_ADAPTER_UP, &qdev->flags);
ql_disable_interrupts(qdev);
@@ -3501,6 +3548,9 @@ static int ql_configure_rings(struct ql_adapter *qdev)
* immediately after the default Q ID, which is zero.
*/
tx_ring->cq_id = i + 1;
+ init_timer(&tx_ring->txq_clean_timer);
+ tx_ring->txq_clean_timer.data = (unsigned long)tx_ring;
+ tx_ring->txq_clean_timer.function = ql_txq_clean_timer;
}
for (i = 0; i < qdev->rx_ring_count; i++) {
--
1.6.0.2
^ permalink raw reply related
* [net-next PATCH 3/3] qlge: Remove unused workers and irq handler.
From: Ron Mercer @ 2009-08-19 23:53 UTC (permalink / raw)
To: davem; +Cc: netdev, ron.mercer
In-Reply-To: <1250725991-7155-1-git-send-email-ron.mercer@qlogic.com>
Signed-off-by: Ron Mercer <ron.mercer@qlogic.com>
---
drivers/net/qlge/qlge.h | 2 --
drivers/net/qlge/qlge_main.c | 32 --------------------------------
2 files changed, 0 insertions(+), 34 deletions(-)
diff --git a/drivers/net/qlge/qlge.h b/drivers/net/qlge/qlge.h
index 9e30fb0..ac14f1b 100644
--- a/drivers/net/qlge/qlge.h
+++ b/drivers/net/qlge/qlge.h
@@ -1292,7 +1292,6 @@ struct rx_ring {
u32 cpu; /* Which CPU this should run on. */
char name[IFNAMSIZ + 5];
struct napi_struct napi;
- struct delayed_work rx_work;
u8 reserved;
struct ql_adapter *qdev;
};
@@ -1516,7 +1515,6 @@ struct ql_adapter {
union flash_params flash;
struct net_device_stats stats;
- struct workqueue_struct *q_workqueue;
struct workqueue_struct *workqueue;
struct delayed_work asic_reset_work;
struct delayed_work mpi_reset_work;
diff --git a/drivers/net/qlge/qlge_main.c b/drivers/net/qlge/qlge_main.c
index 56b5d0e..b814c4d 100644
--- a/drivers/net/qlge/qlge_main.c
+++ b/drivers/net/qlge/qlge_main.c
@@ -1945,38 +1945,6 @@ static void ql_vlan_rx_kill_vid(struct net_device *ndev, u16 vid)
}
-/* Worker thread to process a given rx_ring that is dedicated
- * to outbound completions.
- */
-static void ql_tx_clean(struct work_struct *work)
-{
- struct rx_ring *rx_ring =
- container_of(work, struct rx_ring, rx_work.work);
- ql_clean_outbound_rx_ring(rx_ring);
- ql_enable_completion_interrupt(rx_ring->qdev, rx_ring->irq);
-
-}
-
-/* Worker thread to process a given rx_ring that is dedicated
- * to inbound completions.
- */
-static void ql_rx_clean(struct work_struct *work)
-{
- struct rx_ring *rx_ring =
- container_of(work, struct rx_ring, rx_work.work);
- ql_clean_inbound_rx_ring(rx_ring, 64);
- ql_enable_completion_interrupt(rx_ring->qdev, rx_ring->irq);
-}
-
-/* MSI-X Multiple Vector Interrupt Handler for outbound completions. */
-static irqreturn_t qlge_msix_tx_isr(int irq, void *dev_id)
-{
- struct rx_ring *rx_ring = dev_id;
- queue_delayed_work_on(rx_ring->cpu, rx_ring->qdev->q_workqueue,
- &rx_ring->rx_work, 0);
- return IRQ_HANDLED;
-}
-
/* MSI-X Multiple Vector Interrupt Handler for inbound completions. */
static irqreturn_t qlge_msix_dflt_rx_isr(int irq, void *dev_id)
{
--
1.6.0.2
^ permalink raw reply related
* [net-next PATCH 2/3] qlge: Change RSS ring to MSIx vector mapping.
From: Ron Mercer @ 2009-08-19 23:53 UTC (permalink / raw)
To: davem; +Cc: netdev, ron.mercer
In-Reply-To: <1250725991-7155-1-git-send-email-ron.mercer@qlogic.com>
Currently we set aside one vector for a 'default' ring that handles
broadcast/multicast, and events from the chip and firmware.
The remainder of the vectors are split among the RSS rings and TX
completion rings.
Now that TX completions are handled in the send path, this change uses
all vectors for RSS rings on a one to one basis.
The broadcast/multicast and events are rolled into the functionality of
the 1st RSS ring and are processed before NAPI is called for RSS.
The remaining rings call NAPI directly.
Signed-off-by: Ron Mercer <ron.mercer@qlogic.com>
---
drivers/net/qlge/qlge.h | 12 +-
drivers/net/qlge/qlge_dbg.c | 8 +-
drivers/net/qlge/qlge_ethtool.c | 20 ++--
drivers/net/qlge/qlge_main.c | 299 ++++++++++++++++-----------------------
4 files changed, 135 insertions(+), 204 deletions(-)
diff --git a/drivers/net/qlge/qlge.h b/drivers/net/qlge/qlge.h
index 94dfba4..9e30fb0 100644
--- a/drivers/net/qlge/qlge.h
+++ b/drivers/net/qlge/qlge.h
@@ -34,7 +34,7 @@
#define QLGE_DEVICE_ID_8000 0x8000
#define MAX_CPUS 8
#define MAX_TX_RINGS MAX_CPUS
-#define MAX_RX_RINGS ((MAX_CPUS * 2) + 1)
+#define MAX_RX_RINGS (MAX_CPUS * 2)
#define NUM_TX_RING_ENTRIES 256
#define NUM_RX_RING_ENTRIES 256
@@ -1236,7 +1236,6 @@ struct tx_ring {
* Type of inbound queue.
*/
enum {
- DEFAULT_Q = 2, /* Handles slow queue and chip/MPI events. */
TX_Q = 3, /* Handles outbound completions. */
RX_Q = 4, /* Handles inbound completions. */
};
@@ -1288,7 +1287,7 @@ struct rx_ring {
u32 sbq_free_cnt; /* free buffer desc cnt */
/* Misc. handler elements. */
- u32 type; /* Type of queue, tx, rx, or default. */
+ u32 type; /* Type of queue, tx, rx. */
u32 irq; /* Which vector this ring is assigned. */
u32 cpu; /* Which CPU this should run on. */
char name[IFNAMSIZ + 5];
@@ -1487,13 +1486,11 @@ struct ql_adapter {
struct intr_context intr_context[MAX_RX_RINGS];
int tx_ring_count; /* One per online CPU. */
- u32 rss_ring_first_cq_id;/* index of first inbound (rss) rx_ring */
- u32 rss_ring_count; /* One per online CPU. */
+ u32 rss_ring_count; /* One per irq vector. */
/*
* rx_ring_count =
- * one default queue +
* (CPU count * outbound completion rx_ring) +
- * (CPU count * inbound (RSS) completion rx_ring)
+ * (irq_vector_cnt * inbound (RSS) completion rx_ring)
*/
int rx_ring_count;
int ring_mem_size;
@@ -1503,7 +1500,6 @@ struct ql_adapter {
struct tx_ring tx_ring[MAX_TX_RINGS];
int rx_csum;
- u32 default_rx_queue;
u16 rx_coalesce_usecs; /* cqicb->int_delay */
u16 rx_max_coalesced_frames; /* cqicb->pkt_int_delay */
diff --git a/drivers/net/qlge/qlge_dbg.c b/drivers/net/qlge/qlge_dbg.c
index 40a70c3..c6d4db5 100644
--- a/drivers/net/qlge/qlge_dbg.c
+++ b/drivers/net/qlge/qlge_dbg.c
@@ -418,13 +418,9 @@ void ql_dump_qdev(struct ql_adapter *qdev)
printk(KERN_ERR PFX "qdev->intr_count = %d.\n", qdev->intr_count);
printk(KERN_ERR PFX "qdev->tx_ring = %p.\n",
qdev->tx_ring);
- printk(KERN_ERR PFX "qdev->rss_ring_first_cq_id = %d.\n",
- qdev->rss_ring_first_cq_id);
printk(KERN_ERR PFX "qdev->rss_ring_count = %d.\n",
qdev->rss_ring_count);
printk(KERN_ERR PFX "qdev->rx_ring = %p.\n", qdev->rx_ring);
- printk(KERN_ERR PFX "qdev->default_rx_queue = %d.\n",
- qdev->default_rx_queue);
printk(KERN_ERR PFX "qdev->xg_sem_mask = 0x%08x.\n",
qdev->xg_sem_mask);
printk(KERN_ERR PFX "qdev->port_link_up = 0x%08x.\n",
@@ -545,8 +541,8 @@ void ql_dump_rx_ring(struct rx_ring *rx_ring)
printk(KERN_ERR PFX
"===================== Dumping rx_ring %d ===============.\n",
rx_ring->cq_id);
- printk(KERN_ERR PFX "Dumping rx_ring %d, type = %s%s%s.\n",
- rx_ring->cq_id, rx_ring->type == DEFAULT_Q ? "DEFAULT" : "",
+ printk(KERN_ERR PFX "Dumping rx_ring %d, type = %s%s.\n",
+ rx_ring->cq_id,
rx_ring->type == TX_Q ? "OUTBOUND COMPLETIONS" : "",
rx_ring->type == RX_Q ? "INBOUND_COMPLETIONS" : "");
printk(KERN_ERR PFX "rx_ring->cqicb = %p.\n", &rx_ring->cqicb);
diff --git a/drivers/net/qlge/qlge_ethtool.c b/drivers/net/qlge/qlge_ethtool.c
index eb6a9ee..8938111 100644
--- a/drivers/net/qlge/qlge_ethtool.c
+++ b/drivers/net/qlge/qlge_ethtool.c
@@ -49,15 +49,16 @@ static int ql_update_ring_coalescing(struct ql_adapter *qdev)
/* Skip the default queue, and update the outbound handler
* queues if they changed.
*/
- cqicb = (struct cqicb *)&qdev->rx_ring[1];
+ cqicb = (struct cqicb *)&qdev->rx_ring[qdev->rss_ring_count];
if (le16_to_cpu(cqicb->irq_delay) != qdev->tx_coalesce_usecs ||
- le16_to_cpu(cqicb->pkt_delay) != qdev->tx_max_coalesced_frames) {
- for (i = 1; i < qdev->rss_ring_first_cq_id; i++, rx_ring++) {
+ le16_to_cpu(cqicb->pkt_delay) !=
+ qdev->tx_max_coalesced_frames) {
+ for (i = qdev->rss_ring_count; i < qdev->rx_ring_count; i++) {
rx_ring = &qdev->rx_ring[i];
cqicb = (struct cqicb *)rx_ring;
cqicb->irq_delay = cpu_to_le16(qdev->tx_coalesce_usecs);
cqicb->pkt_delay =
- cpu_to_le16(qdev->tx_max_coalesced_frames);
+ cpu_to_le16(qdev->tx_max_coalesced_frames);
cqicb->flags = FLAGS_LI;
status = ql_write_cfg(qdev, cqicb, sizeof(*cqicb),
CFG_LCQ, rx_ring->cq_id);
@@ -70,17 +71,16 @@ static int ql_update_ring_coalescing(struct ql_adapter *qdev)
}
/* Update the inbound (RSS) handler queues if they changed. */
- cqicb = (struct cqicb *)&qdev->rx_ring[qdev->rss_ring_first_cq_id];
+ cqicb = (struct cqicb *)&qdev->rx_ring[0];
if (le16_to_cpu(cqicb->irq_delay) != qdev->rx_coalesce_usecs ||
- le16_to_cpu(cqicb->pkt_delay) != qdev->rx_max_coalesced_frames) {
- for (i = qdev->rss_ring_first_cq_id;
- i <= qdev->rss_ring_first_cq_id + qdev->rss_ring_count;
- i++) {
+ le16_to_cpu(cqicb->pkt_delay) !=
+ qdev->rx_max_coalesced_frames) {
+ for (i = 0; i < qdev->rss_ring_count; i++, rx_ring++) {
rx_ring = &qdev->rx_ring[i];
cqicb = (struct cqicb *)rx_ring;
cqicb->irq_delay = cpu_to_le16(qdev->rx_coalesce_usecs);
cqicb->pkt_delay =
- cpu_to_le16(qdev->rx_max_coalesced_frames);
+ cpu_to_le16(qdev->rx_max_coalesced_frames);
cqicb->flags = FLAGS_LI;
status = ql_write_cfg(qdev, cqicb, sizeof(*cqicb),
CFG_LCQ, rx_ring->cq_id);
diff --git a/drivers/net/qlge/qlge_main.c b/drivers/net/qlge/qlge_main.c
index c964066..56b5d0e 100644
--- a/drivers/net/qlge/qlge_main.c
+++ b/drivers/net/qlge/qlge_main.c
@@ -370,9 +370,7 @@ static int ql_set_mac_addr_reg(struct ql_adapter *qdev, u8 *addr, u32 type,
cam_output = (CAM_OUT_ROUTE_NIC |
(qdev->
func << CAM_OUT_FUNC_SHIFT) |
- (qdev->
- rss_ring_first_cq_id <<
- CAM_OUT_CQ_ID_SHIFT));
+ (0 << CAM_OUT_CQ_ID_SHIFT));
if (qdev->vlgrp)
cam_output |= CAM_OUT_RV;
/* route to NIC core */
@@ -616,9 +614,8 @@ u32 ql_enable_completion_interrupt(struct ql_adapter *qdev, u32 intr)
unsigned long hw_flags = 0;
struct intr_context *ctx = qdev->intr_context + intr;
- if (likely(test_bit(QL_MSIX_ENABLED, &qdev->flags) && intr)) {
- /* Always enable if we're MSIX multi interrupts and
- * it's not the default (zeroeth) interrupt.
+ if (likely(test_bit(QL_MSIX_ENABLED, &qdev->flags))) {
+ /* Always enable if we're MSIX multi interrupts.
*/
ql_write32(qdev, INTR_EN,
ctx->intr_en_mask);
@@ -644,7 +641,7 @@ static u32 ql_disable_completion_interrupt(struct ql_adapter *qdev, u32 intr)
/* HW disables for us if we're MSIX multi interrupts and
* it's not the default (zeroeth) interrupt.
*/
- if (likely(test_bit(QL_MSIX_ENABLED, &qdev->flags) && intr))
+ if (likely(test_bit(QL_MSIX_ENABLED, &qdev->flags)))
return 0;
ctx = qdev->intr_context + intr;
@@ -1649,8 +1646,7 @@ static void ql_process_mac_rx_intr(struct ql_adapter *qdev,
qdev->stats.rx_packets++;
qdev->stats.rx_bytes += skb->len;
- skb_record_rx_queue(skb,
- rx_ring->cq_id - qdev->rss_ring_first_cq_id);
+ skb_record_rx_queue(skb, rx_ring->cq_id);
if (skb->ip_summed == CHECKSUM_UNNECESSARY) {
if (qdev->vlgrp &&
(ib_mac_rsp->flags2 & IB_MAC_IOCB_RSP_V) &&
@@ -1982,6 +1978,50 @@ static irqreturn_t qlge_msix_tx_isr(int irq, void *dev_id)
}
/* MSI-X Multiple Vector Interrupt Handler for inbound completions. */
+static irqreturn_t qlge_msix_dflt_rx_isr(int irq, void *dev_id)
+{
+ struct rx_ring *rx_ring = dev_id;
+ struct ql_adapter *qdev = rx_ring->qdev;
+ u32 var;
+
+ var = ql_read32(qdev, STS);
+ /*
+ * Check for fatal error.
+ */
+ if (var & STS_FE) {
+ ql_queue_asic_error(qdev);
+ QPRINTK(qdev, INTR, ERR, "Got fatal error, STS = %x.\n", var);
+ var = ql_read32(qdev, ERR_STS);
+ QPRINTK(qdev, INTR, ERR,
+ "Resetting chip. Error Status Register = 0x%x\n", var);
+ return IRQ_HANDLED;
+ }
+
+ /*
+ * Check MPI processor activity.
+ */
+ if ((var & STS_PI) &&
+ (ql_read32(qdev, INTR_MASK) & INTR_MASK_PI)) {
+ /*
+ * We've got an async event or mailbox completion.
+ * Handle it and clear the source of the interrupt.
+ */
+ QPRINTK(qdev, INTR, ERR, "Got MPI processor interrupt.\n");
+ queue_delayed_work(qdev->workqueue,
+ &qdev->mpi_work, 0);
+ return IRQ_HANDLED;
+ }
+
+ /*
+ * Start NAPI if there's work to do.
+ */
+ if (ql_read_sh_reg(rx_ring->prod_idx_sh_reg) !=
+ rx_ring->cnsmr_idx)
+ napi_schedule(&rx_ring->napi);
+ return IRQ_HANDLED;
+}
+
+/* MSI-X Multiple Vector Interrupt Handler for inbound completions. */
static irqreturn_t qlge_msix_rx_isr(int irq, void *dev_id)
{
struct rx_ring *rx_ring = dev_id;
@@ -2040,23 +2080,11 @@ static irqreturn_t qlge_isr(int irq, void *dev_id)
work_done++;
}
- /*
- * Check the default queue and wake handler if active.
- */
- rx_ring = &qdev->rx_ring[0];
- if (ql_read_sh_reg(rx_ring->prod_idx_sh_reg) != rx_ring->cnsmr_idx) {
- QPRINTK(qdev, INTR, INFO, "Waking handler for rx_ring[0].\n");
- ql_disable_completion_interrupt(qdev, intr_context->intr);
- queue_delayed_work_on(smp_processor_id(), qdev->q_workqueue,
- &rx_ring->rx_work, 0);
- work_done++;
- }
-
if (!test_bit(QL_MSIX_ENABLED, &qdev->flags)) {
/*
* Start the DPC for each active queue.
*/
- for (i = 1; i < qdev->rx_ring_count; i++) {
+ for (i = 0; i < qdev->rx_ring_count; i++) {
rx_ring = &qdev->rx_ring[i];
if (ql_read_sh_reg(rx_ring->prod_idx_sh_reg) !=
rx_ring->cnsmr_idx) {
@@ -2067,13 +2095,7 @@ static irqreturn_t qlge_isr(int irq, void *dev_id)
ql_disable_completion_interrupt(qdev,
intr_context->
intr);
- if (i < qdev->rss_ring_first_cq_id)
- queue_delayed_work_on(rx_ring->cpu,
- qdev->q_workqueue,
- &rx_ring->rx_work,
- 0);
- else
- napi_schedule(&rx_ring->napi);
+ napi_schedule(&rx_ring->napi);
work_done++;
}
}
@@ -2656,6 +2678,7 @@ static int ql_start_rx_ring(struct ql_adapter *qdev, struct rx_ring *rx_ring)
/* Set up the shadow registers for this ring. */
rx_ring->prod_idx_sh_reg = shadow_reg;
rx_ring->prod_idx_sh_reg_dma = shadow_reg_dma;
+ *rx_ring->prod_idx_sh_reg = 0;
shadow_reg += sizeof(u64);
shadow_reg_dma += sizeof(u64);
rx_ring->lbq_base_indirect = shadow_reg;
@@ -2744,34 +2767,9 @@ static int ql_start_rx_ring(struct ql_adapter *qdev, struct rx_ring *rx_ring)
}
switch (rx_ring->type) {
case TX_Q:
- /* If there's only one interrupt, then we use
- * worker threads to process the outbound
- * completion handling rx_rings. We do this so
- * they can be run on multiple CPUs. There is
- * room to play with this more where we would only
- * run in a worker if there are more than x number
- * of outbound completions on the queue and more
- * than one queue active. Some threshold that
- * would indicate a benefit in spite of the cost
- * of a context switch.
- * If there's more than one interrupt, then the
- * outbound completions are processed in the ISR.
- */
- if (!test_bit(QL_MSIX_ENABLED, &qdev->flags))
- INIT_DELAYED_WORK(&rx_ring->rx_work, ql_tx_clean);
- else {
- /* With all debug warnings on we see a WARN_ON message
- * when we free the skb in the interrupt context.
- */
- INIT_DELAYED_WORK(&rx_ring->rx_work, ql_tx_clean);
- }
cqicb->irq_delay = cpu_to_le16(qdev->tx_coalesce_usecs);
cqicb->pkt_delay = cpu_to_le16(qdev->tx_max_coalesced_frames);
- break;
- case DEFAULT_Q:
- INIT_DELAYED_WORK(&rx_ring->rx_work, ql_rx_clean);
- cqicb->irq_delay = 0;
- cqicb->pkt_delay = 0;
+ cqicb->msix_vect = 0;
break;
case RX_Q:
/* Inbound completion handling rx_rings run in
@@ -2859,17 +2857,20 @@ static void ql_disable_msix(struct ql_adapter *qdev)
}
}
+/* We start by trying to get the number of vectors
+ * stored in qdev->intr_count. If we don't get that
+ * many then we decrement by 1 and try again.
+ */
static void ql_enable_msix(struct ql_adapter *qdev)
{
- int i;
+ int i, err;
- qdev->intr_count = 1;
/* Get the MSIX vectors. */
if (irq_type == MSIX_IRQ) {
/* Try to alloc space for the msix struct,
* if it fails then go to MSI/legacy.
*/
- qdev->msi_x_entry = kcalloc(qdev->rx_ring_count,
+ qdev->msi_x_entry = kcalloc(qdev->intr_count,
sizeof(struct msix_entry),
GFP_KERNEL);
if (!qdev->msi_x_entry) {
@@ -2877,26 +2878,39 @@ static void ql_enable_msix(struct ql_adapter *qdev)
goto msi;
}
- for (i = 0; i < qdev->rx_ring_count; i++)
+ for (i = 0; i < qdev->intr_count; i++) {
qdev->msi_x_entry[i].entry = i;
+ qdev->msi_x_entry[i].vector = 0;
+ }
- if (!pci_enable_msix
- (qdev->pdev, qdev->msi_x_entry, qdev->rx_ring_count)) {
- set_bit(QL_MSIX_ENABLED, &qdev->flags);
- qdev->intr_count = qdev->rx_ring_count;
- QPRINTK(qdev, IFUP, DEBUG,
- "MSI-X Enabled, got %d vectors.\n",
- qdev->intr_count);
- return;
- } else {
+ /* Loop to get our vectors. We start with
+ * what we want and settle for what we get.
+ */
+ do {
+ err = pci_enable_msix(qdev->pdev,
+ qdev->msi_x_entry, qdev->intr_count);
+ if (err > 0)
+ qdev->intr_count = err;
+ } while (err > 0);
+
+ if (err < 0) {
+ pci_disable_msix(qdev->pdev);
kfree(qdev->msi_x_entry);
qdev->msi_x_entry = NULL;
QPRINTK(qdev, IFUP, WARNING,
"MSI-X Enable failed, trying MSI.\n");
+ qdev->intr_count = 1;
irq_type = MSI_IRQ;
+ } else if (err == 0) {
+ set_bit(QL_MSIX_ENABLED, &qdev->flags);
+ QPRINTK(qdev, IFUP, INFO,
+ "MSI-X Enabled, got %d vectors.\n",
+ qdev->intr_count);
+ return;
}
}
msi:
+ qdev->intr_count = 1;
if (irq_type == MSI_IRQ) {
if (!pci_enable_msi(qdev->pdev)) {
set_bit(QL_MSI_ENABLED, &qdev->flags);
@@ -2920,13 +2934,10 @@ static void ql_resolve_queues_to_irqs(struct ql_adapter *qdev)
int i = 0;
struct intr_context *intr_context = &qdev->intr_context[0];
- ql_enable_msix(qdev);
-
if (likely(test_bit(QL_MSIX_ENABLED, &qdev->flags))) {
/* Each rx_ring has it's
* own intr_context since we have separate
* vectors for each queue.
- * This only true when MSI-X is enabled.
*/
for (i = 0; i < qdev->intr_count; i++, intr_context++) {
qdev->rx_ring[i].irq = i;
@@ -2948,21 +2959,14 @@ static void ql_resolve_queues_to_irqs(struct ql_adapter *qdev)
INTR_EN_TYPE_MASK | INTR_EN_INTR_MASK |
INTR_EN_TYPE_READ | INTR_EN_IHD_MASK | INTR_EN_IHD |
i;
-
if (i == 0) {
- /*
- * Default queue handles bcast/mcast plus
- * async events. Needs buffers.
- */
- intr_context->handler = qlge_isr;
- sprintf(intr_context->name, "%s-default-queue",
- qdev->ndev->name);
- } else if (i < qdev->rss_ring_first_cq_id) {
- /*
- * Outbound queue is for outbound completions only.
+ /* The first vector/queue handles
+ * broadcast/multicast, fatal errors,
+ * and firmware events. This in addition
+ * to normal inbound NAPI processing.
*/
- intr_context->handler = qlge_msix_tx_isr;
- sprintf(intr_context->name, "%s-tx-%d",
+ intr_context->handler = qlge_msix_dflt_rx_isr;
+ sprintf(intr_context->name, "%s-rx-%d",
qdev->ndev->name, i);
} else {
/*
@@ -2996,7 +3000,7 @@ static void ql_resolve_queues_to_irqs(struct ql_adapter *qdev)
*/
intr_context->handler = qlge_isr;
sprintf(intr_context->name, "%s-single_irq", qdev->ndev->name);
- for (i = 0; i < qdev->rx_ring_count; i++)
+ for (i = 0; i < qdev->rss_ring_count; i++)
qdev->rx_ring[i].irq = 0;
}
}
@@ -3047,11 +3051,10 @@ static int ql_request_irq(struct ql_adapter *qdev)
goto err_irq;
} else {
QPRINTK(qdev, IFUP, DEBUG,
- "Hooked intr %d, queue type %s%s%s, with name %s.\n",
+ "Hooked intr %d, queue type %s%s, "
+ "with name %s.\n",
i,
qdev->rx_ring[i].type ==
- DEFAULT_Q ? "DEFAULT_Q" : "",
- qdev->rx_ring[i].type ==
TX_Q ? "TX_Q" : "",
qdev->rx_ring[i].type ==
RX_Q ? "RX_Q" : "", intr_context->name);
@@ -3077,10 +3080,8 @@ static int ql_request_irq(struct ql_adapter *qdev)
goto err_irq;
QPRINTK(qdev, IFUP, ERR,
- "Hooked intr %d, queue type %s%s%s, with name %s.\n",
+ "Hooked intr %d, queue type %s%s, with name %s.\n",
i,
- qdev->rx_ring[0].type ==
- DEFAULT_Q ? "DEFAULT_Q" : "",
qdev->rx_ring[0].type == TX_Q ? "TX_Q" : "",
qdev->rx_ring[0].type == RX_Q ? "RX_Q" : "",
intr_context->name);
@@ -3103,7 +3104,7 @@ static int ql_start_rss(struct ql_adapter *qdev)
memset((void *)ricb, 0, sizeof(*ricb));
- ricb->base_cq = qdev->rss_ring_first_cq_id | RSS_L4K;
+ ricb->base_cq = RSS_L4K;
ricb->flags =
(RSS_L6K | RSS_LI | RSS_LB | RSS_LM | RSS_RI4 | RSS_RI6 | RSS_RT4 |
RSS_RT6);
@@ -3305,7 +3306,7 @@ static int ql_adapter_initialize(struct ql_adapter *qdev)
}
/* Start NAPI for the RSS queues. */
- for (i = qdev->rss_ring_first_cq_id; i < qdev->rx_ring_count; i++) {
+ for (i = 0; i < qdev->rss_ring_count; i++) {
QPRINTK(qdev, IFUP, DEBUG, "Enabling NAPI for rx_ring[%d].\n",
i);
napi_enable(&qdev->rx_ring[i].napi);
@@ -3367,7 +3368,6 @@ static void ql_display_dev_info(struct net_device *ndev)
static int ql_adapter_down(struct ql_adapter *qdev)
{
int i, status = 0;
- struct rx_ring *rx_ring;
ql_link_off(qdev);
@@ -3381,27 +3381,11 @@ static int ql_adapter_down(struct ql_adapter *qdev)
cancel_delayed_work_sync(&qdev->mpi_idc_work);
cancel_delayed_work_sync(&qdev->mpi_port_cfg_work);
- /* The default queue at index 0 is always processed in
- * a workqueue.
- */
- cancel_delayed_work_sync(&qdev->rx_ring[0].rx_work);
-
- /* The rest of the rx_rings are processed in
- * a workqueue only if it's a single interrupt
- * environment (MSI/Legacy).
+ /* Only the RSS rings use NAPI.
+ * Outbound completion processing is done in send context.
*/
- for (i = 1; i < qdev->rx_ring_count; i++) {
- rx_ring = &qdev->rx_ring[i];
- /* Only the RSS rings use NAPI on multi irq
- * environment. Outbound completion processing
- * is done in interrupt context.
- */
- if (i >= qdev->rss_ring_first_cq_id) {
- napi_disable(&rx_ring->napi);
- } else {
- cancel_delayed_work_sync(&rx_ring->rx_work);
- }
- }
+ for (i = 0; i < qdev->rss_ring_count; i++)
+ napi_disable(&qdev->rx_ring[i].napi);
/* Delete the timers used for cleaning up
* TX completions.
@@ -3417,7 +3401,7 @@ static int ql_adapter_down(struct ql_adapter *qdev)
/* Call netif_napi_del() from common point.
*/
- for (i = qdev->rss_ring_first_cq_id; i < qdev->rx_ring_count; i++)
+ for (i = 0; i < qdev->rss_ring_count; i++)
netif_napi_del(&qdev->rx_ring[i].napi);
ql_free_rx_buffers(qdev);
@@ -3496,43 +3480,21 @@ static int ql_configure_rings(struct ql_adapter *qdev)
int i;
struct rx_ring *rx_ring;
struct tx_ring *tx_ring;
- int cpu_cnt = num_online_cpus();
-
- /*
- * For each processor present we allocate one
- * rx_ring for outbound completions, and one
- * rx_ring for inbound completions. Plus there is
- * always the one default queue. For the CPU
- * counts we end up with the following rx_rings:
- * rx_ring count =
- * one default queue +
- * (CPU count * outbound completion rx_ring) +
- * (CPU count * inbound (RSS) completion rx_ring)
- * To keep it simple we limit the total number of
- * queues to < 32, so we truncate CPU to 8.
- * This limitation can be removed when requested.
+ int cpu_cnt = min(MAX_CPUS, (int)num_online_cpus());
+
+ /* In a perfect world we have one RSS ring for each CPU
+ * and each has it's own vector. To do that we ask for
+ * cpu_cnt vectors. ql_enable_msix() will adjust the
+ * vector count to what we actually get. We then
+ * allocate an RSS ring for each.
+ * Essentially, we are doing min(cpu_count, msix_vector_count).
*/
-
- if (cpu_cnt > MAX_CPUS)
- cpu_cnt = MAX_CPUS;
-
- /*
- * rx_ring[0] is always the default queue.
- */
- /* Allocate outbound completion ring for each CPU. */
+ qdev->intr_count = cpu_cnt;
+ ql_enable_msix(qdev);
+ /* Adjust the RSS ring count to the actual vector count. */
+ qdev->rss_ring_count = qdev->intr_count;
qdev->tx_ring_count = cpu_cnt;
- /* Allocate inbound completion (RSS) ring for each CPU. */
- qdev->rss_ring_count = cpu_cnt;
- /* cq_id for the first inbound ring handler. */
- qdev->rss_ring_first_cq_id = cpu_cnt + 1;
- /*
- * qdev->rx_ring_count:
- * Total number of rx_rings. This includes the one
- * default queue, a number of outbound completion
- * handler rx_rings, and the number of inbound
- * completion handler rx_rings.
- */
- qdev->rx_ring_count = qdev->tx_ring_count + qdev->rss_ring_count + 1;
+ qdev->rx_ring_count = qdev->tx_ring_count + qdev->rss_ring_count;
for (i = 0; i < qdev->tx_ring_count; i++) {
tx_ring = &qdev->tx_ring[i];
@@ -3545,9 +3507,9 @@ static int ql_configure_rings(struct ql_adapter *qdev)
/*
* The completion queue ID for the tx rings start
- * immediately after the default Q ID, which is zero.
+ * immediately after the rss rings.
*/
- tx_ring->cq_id = i + 1;
+ tx_ring->cq_id = qdev->rss_ring_count + i;
init_timer(&tx_ring->txq_clean_timer);
tx_ring->txq_clean_timer.data = (unsigned long)tx_ring;
tx_ring->txq_clean_timer.function = ql_txq_clean_timer;
@@ -3558,11 +3520,9 @@ static int ql_configure_rings(struct ql_adapter *qdev)
memset((void *)rx_ring, 0, sizeof(*rx_ring));
rx_ring->qdev = qdev;
rx_ring->cq_id = i;
- rx_ring->cpu = i % cpu_cnt; /* CPU to run handler on. */
- if (i == 0) { /* Default queue at index 0. */
+ if (i < qdev->rss_ring_count) {
/*
- * Default queue handles bcast/mcast plus
- * async events. Needs buffers.
+ * Inbound (RSS) queues.
*/
rx_ring->cq_len = qdev->rx_ring_size;
rx_ring->cq_size =
@@ -3575,8 +3535,8 @@ static int ql_configure_rings(struct ql_adapter *qdev)
rx_ring->sbq_size =
rx_ring->sbq_len * sizeof(__le64);
rx_ring->sbq_buf_size = SMALL_BUFFER_SIZE * 2;
- rx_ring->type = DEFAULT_Q;
- } else if (i < qdev->rss_ring_first_cq_id) {
+ rx_ring->type = RX_Q;
+ } else {
/*
* Outbound queue handles outbound completions only.
*/
@@ -3591,22 +3551,6 @@ static int ql_configure_rings(struct ql_adapter *qdev)
rx_ring->sbq_size = 0;
rx_ring->sbq_buf_size = 0;
rx_ring->type = TX_Q;
- } else { /* Inbound completions (RSS) queues */
- /*
- * Inbound queues handle unicast frames only.
- */
- rx_ring->cq_len = qdev->rx_ring_size;
- rx_ring->cq_size =
- rx_ring->cq_len * sizeof(struct ql_net_rsp_iocb);
- rx_ring->lbq_len = NUM_LARGE_BUFFERS;
- rx_ring->lbq_size =
- rx_ring->lbq_len * sizeof(__le64);
- rx_ring->lbq_buf_size = LARGE_BUFFER_SIZE;
- rx_ring->sbq_len = NUM_SMALL_BUFFERS;
- rx_ring->sbq_size =
- rx_ring->sbq_len * sizeof(__le64);
- rx_ring->sbq_buf_size = SMALL_BUFFER_SIZE * 2;
- rx_ring->type = RX_Q;
}
}
return 0;
@@ -3895,10 +3839,7 @@ static void ql_release_all(struct pci_dev *pdev)
destroy_workqueue(qdev->workqueue);
qdev->workqueue = NULL;
}
- if (qdev->q_workqueue) {
- destroy_workqueue(qdev->q_workqueue);
- qdev->q_workqueue = NULL;
- }
+
if (qdev->reg_base)
iounmap(qdev->reg_base);
if (qdev->doorbell_area)
@@ -4011,8 +3952,6 @@ static int __devinit ql_init_device(struct pci_dev *pdev,
* Set up the operating parameters.
*/
qdev->rx_csum = 1;
-
- qdev->q_workqueue = create_workqueue(ndev->name);
qdev->workqueue = create_singlethread_workqueue(ndev->name);
INIT_DELAYED_WORK(&qdev->asic_reset_work, ql_asic_reset_work);
INIT_DELAYED_WORK(&qdev->mpi_reset_work, ql_mpi_reset_work);
--
1.6.0.2
^ permalink raw reply related
* DF Bit set on UDP traffic -- bug or feature?
From: Glen Turner @ 2009-08-19 23:22 UTC (permalink / raw)
To: netdev
Hi folks,
Is it a bug that the DF bit is set on UDP traffic when TCP Path
MTU Discovery is active, but yet when a returning ICMP DF Set But
Fragmentation Required response is received the kernel does not
attempt fragmentation of the outgoing UDP-carrying IP packets?
This has practical consequences for large RADIUS packets, such
as in the RADIUS/TLS/UDP protocol "DTLS". They are not fragmented
by the network and they are not fragmented by the kernel, and thus
the packets fail to pass from the application to the end-user.
Can I humbly suggest that when the kernel does not implement
its own fragmentation strategy (as it does with TCP Path MTU
Discovery) that the DF bit not be set by the kernel?
Thanks very much, Glen
--
Glen Turner, Network Engineer, Australia's Academic & Research Network
www.aarnet.edu.au
+61 8 8303 3936
^ permalink raw reply
* Re: [Alacrityvm-devel] configfs/sysfs
From: Alex Tsariounov @ 2009-08-19 23:48 UTC (permalink / raw)
To: Joel Becker
Cc: Avi Kivity, kvm, Michael S. Tsirkin, netdev, linux-kernel,
Nicholas A. Bellinger, alacrityvm-devel, Anthony Liguori,
Ingo Molnar
In-Reply-To: <20090819221654.GA29503@mail.oracle.com>
On Wed, 2009-08-19 at 15:16 -0700, Joel Becker wrote:
> On Wed, Aug 19, 2009 at 11:12:43PM +0300, Avi Kivity wrote:
> > On 08/19/2009 09:23 PM, Nicholas A. Bellinger wrote:
> > >Anyways, I was wondering if you might be interesting in sharing your
> > >concerns wrt to configfs (conigfs maintainer CC'ed), at some point..?
> >
> > My concerns aren't specifically with configfs, but with all the text
> > based pseudo filesystems that the kernel exposes.
>
> Phew! It's not just me :-)
The points on *fs vs. ioctl are interesting. I think both have their
benefits and both have the downfalls, for example efficiency vs. ease of
(human) use. I suppose it comes down to whether you're in the fast path
or not for the most part. However, just because and interface is not as
efficient as it can be does not necessarily mean that it's not a good
one.
As as an example, many moons ago, I worked on implementing some serial
comms between an embedded speed controller and its command console.
Being young and efficiency starved ;) I disregarded our other
controllers which implemented these serial comms with ASCII strings, and
used binary blobs instead. I indeed got some respectable performance
out of doing this, even to the effect of creating a "real time" status
monitor that updated multiple times a second via the hand-held terminal.
However, I totally missed the point of intentionally doing things
"inefficiently." For example, our serial debugging setup consisted of
two VT100 terminals wired up with a custom serial cable that went
between two communicating units. Each term would show what each end was
saying. Kinda crude, but effective. Of course with my new and improved
controller that "spoke the binary language of moisture evaporators,"
well, all one saw was garbage. :)
Additionally, someone debugging the controllers could just use a term to
talk to it if it used simple ASCII commands and take terminals, hosts
and other software out of the picture, but for my controller, well, you
could only use the custom programmed hand-held term.
I ended up supporting both ASCII and binary communications on the
controller for these (and other) reasons. However, in the end, I
ditched the binary comms since they really didn't add the efficiency in
the fast path where it should be added. (Well, I also ran out of eprom
space... :)
In any case, having a humanly understandable communications protocol (or
ABI) can be extremely useful, and just because it's not efficient
doesn't automatically mean that it's a bad thing, especially if it's in
the slow path. It does have it's down sides as mentioned in this
thread, so we really need both types. Because of that, the fuse layer
on top of a binary ABI is an interesting idea.
Alex
^ permalink raw reply
* Re: Kernel oops on setting sky2 interfaces down
From: Stephen Hemminger @ 2009-08-20 0:46 UTC (permalink / raw)
To: Rene Mayrhofer; +Cc: Mike McCormack, netdev, Richard Leitner
In-Reply-To: <392fb48f0908191505v47f99d1cvef18d40bcf4c08d1@mail.gmail.com>
On Thu, 20 Aug 2009 07:05:03 +0900
Mike McCormack <mikem@ring3k.org> wrote:
> 2009/8/20 Rene Mayrhofer <rene.mayrhofer@gibraltar.at>:
>
> > Pulling the latest sky2.c and sky2.h from net-next-2.6 and applying the patch
> > rids me of the oops - it is unreproducible right now. However, a networking
> > restart (i.e. all interfaces attached to sky2) leaves the devices in a state
> > where they no longer receive any network packets (at least nothing visible in
> > tcpdump). In this state, rmmod sky2 / modprobe sky2 gives:
>
> After you've got it into that state, does "rmmod sky2; modprobe sky2"
> change anything?
>
> thanks,
>
> Mike
Please send (I forget) the hardware info (lspci) and the register values
from ethtool -d ethX.
Some part of the power control doesn't work on Rene's system, so
device falls off the bus. Probably no auxilary +5 supplied, the register
values will tell whether driver is at fault (turning on aux when not
available), or hardware is lying about vaux.
^ permalink raw reply
* Re: [PATCH 2/2] sctp: fix heartbeat count of path failure
From: Luo Chunbo @ 2009-08-20 1:36 UTC (permalink / raw)
To: Vlad Yasevich; +Cc: davem, netdev, linux-sctp
In-Reply-To: <4A8C10BB.2040804@hp.com>
On Wed, 2009-08-19 at 10:48 -0400, Vlad Yasevich wrote:
> Chunbo Luo wrote:
> > RFC4960 Section 8.2 defined that the transport should enter INACTIVE
> > state only when the value in the error counter exceeds the protocol
> > parameter 'Path.Max.Retrans' of that destination address. This means
> > that the transport should enter INACTIVE state after pathmaxrxt+1
> > heartbeats are not acknowledged.
> >
> >
> > Signed-off-by: Chunbo Luo <chunbo.luo@windriver.com>
>
> NAK. This patch seems to resurface periodically and I have to keep
> explaining that it's wrong.
>
> Every time we send a HB, we tick up the error count and clear it when
> the HB-ACK is received. Each HB is separate and not a retransmission,
> so we once we reach the pathmaxrxt, we've already sent max+1 HB, so we
> have time out. Walk through the code with some values and you'll see
> what I mean.
Although we've already sent max+1 HB, but the code set the transport to
INACTIVE state immediately, which is equal to not sending the max+1 HB
at all. We should wait for a next period and make sure the max+1 HB was
not acknowledged.
Chunbo
>
> -vlad
>
> > ---
> > net/sctp/sm_sideeffect.c | 2 +-
> > 1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
> > index 86426aa..0e2e269 100644
> > --- a/net/sctp/sm_sideeffect.c
> > +++ b/net/sctp/sm_sideeffect.c
> > @@ -447,7 +447,7 @@ static void sctp_do_8_2_transport_strike(struct sctp_association *asoc,
> > asoc->overall_error_count++;
> >
> > if (transport->state != SCTP_INACTIVE &&
> > - (transport->error_count++ >= transport->pathmaxrxt)) {
> > + (transport->error_count++ > transport->pathmaxrxt)) {
> > SCTP_DEBUG_PRINTK_IPADDR("transport_strike:association %p",
> > " transport IP: port:%d failed.\n",
> > asoc,
>
^ permalink raw reply
* Re: [PATCH 3/3] Save and restore UNIX socket peer credentials (v2)
From: Serge E. Hallyn @ 2009-08-20 1:36 UTC (permalink / raw)
To: Dan Smith; +Cc: orenl, containers, netdev
In-Reply-To: <1250625435-16299-4-git-send-email-danms@us.ibm.com>
Quoting Dan Smith (danms@us.ibm.com):
> This saves the uid/gid of the sk_peercred structure in the checkpoint
> stream. On restart, it uses may_setuid() and may_setgid() to determine
> if the uid/gid from the checkpoint stream may be used.
>
> Changes in v2:
> - Adjust for may_setgid() change
>
> Signed-off-by: Dan Smith <danms@us.ibm.com>
> ---
> include/linux/checkpoint_hdr.h | 2 ++
> net/unix/checkpoint.c | 29 ++++++++++++++++-------------
> 2 files changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/include/linux/checkpoint_hdr.h b/include/linux/checkpoint_hdr.h
> index 4d5c22a..78f1f27 100644
> --- a/include/linux/checkpoint_hdr.h
> +++ b/include/linux/checkpoint_hdr.h
> @@ -414,6 +414,8 @@ struct ckpt_hdr_socket_unix {
> struct ckpt_hdr h;
> __s32 this;
> __s32 peer;
> + __u32 peercred_uid;
> + __u32 peercred_gid;
> __u32 flags;
> __u32 laddr_len;
> __u32 raddr_len;
> diff --git a/net/unix/checkpoint.c b/net/unix/checkpoint.c
> index 81252e3..366bc80 100644
> --- a/net/unix/checkpoint.c
> +++ b/net/unix/checkpoint.c
> @@ -3,6 +3,7 @@
> #include <linux/fs_struct.h>
> #include <linux/checkpoint.h>
> #include <linux/checkpoint_hdr.h>
> +#include <linux/user.h>
> #include <net/af_unix.h>
> #include <net/tcp_states.h>
>
> @@ -94,6 +95,9 @@ int unix_checkpoint(struct ckpt_ctx *ctx, struct socket *sock)
> goto out;
> }
>
> + un->peercred_uid = sock->sk->sk_peercred.uid;
> + un->peercred_gid = sock->sk->sk_peercred.gid;
> +
> ret = ckpt_write_obj(ctx, (struct ckpt_hdr *) un);
> if (ret < 0)
> goto out;
> @@ -217,19 +221,6 @@ static int unix_join(struct ckpt_ctx *ctx,
> unix_sk(a)->peer = b;
> unix_sk(b)->peer = a;
>
> - /* TODO:
> - * Checkpoint the credentials, restore them here if the values match
> - * the restored creds or we may_setuid()
> - */
> -
> - a->sk_peercred.pid = task_tgid_vnr(current);
> - a->sk_peercred.uid = ctx->realcred->uid;
> - a->sk_peercred.gid = ctx->realcred->gid;
> -
> - b->sk_peercred.pid = a->sk_peercred.pid;
> - b->sk_peercred.uid = a->sk_peercred.uid;
> - b->sk_peercred.gid = a->sk_peercred.gid;
> -
> if (!UNIX_ADDR_EMPTY(un->raddr_len))
> addr = unix_makeaddr(&un->raddr, un->raddr_len);
> else if (!UNIX_ADDR_EMPTY(un->laddr_len))
> @@ -295,6 +286,18 @@ static int unix_restore_connected(struct ckpt_ctx *ctx,
> goto out;
> }
>
> + this->sk_peercred.pid = task_tgid_vnr(current);
> +
> + if (may_setuid(ctx->realcred->user->user_ns, un->peercred_uid) &&
> + may_setgid(un->peercred_gid)) {
> + this->sk_peercred.uid = un->peercred_uid;
> + this->sk_peercred.gid = un->peercred_gid;
> + } else {
> + ckpt_debug("peercred %i:%i would require setuid",
> + un->peercred_uid, un->peercred_gid);
> + return -1;
> + }
Ok, except don't you need to do a goto out; here?
Other than that,
Acked-by: Serge Hallyn <serue@us.ibm.com>
> +
> /* Prime the socket's buffer limit with the maximum. These will be
> * overwritten with the values in the checkpoint stream in a later
> * phase.
> --
> 1.6.2.5
>
> _______________________________________________
> Containers mailing list
> Containers@lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/containers
^ permalink raw reply
* Re: [PATCH 2/3] Expose may_setuid() in user.h and add may_setgid() (v2)
From: Serge E. Hallyn @ 2009-08-20 1:31 UTC (permalink / raw)
To: Dan Smith; +Cc: orenl, containers, netdev
In-Reply-To: <1250625435-16299-3-git-send-email-danms@us.ibm.com>
Quoting Dan Smith (danms@us.ibm.com):
> Make these helpers available to others.
>
> Changes in v2:
> - Avoid checking the groupinfo in ctx->realcred against the current in
> may_setgid()
>
> Cc: Serge Hallyn <serue@us.ibm.com>
Acked-by: Serge Hallyn <serue@us.ibm.com>
> Signed-off-by: Dan Smith <danms@us.ibm.com>
> ---
> include/linux/user.h | 9 +++++++++
> kernel/user.c | 13 ++++++++++++-
> 2 files changed, 21 insertions(+), 1 deletions(-)
>
> diff --git a/include/linux/user.h b/include/linux/user.h
> index 68daf84..c231e9c 100644
> --- a/include/linux/user.h
> +++ b/include/linux/user.h
> @@ -1 +1,10 @@
> +#ifndef _LINUX_USER_H
> +#define _LINUX_USER_H
> +
> #include <asm/user.h>
> +#include <linux/sched.h>
> +
> +extern int may_setuid(struct user_namespace *ns, uid_t uid);
> +extern int may_setgid(gid_t gid);
> +
> +#endif
> diff --git a/kernel/user.c b/kernel/user.c
> index a535ed6..a78fde7 100644
> --- a/kernel/user.c
> +++ b/kernel/user.c
> @@ -604,7 +604,7 @@ int checkpoint_user(struct ckpt_ctx *ctx, void *ptr)
> return do_checkpoint_user(ctx, (struct user_struct *) ptr);
> }
>
> -static int may_setuid(struct user_namespace *ns, uid_t uid)
> +int may_setuid(struct user_namespace *ns, uid_t uid)
> {
> /*
> * this next check will one day become
> @@ -631,6 +631,17 @@ static int may_setuid(struct user_namespace *ns, uid_t uid)
> return 0;
> }
>
> +int may_setgid(gid_t gid)
> +{
> + if (capable(CAP_SETGID))
> + return 1;
> +
> + if (in_egroup_p(gid))
> + return 1;
> +
> + return 0;
> +}
> +
> static struct user_struct *do_restore_user(struct ckpt_ctx *ctx)
> {
> struct user_struct *u;
> --
> 1.6.2.5
^ permalink raw reply
* Re: DF Bit set on UDP traffic -- bug or feature?
From: David Miller @ 2009-08-20 4:16 UTC (permalink / raw)
To: gdt; +Cc: netdev
In-Reply-To: <4A8C8945.70602@gdt.id.au>
From: Glen Turner <gdt@gdt.id.au>
Date: Thu, 20 Aug 2009 09:22:45 +1000
> Can I humbly suggest that when the kernel does not implement
> its own fragmentation strategy (as it does with TCP Path MTU
> Discovery) that the DF bit not be set by the kernel?
It's UDP path MTU discovery, it's legal, and Linux has been
doing it for almost 10 years.
I don't think it's changing, sorry.
^ permalink raw reply
* Re: Strange network timeouts w/ 2.6.30.5
From: David Miller @ 2009-08-20 4:17 UTC (permalink / raw)
To: walt; +Cc: linux-kernel, netdev, khc
In-Reply-To: <2087985663.21250727676944.JavaMail.root@mail.holmansrus.com>
From: Walt Holman <walt@holmansrus.com>
Date: Wed, 19 Aug 2009 19:21:16 -0500 (CDT)
Fixing the broken CC: list, it's "netdev" not "linux-netdev".
> Since patching to 2.6.30.5 I'm experiencing periodic timeouts on my e100 which is used as my WAN interface on a server/router box. Nothing is reported in any logs and eventually the traffic resumes. It seems to happen at fairly regular intervals, although I've not timed them. The timeouts last for approx. 60-120 seconds and then traffic resumes normally with no hint of what happened.
>
> Reverting commit 303d67c288319768b19ed8dbed429fef7eb7c275 allows traffic to flow freely again. Attaching .config & lspci, anything else you need?
>
> -Walt
^ permalink raw reply
* Re: [RFC PATCH V2] lib/vsprintf.c: Add struct sockaddr * "%pN<foo>" output
From: Joe Perches @ 2009-08-20 4:24 UTC (permalink / raw)
To: David Miller; +Cc: Chuck Lever, jens, brian.haley, netdev
In-Reply-To: <1250721391.3407.103.camel@Joe-Laptop.home>
On Wed, 2009-08-19 at 15:36 -0700, Joe Perches wrote:
> > Having a hexadecimal port number option
> > might be more useful. The hexadecimal form is also used by the
> > kernel's RPC implementation, fwiw.
>
> Easy enough.
Do you have an opinion on this style patch to lib/vsprint?
Where does it fall on the useless <-> desirable scale?
^ permalink raw reply
* Re: [RFC PATCH V2] lib/vsprintf.c: Add struct sockaddr * "%pN<foo>" output
From: David Miller @ 2009-08-20 4:29 UTC (permalink / raw)
To: joe; +Cc: chuck.lever, jens, brian.haley, netdev
In-Reply-To: <1250742246.2000.4.camel@Joe-Laptop.home>
From: Joe Perches <joe@perches.com>
Date: Wed, 19 Aug 2009 21:24:06 -0700
> On Wed, 2009-08-19 at 15:36 -0700, Joe Perches wrote:
>> > Having a hexadecimal port number option
>> > might be more useful. The hexadecimal form is also used by the
>> > kernel's RPC implementation, fwiw.
>>
>> Easy enough.
>
> Do you have an opinion on this style patch to lib/vsprint?
> Where does it fall on the useless <-> desirable scale?
Who me?
I'm just following this thread loosely, and just plan to review it
on a whole once things seem to quiet down and the major issues
seem to be worked out.
I really have no hard opinion on anything like this, sorry for
the lack of feedback, I simply have none :)
^ permalink raw reply
* Re: PATCH: Network Device Naming mechanism and policy
From: Bryan Kadzban @ 2009-08-20 4:41 UTC (permalink / raw)
To: Jordan_Hargrave; +Cc: netdev, linux-hotplug
In-Reply-To: <5DDAB7BA7BDB58439DD0EED0B8E9A3AE011CD964@ausx3mpc102.aus.amer.dell.com>
[-- Attachment #1: Type: text/plain, Size: 1223 bytes --]
Jordan_Hargrave@Dell.com wrote:
> example udev config:
> SUBSYSTEM=="net", SYMLINK+="net/by-mac/$sysfs{ifindex}.$sysfs{address}"
So say I have two NICs, currently named eth0/eth1, and I keep them using
those names via the current udev MAC address rules. Furthermore, say
that this patch is applied, and so I start using 0.<addr0> and 1.<addr1>
in my network configuration, instead of eth0/eth1.
Now, say that on some given boot, these two NICs show up to the kernel
in a different order. I might move them around in the machine (this is,
after all, the point behind using the MAC as the identifier ;-) ), or
one of them might be USB, or something else random changes the order.
With this rule, they're now at 0.<addr1> and 1.<addr0>.
In other words, these names are not actually persistent.
If you get rid of the $sysfs{ifindex}, then this should work. If that
doesn't work for what you're trying to do for some reason, then you can
make another directory of symlinks by-ifindex, and use that; that should
work as well. But coupling the ifindex to the MAC address like this
doesn't work. (In general, coupling any two unrelated attributes when
trying to do persistent names doesn't work.)
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 260 bytes --]
^ permalink raw reply
* Re: [PATCH] ibm_newemac: emac_close() needs to call netif_carrier_off()
From: Benjamin Herrenschmidt @ 2009-08-20 6:02 UTC (permalink / raw)
To: Petri Gynther; +Cc: shemminger, David Miller, netdev
In-Reply-To: <fc21faff0908191532i2de570a1g1a6674bf5fe64a05@mail.gmail.com>
On Wed, 2009-08-19 at 15:32 -0700, Petri Gynther wrote:
> Stephen,
>
> I think your suggestion of adding netif_running() check to
> bond_check_dev_link() is valid and a good fix to the bonding driver.
> We can do this in a separate patch.
>
> However, I think that the change to ibm_newemac: emac_close() is
> needed as well. ibm_newemac netdevs should not return
> netif_carrier_ok() == TRUE when they have been shut down.
Well, we definitely don't do that in sungem either, since we continue
the link polling while the interface is down... IE. interface up/down is
the data path and is orthogonal to the PHY polling in sungem. I suppose
we -could- stop the polling while the interface is down, though I think
my initial implementation did only poll the link while the interface was
up and that was causing endless problems with various laptop-net style
tools (however that was years and years ago).
Cheers,
Ben.
> On Wed, Aug 19, 2009 at 2:40 PM, Petri Gynther<pgynther@google.com> wrote:
> > I agree with David. And, that's why I propose this diff for
> > ibm_newemac driver as well.
> >
> > On Wed, Aug 19, 2009 at 2:34 PM, David Miller<davem@davemloft.net> wrote:
> >> From: Stephen Hemminger <shemminger@vyatta.com>
> >> Date: Wed, 19 Aug 2009 14:11:36 -0700
> >>
> >>> On Wed, 19 Aug 2009 14:00:00 -0700 (PDT)
> >>> Petri Gynther <pgynther@google.com> wrote:
> >>>
> >>>> When ibm_newemac netdev instance is shutdown with "ifconfig down",
> >>>> the netdev interface does not go properly down. netif_carrier_ok()
> >>>> keeps returning TRUE even after "ifconfig down".
> >>>>
> >>>> The problem can be seen when ibm_newemac instances are slaves of
> >>>> a bonding interface. The bonding interface code uses netif_carrier_ok()
> >>>> to determine the link status of its slaves. When ibm_newemac slave is
> >>>> shutdown with "ifconfig down", the bonding interface won't detect any
> >>>> link status change because netif_carrier_ok() keeps returning TRUE.
> >>>
> >>> Bonding should be testing for netif_running() && netif_carrier_ok().
> >>>
> >>> In many devices state of carrier is undefined when device is down.
> >>
> >> But if you check all of the drivers, ethernet in particular, the
> >> convention is to call netif_carrier_off() in foo_close().
> >>
> >
^ permalink raw reply
* Re: configfs/sysfs
From: Avi Kivity @ 2009-08-20 6:09 UTC (permalink / raw)
To: Nicholas A. Bellinger, Ingo Molnar, Anthony Liguori, kvm,
alacrityvm-devel, linu
In-Reply-To: <20090819221654.GA29503@mail.oracle.com>
On 08/20/2009 01:16 AM, Joel Becker wrote:
>> My high level concern is that we're optimizing for the active
>> sysadmin, not for libraries and management programs. configfs and
>> sysfs are easy to use from the shell, discoverable, and easily
>> scripted. But they discourage documentation, the text format is
>> ambiguous, and they require a lot of boilerplate to use in code.
>>
> I don't think they "discourage documentation" anymore than any
> ioctl we've ever had. At least you can look at the names and values and
> take a good stab at it (configfs is better than sysfs at this, by virtue
> of what it does, but discoverability is certainly not as good as real
> documentation).
> With an ioctl() that isn't (well) documented, you have to go
> read the structure and probably even read the code that uses the
> structure to be sure what you are doing.
>
An ioctl structure and a configfs/sysfs readdir provide similar
information (the structure also provides the types of fields and isn't
able to hide some of these fields).
"Looking at the values" is what I meant by discouraging documentation.
That implies looking at a self-documenting live system. But that tells
you nothing about which fields were added in which versions, or fields
which are hidden because your hardware doesn't support them or because
you didn't echo 1 > somewhere.
>> You could argue that you can wrap *fs in a library that hides the
>> details of accessing it, but that's the wrong approach IMO. We
>> should make the information easy to use and manipulate for programs;
>> one of these programs can be a fuse filesystem for the active
>> sysadmin if someone thinks it's important.
>>
> You are absolutely correct that they are a boon to the sysadmin,
> where in theory programs can do better with binary interfaces. Except
> what programs? I can't do an ioctl or a syscall from a shell script
> (no, using bash's network capabilities to talk to netlink does not
> count). Same with perl/python/whatever where you have to write
> boilerplate to create binary structures.
>
The maintainer of the subsystem should provide a library that talks to
the binary interface and a CLI program that talks to the library.
Boring nonkernely work. Alternatively a fuse filesystem to talk to the
library, or an IDL can replace the library.
> These interfaces have two opposing forces acting on them. They
> provide a reasonably nice way to cross the user<->kernel boundary, so
> people want to use them. Programmatic things, like a power management
> daemon for example, don't want sysadmins touching anything. It's just
> an interface for the daemon.
Many things start oriented at people and then, if they're useful, cross
the lines to machines. You can convert a machine interface to a human
interface at the cost of some work, but it's difficult to undo the
deficiencies of a human oriented interface so it can be used by a program.
> Conversely, some things are really knobs
> for the sysadmin.
I disagree. If it's useful for a human, it's useful for a machine.
Moreover, *fs+bash is a user interface. It happens that bash is good at
processing files, and filesystems are easily discoverable, so we code to
that. But we make it more difficult to provide other interfaces to the
same controls.
> There's nothing else to it. Why should they have to
> code up a C program just to turn a knob?
Many kernel developers believe that userspace is burned into ROM and the
only thing they can change is the kernel. That turns out to be
incorrect. If you don't want users to write C programs to access your
interface, write your own library+CLI. That will have the added benefit
of providing meaningful errors as well ("Invalid argument" vs "frob must
be between 52 and 91"). The program can have a configuration file so
you don't need to reecho the values on boot. It can have a --daemon
mode and do something when an event occurs.
> Configfs, as its name implies,
> really does exist for that second case. It turns out that it's quite
> nice to use for the first case too, but if folks wanted to go the
> syscall route, no worries.
>
Eventually everything is used in the first case. For example in the
virtualization space it is common to have a zillion nodes running
virtual machine that are only accessed by a management node.
> I've said it many times. We will never come up with one
> over-arching solution to all the disparate use cases. Instead, we
> should use each facility - syscalls, ioctls, sysfs, configfs, etc - as
> appropriate. Even in the same program or subsystem.
>
configfs is optional, but sysfs is not. Everything exposed via sysfs
needs to continue to be exposed via sysfs, and new things as well for
consistency. So now if someone wants a syscall interface they must
duplicate the syscall interface, not replace it.
>> - ambiguity
>>
>> What format is the attribute? does it accept lowercase or uppercase
>> hex digits? is there a newline at the end? how many digits can it
>> take before the attribute overflows? All of this has to be
>> documented and checked by the OS, otherwise we risk regressions
>> later. In contrast, __u64 says everything in a binary interface.
>>
> Um, is that __u64 a pointer to a userspace object? A key to a
> lookup table? A file descriptor that is padded out? It's no less
> ambiguous.
>
__u64 says everything about the type and space requirements of a field.
It doesn't describe everything (like the name of the field or what it
means) but it does provide a bunch of boring information that people
rarely document in other ways.
If my program reads a *fs field into a u32 and it later turns out the
field was a u64, I'll get an overflow. It's a lot harder to get that
wrong with a typed interface.
>> - lifetime and access control
>>
>> If a process brings an object into being (using mkdir) and then
>> dies, the object remains behind. The syscall/ioctl approach ties
>> the object into an fd, which will be destroyed when the process
>> dies, and which can be passed around using SCM_RIGHTS, allowing a
>> server process to create and configure an object before passing it
>> to an unprivileged program
>>
> Most things here do *not* want to be tied to the lifetime of one
> process. We don't want our cpu_freq governor changing just because the
> power manager died.
>
Using file descriptors doesn't force you to tie their lifetime to the
fd; it only allows it.
>> You may argue, correctly, that syscalls and ioctls are not as
>> flexible. But this is because no one has invested the effort in
>> making them so. A struct passed as an argument to a syscall is not
>> extensible. But if you pass the size of the structure, and also a
>> bitmap of which attributes are present, you gain extensibility and
>> retain the atomicity property of a syscall interface. I don't think
>> a lot of effort is needed to make an extensible syscall interface
>> just as usable and a lot more efficient than configfs/sysfs. It
>> should also be simple to bolt a fuse interface on top to expose it
>> to us commandline types.
>>
> Your extensible syscall still needs to be known. The
> flexibility provided by configfs and sysfs is of generic access to
> non-generic things. It's different.
> The follow-ups regarding the perf_counter call are a good
> example. If you know the perf_counter call, you can code up a C program
> that asks what attributes or things are there. But if you don't, you've
> first got to find out that there's a perf_counter call, then learn how
> to use it. With configfs/sysfs, you notice that there's now a
> perf_counter directory under a tree, and you can figure out what
> attributes and items are there.
>
Right, that's the great allure of *fs, discoverability. Everything is
at your fingertips. Except if you're writing a program to manage
things. The program can't explore *fs until it's run and usually does
not want to present nongeneric things in a generic way. Ultimately most
of our users are behind programs.
>> configfs is more maintainable that a bunch of hand-maintained
>> ioctls. But if we put some effort into an extendable syscall
>> infrastructure (perhaps to the point of using an IDL) I'm sure we
>> can improve on that without the problems pseudo filesystems
>> introduce.
>>
> Oh, boy, IDL :-) Seriously, if you can solve the "how do I just
> poke around without actually writing C code or installing a
> domain-specific binary" problem, you will probably get somewhere.
>
IDL is very unpleasant to work with but it gets the work done. I don't
see an issue with domain specific binaries (except that you have to
write them). Some say there's the problem of distribution, but if the
kernel distributed itself to the user somehow then the tool can be
distributed just as well (maybe via tools/).
>> I can't really fault a project for using configfs; it's an accepted
>> and recommented (by the community) interface. I'd much prefer it
>> though if there was an effort to create a usable fd/struct based
>> alternative.
>>
> Oh, and configfs was explicitly designed to be interface
> agnostic to the client. The filesystem portions, to the best of my
> ability, are not exposed to client drivers. So you can replace the
> configfs filesystem interface with a system call set that does the same
> operations, and no configfs user will actually need to change their
> code (if you want to change from text values to non-text, that would
> require changing the show/store operation prototypes, but that's about
> it).
>
>
>
But the user visible part is now ABI. I have no issues with the kernel
internals.
--
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
^ permalink raw reply
* Re: [PATCH 2/2] vhost_net: a kernel-level virtio server
From: Rusty Russell @ 2009-08-20 7:25 UTC (permalink / raw)
To: virtualization
Cc: Arnd Bergmann, Michael S. Tsirkin, netdev, Ira W. Snyder,
linux-kernel, kvm
In-Reply-To: <200908131653.47029.arnd@arndb.de>
On Fri, 14 Aug 2009 12:23:46 am Arnd Bergmann wrote:
> On Thursday 13 August 2009, Michael S. Tsirkin wrote:
> > The best way to do this IMO would be to add zero copy support to raw
> > sockets, vhost will then get it basically for free.
>
> Yes, that would be nice. I wonder if that could lead to security
> problems on TX though. I guess It will only bring significant performance
> improvements if we leave the data writable in the user space or guest
> during the operation. If the user finds the right timing, it could
> modify the frame headers after they have been checked using netfilter,
> or while the frames are being consumed in the kernel (e.g. an NFS
> server running in a guest).
For this reason, we always linearize parts of packets we're filtering.
ie. copy.
Cheers,
Rusty.
^ permalink raw reply
* Re: Receive side performance issue with multi-10-GigE and NUMA
From: Bill Fink @ 2009-08-20 7:26 UTC (permalink / raw)
To: Aviv Greenberg; +Cc: Andrew Gallatin, netdev
In-Reply-To: <3000d2e90908141413h3e3eae1dm796f4bf788319561@mail.gmail.com>
On Sat, 15 Aug 2009, Aviv Greenberg wrote:
> > There may be something in the chipset
>
> shooting in the dark: when you lspci -vvv and check the MaxPayload and
> MaxReadReq values for the myri devices - what are the values and are
> they equal? Are they the same on all your platforms?
IIRC, under DevCap they indicated MaxPayload 4096 bytes, and under
DevCtl they indicated MaxPayload 128 bytes and MaxReadReq 4096 bytes,
and was the same on both the Asus and SuperMicro systems. I will
doublecheck tomorrow at work. I am not clear on the meanings of
the different parameters. And is DevCtl for PCI control messages
and DevCap for actual data transfers or something else?
-Thanks
-Bill
^ permalink raw reply
* Re: [Bug #13328] b44: eth0: BUG! Timeout waiting for bit 00000002 of register 42c to clear.
From: Francis Moreau @ 2009-08-20 7:38 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Linux Kernel Mailing List, Kernel Testers List, netdev
In-Reply-To: <Era7U-todKF.A.-rD.DoIjKB@chimera>
Hello,
On Wed, Aug 19, 2009 at 10:40 PM, Rafael J. Wysocki<rjw-KKrjLPT3xs0@public.gmane.org> wrote:
> This message has been generated automatically as a part of a report
> of regressions introduced between 2.6.29 and 2.6.30.
>
> The following bug entry is on the current list of known regressions
> introduced between 2.6.29 and 2.6.30. Please verify if it still should
> be listed and let me know (either way).
>
>
> Bug-Entry : http://bugzilla.kernel.org/show_bug.cgi?id=13328
> Subject : b44: eth0: BUG! Timeout waiting for bit 00000002 of register 42c to clear.
> Submitter : Francis Moreau <francis.moro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Date : 2009-05-03 16:22 (109 days old)
> References : http://marc.info/?l=linux-kernel&m=124136778012280&w=4
>
It hasn't triggered on 2.6.31-rc5 after a couple of days.
--
Francis
^ permalink raw reply
* Re: Receive side performance issue with multi-10-GigE and NUMA
From: Bill Fink @ 2009-08-20 7:50 UTC (permalink / raw)
To: Neil Horman; +Cc: Linux Network Developers, brice, gallatin
In-Reply-To: <20090814232543.GA28599@hmsreliant.think-freely.org>
On Fri, 14 Aug 2009, Neil Horman wrote:
> On Fri, Aug 14, 2009 at 04:44:12PM -0400, Bill Fink wrote:
> > On Fri, 7 Aug 2009, Neil Horman wrote:
> >
> > > On Fri, Aug 07, 2009 at 08:54:42PM -0400, Bill Fink wrote:
> > > > On Fri, 7 Aug 2009, Neil Horman wrote:
> > > >
> > > > > You're timing is impeccable! I just posted a patch for an ftrace module to help
> > > > > detect just these kind of conditions:
> > > > > http://marc.info/?l=linux-netdev&m=124967650218846&w=2
> > > > >
> > > > > Hope that helps you out
> > > > > Neil
> > > >
> > > > Thanks! It could be helpful. Do you have a pointer to documentation
> > > > on how to use it? And does it require the latest GIT kernel or could
> > > > it possibly be used with a 2.6.29.6 kernel?
> > > >
> > > > -Bill
> > >
> > > It should apply to 2.6.29.6 no problem (might take a little massaging, but not
> > > much).
> >
> > It doesn't look like I can apply your patches to my 2.6.29.6 kernel.
> >
> > For starters, there's no include/trace/events directory, so there's
> > no include/trace/events/skb.h. There is an include/trace/skb.h file,
> > but there's no TRACE_EVENT defined anywhere in the kernel.
> >
> > I don't suppose it's as simple as defining (from include/linux/tracepoint.h
> > from Linus's GIT tree):
> >
> > #define PARAMS(args...) args
> >
> > #define TRACE_EVENT(name, proto, args, struct, assign, print) \
> > DECLARE_TRACE(name, PARAMS(proto), PARAMS(args))
> >
> > So do you still think it's reasonable to try applying your patches
> > to my 2.6.29.6 kernel, or should I get a newer kernel like 2.6.30.4
> > or 2.6.31-rc6?
> >
> > -Thanks
> >
> > -Bill
> >
> >
> >
> I thought the trace stuff went it around 2.6.29 but I might be mistaken.
> Easiest thing to do likely would be find where in the tree those were introduced
> and just apply them prior to my patches, or move to the latest kernel if you
> can (at least for the purposes of testing)
I finally got a 2.6.31-rc6 kernel built and had some limited success
with your ftrace patches. Doing some simple ping tests I was able to
verify that everything was mostly as expected regarding CPU and NUMA
memory affinity, with one weird exception. eth2 through eth7, which
all connect to the 5520 I/O Hub that connects to NUMA node 1, all
correctly showed their allocations and consumptions on NUMA node 1.
eth8 through eth13 are all connected to the 5520 I/O Hub that connects
to NUMA node 0, and eth9 through eth13 all correctly reflected that
on the ping ftrace tests. But eth8 showed its allocations being
done on NUMA node 1 instead of the expected NUMA node 0, which just
doesn't make sense since eth8 and eth9 are part of a dual-port 10-GigE
Myricom NIC (and I doublechecked that all the IRQ assignments were
correct).
When I tried an actual nuttcp performance test, even when rate limiting
to just 1 Mbps, I immediately got a kernel oops. I tried to get a
crashdump via kexec/kdump, but the kexec kernel, instead of just
generating a crashdump, fully booted the new kernel, which was
extremely sluggish until I rebooted it through a BIOS re-init,
and never produced a crashdump. I tried this several times and
an immediate kernel oops was always the result (with either a TCP
or UDP test). A ping test of 1000 9000-byte packets with an interval
of 0.001 seconds (which is 72 Mbps for 1 second) on the other hand
worked just fine.
-Thanks
-Bill
^ permalink raw reply
* Re: [PATCHv3 2/2] vhost_net: a kernel-level virtio server
From: Michael S. Tsirkin @ 2009-08-20 8:31 UTC (permalink / raw)
To: Arnd Bergmann
Cc: virtualization, netdev, kvm, linux-kernel, mingo, linux-mm, akpm,
hpa, gregory.haskins, Or Gerlitz
In-Reply-To: <200908191727.07681.arnd@arndb.de>
On Wed, Aug 19, 2009 at 05:27:07PM +0200, Arnd Bergmann wrote:
> On Wednesday 19 August 2009, Michael S. Tsirkin wrote:
> > On Wed, Aug 19, 2009 at 03:46:44PM +0200, Arnd Bergmann wrote:
> > > On Wednesday 19 August 2009, Michael S. Tsirkin wrote:
> > >
> > > Leaving that aside for now, you could replace VHOST_NET_SET_SOCKET,
> > > VHOST_SET_OWNER, VHOST_RESET_OWNER
> >
> > SET/RESET OWNER is still needed: otherwise if you share a descriptor
> > with another process, it can corrupt your memory.
>
> How? The point of using user threads is that you only ever access the
> address space of the thread that called the ioctl.
Think about this example with processes A and B sharing an fd:
A does SET_USED_ADDRESS
B does SET_USED_ADDRESS
A does VHOST_NET_SPLICE
See how stuff gets written into a random place in memory of A?
> > > and your kernel thread with a new
> > > VHOST_NET_SPLICE blocking ioctl that does all the transfers in the
> > > context of the calling thread.
> >
> > For one, you'd want a thread per virtqueue.
>
> Don't understand why. The thread can be blocked on all four ends
> of the device and wake up whenever there is some work do to in
> any direction. If we have data to be transferred in both ways,
> we save one thread switch. It probably won't hurt much to have one
> thread per direction, but I don't see the point.
> > Second, an incoming traffic might arrive on another CPU, we want
> > to keep it local. I guess you would also want ioctls to wake up
> > the threads spuriously ...
>
> Outbound traffic should just stay on whatever CPU was sending it
> from the guest. For inbound traffic, we should only wake up
> the thread on the CPU that got the data to start with.
Exactly. Since we have RX and TX virtqueues, this would mean thread per
direction or virtqueue, same thing. OTOH if RX and TX happen to run on the
same CPU, thread switch would be just a waste of time: but userspace
does not know it, only guest and kernel know.
> Why would I wake up the threads spuriously? Do you mean for
> stopping the transmission or something else? I guess a pthread_kill
> would be enough for shutting it down.
If you kill and restart them you lost priority etc parameters, but maybe.
> > > This would improve the driver on various fronts:
> > >
> > > - no need for playing tricks with use_mm/unuse_mm
> > > - possibly fewer global TLB flushes from switch_mm, which
> > > may improve performance.
> >
> > Why would there be less flushes?
>
> I just read up on task->active_mm handling. There probably wouldn't
> be any. I got that wrong.
>
> > > - based on that, the ability to use any kind of file
> > > descriptor that can do writev/readv or sendmsg/recvmsg
> > > without the nastiness you mentioned.
> >
> > Yes, it's an interesting approach. As I said, need to tread very
> > carefully though, I don't think all issues are figured out. For example:
> > what happens if we pass our own fd here? Will refcount on file ever get
> > to 0 on exit? There may be others ...
>
> right.
>
> > > The disadvantage of course is that you need to add a user
> > > thread for each guest device to make up for the workqueue
> > > that you save.
> >
> > More importantly, you lose control of CPU locality. Simply put, a
> > natural threading model in virtualization is one thread per guest vcpu.
> > Asking applications to add multiple helper threads just so they can
> > block forever is wrong, IMO, as userspace has no idea which CPU
> > they should be on, what priority to use, etc.
>
> But the kernel also doesn't know this, you get the same problem in
> another way. If you have multiple guests running at different priorities,
> the kernel will use those priorities to do the more important transfers
> first, while with a global workqueue every guest gets the same priority.
We could create more threads if this becomes a problem. I just think it
should be transparent to userspace. Possibly it's useful to look at the
packet header as well to decide on priority: this is something userspace
can't do.
> You say that the natural model is to have one thread per guest
> CPU,
Sorry I was not clear. I think userspace should create thread per guest.
We can create as many as we need for networking but I think this should
be behind the scenes, so userspace shouldn't bother with host CPUs, it
will just get it wrong. Think of CPU hotplug, interrupts migrating
between CPUs, etc ...
> but you have a thread per host CPU instead. If the numbers
> are different, you probably lose either way.
The trick I used is to keep as much as possible local
TX done on the CPU that runs the guest,
RX done on the CPU that runs the NIC interrupt.
a smart SMP guest sees which cpu gets interrupts
from NIC and schedules RX there, and it shouldn't matter
if the numbers of CPUs are different.
> It gets worse if you try to apply NUMA policies.
I believe the best we could do is avoid switching CPUs
until we know the actual destination.
> > > > > to
> > > > > avoid some of the implications of kernel threads like the missing
> > > > > ability to handle transfer errors in user space.
> > > >
> > > > Are you talking about TCP here?
> > > > Transfer errors are typically asynchronous - possibly eventfd
> > > > as I expose for vhost net is sufficient there.
> > >
> > > I mean errors in general if we allow random file descriptors to be used.
> > > E.g. tun_chr_aio_read could return EBADFD, EINVAL, EFAULT, ERESTARTSYS,
> > > EIO, EAGAIN and possibly others. We can handle some in kernel, others
> > > should never happen with vhost_net, but if something unexpected happens
> > > it would be nice to just bail out to user space.
> >
> > And note that there might be more than one error. I guess, that's
> > another problem with trying to layer on top of vfs.
>
> Why is that different from any other system call?
With other system calls nothing happens while you process the error.
Here, the guest (other queues) and the network keep running (unless
there is a thread per queue, maybe we can block a queue, but we both
agreed above we don't want that).
> We just return when
> we hit the first error condition.
>
> Arnd <><
If you assume losing the code for the second error condition is OK, why
is the first one so important? That's why I used a counter (eventfd)
per virtqueue, on error userspace can scan the ring and poll the socket
and discover what's wrong, and counter ensures we can detect that error
happened while we were not looking.
--
MST
--
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
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