* Re: [PATCH 09/18] virtio: use avail_event index
From: Tom Lendacky @ 2011-05-17 16:23 UTC (permalink / raw)
To: Rusty Russell
Cc: Michael S. Tsirkin, linux-kernel, Carsten Otte,
Christian Borntraeger, linux390, Martin Schwidefsky,
Heiko Carstens, Shirley Ma, lguest, virtualization, netdev,
linux-s390, kvm, Krishna Kumar, steved, habanero
In-Reply-To: <87ei3zdsq2.fsf@rustcorp.com.au>
On Monday, May 16, 2011 02:12:21 AM Rusty Russell wrote:
> On Sun, 15 May 2011 16:55:41 +0300, "Michael S. Tsirkin" <mst@redhat.com>
wrote:
> > On Mon, May 09, 2011 at 02:03:26PM +0930, Rusty Russell wrote:
> > > On Wed, 4 May 2011 23:51:47 +0300, "Michael S. Tsirkin" <mst@redhat.com>
wrote:
> > > > Use the new avail_event feature to reduce the number
> > > > of exits from the guest.
> > >
> > > Figures here would be nice :)
> >
> > You mean ASCII art in comments?
>
> I mean benchmarks of some kind.
I'm working on getting some benchmark results for the patches. I should
hopefully have something in the next day or two.
Tom
>
> > > > @@ -228,6 +237,12 @@ add_head:
> > > > * new available array entries. */
> > > >
> > > > virtio_wmb();
> > > > vq->vring.avail->idx++;
> > > >
> > > > + /* If the driver never bothers to kick in a very long while,
> > > > + * avail index might wrap around. If that happens, invalidate
> > > > + * kicked_avail index we stored. TODO: make sure all drivers
> > > > + * kick at least once in 2^16 and remove this. */
> > > > + if (unlikely(vq->vring.avail->idx == vq->kicked_avail))
> > > > + vq->kicked_avail_valid = true;
> > >
> > > If they don't, they're already buggy. Simply do:
> > > WARN_ON(vq->vring.avail->idx == vq->kicked_avail);
> >
> > Hmm, but does it say that somewhere?
>
> AFAICT it's a corollary of:
> 1) You have a finite ring of size <= 2^16.
> 2) You need to kick the other side once you've done some work.
>
> > > > @@ -482,6 +517,8 @@ void vring_transport_features(struct
> > > > virtio_device *vdev)
> > > >
> > > > break;
> > > >
> > > > case VIRTIO_RING_F_USED_EVENT_IDX:
> > > > break;
> > > >
> > > > + case VIRTIO_RING_F_AVAIL_EVENT_IDX:
> > > > + break;
> > > >
> > > > default:
> > > > /* We don't understand this bit. */
> > > > clear_bit(i, vdev->features);
> > >
> > > Does this belong in a prior patch?
> > >
> > > Thanks,
> > > Rusty.
> >
> > Well if we don't support the feature in the ring we should not
> > ack the feature, right?
>
> Ah, you're right.
>
> Thanks,
> Rusty.
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH net-2.6] net: use hlist_del_rcu() in dev_change_name()
From: Eric Dumazet @ 2011-05-17 16:21 UTC (permalink / raw)
To: David Miller; +Cc: netdev
Using plain hlist_del() in dev_change_name() is wrong since a
concurrent reader can crash trying to dereference LIST_POISON1.
Bug introduced in commit 72c9528bab94 (net: Introduce
dev_get_by_name_rcu())
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
Stable candidate, for 2.6.33+
net/core/dev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index b624fe4..30a4078 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1007,7 +1007,7 @@ rollback:
}
write_lock_bh(&dev_base_lock);
- hlist_del(&dev->name_hlist);
+ hlist_del_rcu(&dev->name_hlist);
write_unlock_bh(&dev_base_lock);
synchronize_rcu();
^ permalink raw reply related
* [PATCH net-next-2.6 3/3] sfc: Use netif_device_{detach,attach}() around reset and self-test
From: Ben Hutchings @ 2011-05-17 16:06 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-net-drivers
In-Reply-To: <1305647988.2848.14.camel@bwh-desktop>
We need to keep the TX queues stopped throughout a reset, without
triggering the TX watchdog and regardless of the link state. The
proper way to do this is to use netif_device_{detach,attach}() just as
we do around suspend/resume, rather than the current bodge of faking
link-down.
Since we also need to do this during an offline self-test and we
perform a reset during that, add these function calls outside of
efx_reset_down() and efx_reset_up().
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
drivers/net/sfc/efx.c | 9 +++------
drivers/net/sfc/net_driver.h | 4 +---
drivers/net/sfc/selftest.c | 11 +++++------
drivers/net/sfc/tx.c | 9 +++++----
4 files changed, 14 insertions(+), 19 deletions(-)
diff --git a/drivers/net/sfc/efx.c b/drivers/net/sfc/efx.c
index 796c47e..05502b3 100644
--- a/drivers/net/sfc/efx.c
+++ b/drivers/net/sfc/efx.c
@@ -798,11 +798,6 @@ void efx_link_status_changed(struct efx_nic *efx)
if (!netif_running(efx->net_dev))
return;
- if (efx->port_inhibited) {
- netif_carrier_off(efx->net_dev);
- return;
- }
-
if (link_state->up != netif_carrier_ok(efx->net_dev)) {
efx->n_link_state_changes++;
@@ -1450,7 +1445,7 @@ static void efx_start_all(struct efx_nic *efx)
* restart the transmit interface early so the watchdog timer stops */
efx_start_port(efx);
- if (efx_dev_registered(efx) && !efx->port_inhibited)
+ if (efx_dev_registered(efx) && netif_device_present(efx->net_dev))
netif_tx_wake_all_queues(efx->net_dev);
efx_for_each_channel(channel, efx)
@@ -2114,6 +2109,7 @@ int efx_reset(struct efx_nic *efx, enum reset_type method)
netif_info(efx, drv, efx->net_dev, "resetting (%s)\n",
RESET_TYPE(method));
+ netif_device_detach(efx->net_dev);
efx_reset_down(efx, method);
rc = efx->type->reset(efx, method);
@@ -2147,6 +2143,7 @@ out:
efx->state = STATE_DISABLED;
} else {
netif_dbg(efx, drv, efx->net_dev, "reset complete\n");
+ netif_device_attach(efx->net_dev);
}
return rc;
}
diff --git a/drivers/net/sfc/net_driver.h b/drivers/net/sfc/net_driver.h
index 5718260..ce9697b 100644
--- a/drivers/net/sfc/net_driver.h
+++ b/drivers/net/sfc/net_driver.h
@@ -670,13 +670,12 @@ struct efx_filter_state;
* @mtd_list: List of MTDs attached to the NIC
* @nic_data: Hardware dependent state
* @mac_lock: MAC access lock. Protects @port_enabled, @phy_mode,
- * @port_inhibited, efx_monitor() and efx_reconfigure_port()
+ * efx_monitor() and efx_reconfigure_port()
* @port_enabled: Port enabled indicator.
* Serialises efx_stop_all(), efx_start_all(), efx_monitor() and
* efx_mac_work() with kernel interfaces. Safe to read under any
* one of the rtnl_lock, mac_lock, or netif_tx_lock, but all three must
* be held to modify it.
- * @port_inhibited: If set, the netif_carrier is always off. Hold the mac_lock
* @port_initialized: Port initialized?
* @net_dev: Operating system network device. Consider holding the rtnl lock
* @stats_buffer: DMA buffer for statistics
@@ -764,7 +763,6 @@ struct efx_nic {
struct mutex mac_lock;
struct work_struct mac_work;
bool port_enabled;
- bool port_inhibited;
bool port_initialized;
struct net_device *net_dev;
diff --git a/drivers/net/sfc/selftest.c b/drivers/net/sfc/selftest.c
index 50ad3bc..822f6c2 100644
--- a/drivers/net/sfc/selftest.c
+++ b/drivers/net/sfc/selftest.c
@@ -695,12 +695,12 @@ int efx_selftest(struct efx_nic *efx, struct efx_self_tests *tests,
/* Offline (i.e. disruptive) testing
* This checks MAC and PHY loopback on the specified port. */
- /* force the carrier state off so the kernel doesn't transmit during
- * the loopback test, and the watchdog timeout doesn't fire. Also put
- * falcon into loopback for the register test.
+ /* Detach the device so the kernel doesn't transmit during the
+ * loopback test and the watchdog timeout doesn't fire.
*/
+ netif_device_detach(efx->net_dev);
+
mutex_lock(&efx->mac_lock);
- efx->port_inhibited = true;
if (efx->loopback_modes) {
/* We need the 312 clock from the PHY to test the XMAC
* registers, so move into XGMII loopback if available */
@@ -750,12 +750,11 @@ int efx_selftest(struct efx_nic *efx, struct efx_self_tests *tests,
/* restore the PHY to the previous state */
mutex_lock(&efx->mac_lock);
efx->phy_mode = phy_mode;
- efx->port_inhibited = false;
efx->loopback_mode = loopback_mode;
__efx_reconfigure_port(efx);
mutex_unlock(&efx->mac_lock);
- netif_tx_wake_all_queues(efx->net_dev);
+ netif_device_attach(efx->net_dev);
return rc_test;
}
diff --git a/drivers/net/sfc/tx.c b/drivers/net/sfc/tx.c
index d2c85df..84eb99e 100644
--- a/drivers/net/sfc/tx.c
+++ b/drivers/net/sfc/tx.c
@@ -205,7 +205,9 @@ netdev_tx_t efx_enqueue_skb(struct efx_tx_queue *tx_queue, struct sk_buff *skb)
goto unwind;
}
smp_mb();
- netif_tx_start_queue(tx_queue->core_txq);
+ if (likely(!efx->loopback_selftest))
+ netif_tx_start_queue(
+ tx_queue->core_txq);
}
insert_ptr = tx_queue->insert_count & tx_queue->ptr_mask;
@@ -338,8 +340,7 @@ netdev_tx_t efx_hard_start_xmit(struct sk_buff *skb,
struct efx_tx_queue *tx_queue;
unsigned index, type;
- if (unlikely(efx->port_inhibited))
- return NETDEV_TX_BUSY;
+ EFX_WARN_ON_PARANOID(!netif_device_present(net_dev));
index = skb_get_queue_mapping(skb);
type = skb->ip_summed == CHECKSUM_PARTIAL ? EFX_TXQ_TYPE_OFFLOAD : 0;
@@ -436,7 +437,7 @@ void efx_xmit_done(struct efx_tx_queue *tx_queue, unsigned int index)
smp_mb();
if (unlikely(netif_tx_queue_stopped(tx_queue->core_txq)) &&
likely(efx->port_enabled) &&
- likely(!efx->port_inhibited)) {
+ likely(netif_device_present(efx->net_dev))) {
fill_level = tx_queue->insert_count - tx_queue->read_count;
if (fill_level < EFX_TXQ_THRESHOLD(efx)) {
EFX_BUG_ON_PARANOID(!efx_dev_registered(efx));
--
1.7.4
--
Ben Hutchings, Senior Software Engineer, Solarflare
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 net-next-2.6 2/3] sfc: Fix TX queue numbering when separate_tx_channels=1
From: Ben Hutchings @ 2011-05-17 16:05 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-net-drivers
In-Reply-To: <1305647988.2848.14.camel@bwh-desktop>
This option appears to have been broken by commit
8313aca38b3937947fffebca6e34bac8e24300c8 ('sfc: Allocate each channel
separately, along with its RX and TX queues').
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
drivers/net/sfc/efx.c | 12 ++++++++++++
1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/drivers/net/sfc/efx.c b/drivers/net/sfc/efx.c
index 38a55e9..796c47e 100644
--- a/drivers/net/sfc/efx.c
+++ b/drivers/net/sfc/efx.c
@@ -1319,8 +1319,20 @@ static void efx_remove_interrupts(struct efx_nic *efx)
static void efx_set_channels(struct efx_nic *efx)
{
+ struct efx_channel *channel;
+ struct efx_tx_queue *tx_queue;
+
efx->tx_channel_offset =
separate_tx_channels ? efx->n_channels - efx->n_tx_channels : 0;
+
+ /* We need to adjust the TX queue numbers if we have separate
+ * RX-only and TX-only channels.
+ */
+ efx_for_each_channel(channel, efx) {
+ efx_for_each_channel_tx_queue(tx_queue, channel)
+ tx_queue->queue -= (efx->tx_channel_offset *
+ EFX_TXQ_TYPES);
+ }
}
static int efx_probe_nic(struct efx_nic *efx)
--
1.7.4
--
Ben Hutchings, Senior Software Engineer, Solarflare
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 net-next-2.6 1/3] sfc: Fix return value from efx_ethtool_set_rx_ntuple()
From: Ben Hutchings @ 2011-05-17 16:04 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-net-drivers
In-Reply-To: <1305647988.2848.14.camel@bwh-desktop>
ethtool_ops::set_rx_ntuple is supposed to return 0 on success, but it
currently returns the filter ID when it inserts or modifies a filter.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
drivers/net/sfc/ethtool.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/net/sfc/ethtool.c b/drivers/net/sfc/ethtool.c
index 8c5e005..348437a 100644
--- a/drivers/net/sfc/ethtool.c
+++ b/drivers/net/sfc/ethtool.c
@@ -955,8 +955,9 @@ static int efx_ethtool_set_rx_ntuple(struct net_device *net_dev,
if (ntuple->fs.action == ETHTOOL_RXNTUPLE_ACTION_CLEAR)
return efx_filter_remove_filter(efx, &filter);
- else
- return efx_filter_insert_filter(efx, &filter, true);
+
+ rc = efx_filter_insert_filter(efx, &filter, true);
+ return rc < 0 ? rc : 0;
}
static int efx_ethtool_get_rxfh_indir(struct net_device *net_dev,
--
1.7.4
--
Ben Hutchings, Senior Software Engineer, Solarflare
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
* pull request: sfc-next-2.6 2011-05-17
From: Ben Hutchings @ 2011-05-17 15:59 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-net-drivers
The following changes since commit 7be799a70ba3dd90a59e8d2c72bbe06020005b3f:
ipv4: Remove rt->rt_dst reference from ip_forward_options(). (2011-05-13 17:31:02 -0400)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/bwh/sfc-next-2.6.git master
Miscellaneous bug fixes.
Ben.
Ben Hutchings (3):
sfc: Fix return value from efx_ethtool_set_rx_ntuple()
sfc: Fix TX queue numbering when separate_tx_channels=1
sfc: Use netif_device_{detach,attach}() around reset and self-test
drivers/net/sfc/efx.c | 21 +++++++++++++++------
drivers/net/sfc/ethtool.c | 5 +++--
drivers/net/sfc/net_driver.h | 4 +---
drivers/net/sfc/selftest.c | 11 +++++------
drivers/net/sfc/tx.c | 9 +++++----
5 files changed, 29 insertions(+), 21 deletions(-)
--
Ben Hutchings, Senior Software Engineer, Solarflare
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] net: ping: fix build failure
From: Randy Dunlap @ 2011-05-17 15:41 UTC (permalink / raw)
To: Vasiliy Kulikov
Cc: David Miller, randy.dunlap, sfr, netdev, linux-next, linux-kernel
In-Reply-To: <20110517101656.GA28685@albatros>
On Tue, 17 May 2011 14:16:56 +0400 Vasiliy Kulikov wrote:
> On Mon, May 16, 2011 at 15:38 -0400, David Miller wrote:
> > From: Randy Dunlap <randy.dunlap@oracle.com>
> > Date: Mon, 16 May 2011 12:35:34 -0700
> >
> > > On Mon, 16 May 2011 15:10:19 +1000 Stephen Rothwell wrote:
> > > when CONFIG_PROC_SYSCTL is not enabled:
> > >
> > > ping.c:(.text+0x52af3): undefined reference to `inet_get_ping_group_range_net'
> >
> > Vasiliy, please fix this.
>
> I wonder whether there is any way to test such unusual configurations?
> Only randconfig or are there any (partly-)automated tools for it?
>
>
> [PATCH] net: ping: fix build failure
>
> If CONFIG_PROC_SYSCTL=n the building process fails:
>
> ping.c:(.text+0x52af3): undefined reference to `inet_get_ping_group_range_net'
>
> Moved inet_get_ping_group_range_net() to ping.c.
>
> Reported-by: Randy Dunlap <randy.dunlap@oracle.com>
> Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>
Acked-by: Randy Dunlap <randy.dunlap@oracle.com>
Thanks.
> ---
> include/net/ping.h | 2 --
> net/ipv4/ping.c | 13 +++++++++++++
> net/ipv4/sysctl_net_ipv4.c | 12 ------------
> 3 files changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/include/net/ping.h b/include/net/ping.h
> index 23062c3..682b5ae 100644
> --- a/include/net/ping.h
> +++ b/include/net/ping.h
> @@ -44,8 +44,6 @@ extern struct proto ping_prot;
> extern void ping_rcv(struct sk_buff *);
> extern void ping_err(struct sk_buff *, u32 info);
>
> -extern void inet_get_ping_group_range_net(struct net *net, unsigned int *low, unsigned int *high);
> -
> #ifdef CONFIG_PROC_FS
> extern int __init ping_proc_init(void);
> extern void ping_proc_exit(void);
> diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
> index 7041d09..3635975 100644
> --- a/net/ipv4/ping.c
> +++ b/net/ipv4/ping.c
> @@ -188,6 +188,19 @@ exit:
> return sk;
> }
>
> +static void inet_get_ping_group_range_net(struct net *net, gid_t *low, gid_t *high)
> +{
> + gid_t *data = net->ipv4.sysctl_ping_group_range;
> + unsigned seq;
> + do {
> + seq = read_seqbegin(&sysctl_local_ports.lock);
> +
> + *low = data[0];
> + *high = data[1];
> + } while (read_seqretry(&sysctl_local_ports.lock, seq));
> +}
> +
> +
> static int ping_init_sock(struct sock *sk)
> {
> struct net *net = sock_net(sk);
> diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
> index 28e8273..57d0752 100644
> --- a/net/ipv4/sysctl_net_ipv4.c
> +++ b/net/ipv4/sysctl_net_ipv4.c
> @@ -73,18 +73,6 @@ static int ipv4_local_port_range(ctl_table *table, int write,
> }
>
>
> -void inet_get_ping_group_range_net(struct net *net, gid_t *low, gid_t *high)
> -{
> - gid_t *data = net->ipv4.sysctl_ping_group_range;
> - unsigned seq;
> - do {
> - seq = read_seqbegin(&sysctl_local_ports.lock);
> -
> - *low = data[0];
> - *high = data[1];
> - } while (read_seqretry(&sysctl_local_ports.lock, seq));
> -}
> -
> void inet_get_ping_group_range_table(struct ctl_table *table, gid_t *low, gid_t *high)
> {
> gid_t *data = table->data;
> --
> 1.7.0.4
> --
> To unsubscribe from this list: send the line "unsubscribe linux-next" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
^ permalink raw reply
* Re: [PATCH 0/7] Network namespace manipulation with file descriptors
From: David Lamparter @ 2011-05-17 15:35 UTC (permalink / raw)
To: Eric W. Biederman
Cc: David Lamparter, Alex Bligh, linux-arch, netdev, linux-kernel,
Linux Containers, linux-fsdevel
In-Reply-To: <m1mxil9z2p.fsf@fess.ebiederm.org>
On Tue, May 17, 2011 at 07:33:18AM -0700, Eric W. Biederman wrote:
> I went the round of keeping a daemon open, saw how much code that
> takes and how fragile that can be in the corner cases and decided to
> patch the kernel to make the interfaces better.
Yes, it is more fragile. I'm currently using it without setns-patched
kernels though, so it's all I have.
(More or less related, I was able to cause a kernel oops on doing a
"find /proc" on setns-patched kernels after bind-mounting /proc; I did
not report it because I'm running grsecurity-patched kernels which tend
to mess with /proc quite a bit... I'll try to reproduce it with your
current patches on an otherwise vanilla kernel.)
> > I also have patches for OpenVPN and pptpd floating around that make it
> > possible to 'cross' namespace boundaries, i.e. the VPN servers listen in
> > one namespace and have their devices in another.
>
> For openvpn I have managed to get away with simply using an up script.
> Mostly the script is:
>
> ip netns add $NSNAME || true
> ip netns exec $NSNAME ip link set lo up
> ip link set $dev netns $NSNAME
Historic annotation: This line used to kernel panic around a year and a
half back - nowadays tap devices do get the netns move right...
> ip netns exec $NSNAME ip link set $dev up
> ip netns exec $NSNAME ifconfig $dev $ifconfig_local netmask $ifconfig_netmask broadcast $ifconfig_broadcast
>
> With a few extra bits for dns options and routes. If I had an openvpn
> built with the iproute option I expect I could get away by just wrapping
> iproute. Not that I would mind a patched openvpn.
I didn't even try to make amends for DNS, I just put 127.0.0.1 and have
caches in each of the namespaces. Wrapping iproute2 would work; one of
the advantages of patching OpenVPN is that the OpenVPN daemon is in the
same namespace as the tap devices and can do all the configuration as
usual.
For pptp, er, well, reading up on how ppp devices behave if you splice
them across namespace boundaries would've taken more time than patching
up the UDP/GRE sockets.
> Personally I think using a vpn in a network namespace seems like a
> killer feature.
Yes, it very much is - it provides very nice and clean solutions to
problems that up to now were usually hacked around with policy routing &
co.
-David
^ permalink raw reply
* Re: [PATCH V5 4/6 net-next] vhost: vhost TX zero-copy support
From: Shirley Ma @ 2011-05-17 15:34 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: David Miller, Eric Dumazet, Avi Kivity, Arnd Bergmann, netdev,
kvm, linux-kernel
In-Reply-To: <20110517152840.GA2389@redhat.com>
On Tue, 2011-05-17 at 18:28 +0300, Michael S. Tsirkin wrote:
> Which is the order the descriptors are put on avail ring.
> By design, guest should not depend on used ring entries
> being in order with avail ring (and btw with virtio block,
> they aren't). If it does, it's a bug I think.
Ok, I thought, the order should be maintained.
> > The original code has no problem, because it gets one head and pass
> that
> > head to vhost_add_used one by one once done the copy. So it's in
> > sequence.
> >
> > This issue can easily recreate without zerocopy patch by simply
> changing
> > the order from "head return vhost_get_vq_desc" when passing to
> > vhost_add_used.
> >
> > Thanks
> > Shirley
>
> Ah, did you try that? Could you post this patch pls?
> This seems to imply a bug in guest virtio.
I am creating the patch against net-next for you to test today.
Thanks
Shirley
^ permalink raw reply
* Re: [PATCH V5 4/6 net-next] vhost: vhost TX zero-copy support
From: Michael S. Tsirkin @ 2011-05-17 15:28 UTC (permalink / raw)
To: Shirley Ma
Cc: David Miller, Eric Dumazet, Avi Kivity, Arnd Bergmann, netdev,
kvm, linux-kernel
In-Reply-To: <1305645734.10756.14.camel@localhost.localdomain>
On Tue, May 17, 2011 at 08:22:14AM -0700, Shirley Ma wrote:
> On Tue, 2011-05-17 at 08:55 +0300, Michael S. Tsirkin wrote:
> > Something in your patch that overwrites the id in vhost
> > and makes it put the wrong id in the used ring?
> >
> > By the way, need to keep in mind that a guest can
> > give us the same head twice, need to make sure this
> > at least does not corrupt host memory.
>
> I think I didn't explain the problem very well here.
>
> This patch doesn't overwrite the id. It just keeps the same coming
> sequence from "head return vhost_get_vq_desc()" to pass to
> vhost_add_used.
>
> The same ids can be used many times once it passes to guest from
> vhost_add_used. There is no problem. The zero copy patch doesn't have
> any issue.
>
> The problem is the order of head from return vhost_get_vq_desc should be
> in sequence when it passes to vhost_add_used.
Which is the order the descriptors are put on avail ring.
By design, guest should not depend on used ring entries
being in order with avail ring (and btw with virtio block,
they aren't). If it does, it's a bug I think.
> The original code has no problem, because it gets one head and pass that
> head to vhost_add_used one by one once done the copy. So it's in
> sequence.
>
> This issue can easily recreate without zerocopy patch by simply changing
> the order from "head return vhost_get_vq_desc" when passing to
> vhost_add_used.
>
> Thanks
> Shirley
Ah, did you try that? Could you post this patch pls?
This seems to imply a bug in guest virtio.
--
MST
^ permalink raw reply
* Re: [PATCH V5 4/6 net-next] vhost: vhost TX zero-copy support
From: Shirley Ma @ 2011-05-17 15:22 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: David Miller, Eric Dumazet, Avi Kivity, Arnd Bergmann, netdev,
kvm, linux-kernel
In-Reply-To: <20110517055503.GA26989@redhat.com>
On Tue, 2011-05-17 at 08:55 +0300, Michael S. Tsirkin wrote:
> Something in your patch that overwrites the id in vhost
> and makes it put the wrong id in the used ring?
>
> By the way, need to keep in mind that a guest can
> give us the same head twice, need to make sure this
> at least does not corrupt host memory.
I think I didn't explain the problem very well here.
This patch doesn't overwrite the id. It just keeps the same coming
sequence from "head return vhost_get_vq_desc()" to pass to
vhost_add_used.
The same ids can be used many times once it passes to guest from
vhost_add_used. There is no problem. The zero copy patch doesn't have
any issue.
The problem is the order of head from return vhost_get_vq_desc should be
in sequence when it passes to vhost_add_used.
The original code has no problem, because it gets one head and pass that
head to vhost_add_used one by one once done the copy. So it's in
sequence.
This issue can easily recreate without zerocopy patch by simply changing
the order from "head return vhost_get_vq_desc" when passing to
vhost_add_used.
Thanks
Shirley
^ permalink raw reply
* Re: [PATCH] net: tuntap: Fix tun_net_fix_features()
From: Michael S. Tsirkin @ 2011-05-17 15:11 UTC (permalink / raw)
To: Michał Mirosław; +Cc: netdev, Herbert Xu, Ben Hutchings, Shan Wei
In-Reply-To: <20110517150029.GA23179@rere.qmqm.pl>
On Tue, May 17, 2011 at 05:00:29PM +0200, Michał Mirosław wrote:
> On Tue, May 17, 2011 at 05:54:28PM +0300, Michael S. Tsirkin wrote:
> > On Tue, May 17, 2011 at 04:46:35PM +0200, Michał Mirosław wrote:
> > > On Tue, May 17, 2011 at 05:29:43PM +0300, Michael S. Tsirkin wrote:
> > > > On Tue, May 17, 2011 at 10:19:54AM +0200, Michał Mirosław wrote:
> > > > > tun->set_features are meant to limit not force the features.
> [...]
> > > > One thing that this will do though: previously, if
> > > > ethtool disables offloads, then an application enables
> > > > them, the application will have the last say.
> > > > With this patch, the most conservative approach wins.
> > > > Right?
> > >
> > > Exactly.
> > >
> > > On device creation, wanted_features default to all offloads
> > > enabled, so unless an admin changes the flags, the application controls
> > > what is enabled. This matters only when using persistent tun/tap and
> > > admin and user are two different people. If the admin is using queues
> > > and doesn't want to handle e.g. TSO packets (I'm not sure if they are
> > > properly accounted in all queuing disciplines), then the feature should
> > > not be enabled by user.
> [...]
> > Yes, with virtualization admin and the app are two different people
> > usually. The device doesn't have to be persistent though I think -
> > what limits this to persistent devices?
>
> Hmm. Nothing really. I just forgot about the virtualization case. You
> usually will change the offloads just after device creation unless you're
> testing or debugging something.
That's true. kvm invokes a user script after creating device
but just before configuring it, if there might be a problem
it's likely only because of something such a script might do
(which used to be harmless). My gut feeling is this
is unlikely.
> > I agree this behaviour seems more consistent, I just hope this change
> > does not break any setups.
>
> The only effect would be some performance drop on cases, where admin turned
> off the offloads and they stay like that regardless of what user part does.
>
> Best Regards,
> Michał Mirosław
The performance drop is actually quite drastic :), but yes it
will keep going, which is a good thing.
--
MST
^ permalink raw reply
* Re: [PATCH] net: tuntap: Fix tun_net_fix_features()
From: Michał Mirosław @ 2011-05-17 15:00 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, Herbert Xu, Ben Hutchings, Shan Wei
In-Reply-To: <20110517145428.GA1472@redhat.com>
On Tue, May 17, 2011 at 05:54:28PM +0300, Michael S. Tsirkin wrote:
> On Tue, May 17, 2011 at 04:46:35PM +0200, Michał Mirosław wrote:
> > On Tue, May 17, 2011 at 05:29:43PM +0300, Michael S. Tsirkin wrote:
> > > On Tue, May 17, 2011 at 10:19:54AM +0200, Michał Mirosław wrote:
> > > > tun->set_features are meant to limit not force the features.
[...]
> > > One thing that this will do though: previously, if
> > > ethtool disables offloads, then an application enables
> > > them, the application will have the last say.
> > > With this patch, the most conservative approach wins.
> > > Right?
> >
> > Exactly.
> >
> > On device creation, wanted_features default to all offloads
> > enabled, so unless an admin changes the flags, the application controls
> > what is enabled. This matters only when using persistent tun/tap and
> > admin and user are two different people. If the admin is using queues
> > and doesn't want to handle e.g. TSO packets (I'm not sure if they are
> > properly accounted in all queuing disciplines), then the feature should
> > not be enabled by user.
[...]
> Yes, with virtualization admin and the app are two different people
> usually. The device doesn't have to be persistent though I think -
> what limits this to persistent devices?
Hmm. Nothing really. I just forgot about the virtualization case. You
usually will change the offloads just after device creation unless you're
testing or debugging something.
> I agree this behaviour seems more consistent, I just hope this change
> does not break any setups.
The only effect would be some performance drop on cases, where admin turned
off the offloads and they stay like that regardless of what user part does.
Best Regards,
Michał Mirosław
^ permalink raw reply
* Re: [PATCH] net: ping: fix build failure
From: Randy Dunlap @ 2011-05-17 14:58 UTC (permalink / raw)
To: Vasiliy Kulikov; +Cc: David Miller, sfr, netdev, linux-next, linux-kernel
In-Reply-To: <20110517101656.GA28685@albatros>
On 05/17/11 03:16, Vasiliy Kulikov wrote:
> On Mon, May 16, 2011 at 15:38 -0400, David Miller wrote:
>> From: Randy Dunlap <randy.dunlap@oracle.com>
>> Date: Mon, 16 May 2011 12:35:34 -0700
>>
>>> On Mon, 16 May 2011 15:10:19 +1000 Stephen Rothwell wrote:
>>> when CONFIG_PROC_SYSCTL is not enabled:
>>>
>>> ping.c:(.text+0x52af3): undefined reference to `inet_get_ping_group_range_net'
>>
>> Vasiliy, please fix this.
>
> I wonder whether there is any way to test such unusual configurations?
> Only randconfig or are there any (partly-)automated tools for it?
Hi,
I do automated (cron) randconfigs nightly and occasionally I run a script
that builds a kernel with each one of these options disabled (although
lately CONFIG_PM is not being disabled, but I'll fix that one day):
BLOCK HOTPLUG INET INPUT MAGIC_SYSRQ NET PCI PM PROC_FS SMP SYSFS
Hm, I should add SYSCTL (and/or PROC_SYSCTL) to that list. It often has issues.
--
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
^ permalink raw reply
* Re: [PATCH] net: tuntap: Fix tun_net_fix_features()
From: Michael S. Tsirkin @ 2011-05-17 14:54 UTC (permalink / raw)
To: Michał Mirosław; +Cc: netdev, Herbert Xu, Ben Hutchings, Shan Wei
In-Reply-To: <20110517144635.GA22878@rere.qmqm.pl>
On Tue, May 17, 2011 at 04:46:35PM +0200, Michał Mirosław wrote:
> On Tue, May 17, 2011 at 05:29:43PM +0300, Michael S. Tsirkin wrote:
> > On Tue, May 17, 2011 at 10:19:54AM +0200, Michał Mirosław wrote:
> > > tun->set_features are meant to limit not force the features.
> > >
> > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > > ---
> > > drivers/net/tun.c | 2 +-
> > > 1 files changed, 1 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > > index 74e9405..f77c6d0 100644
> > > --- a/drivers/net/tun.c
> > > +++ b/drivers/net/tun.c
> > > @@ -458,7 +458,7 @@ static u32 tun_net_fix_features(struct net_device *dev, u32 features)
> > > {
> > > struct tun_struct *tun = netdev_priv(dev);
> > >
> > > - return (features & tun->set_features) | (features & ~TUN_USER_FEATURES);
> > > + return features & (tun->set_features | ~TUN_USER_FEATURES);
> > > }
> > >
> > > static const struct net_device_ops tun_netdev_ops = {
> > > --
> > > 1.7.2.5
> >
> > One thing that this will do though: previously, if
> > ethtool disables offloads, then an application enables
> > them, the application will have the last say.
> > With this patch, the most conservative approach wins.
> > Right?
>
> Exactly.
>
> On device creation, wanted_features default to all offloads
> enabled, so unless an admin changes the flags, the application controls
> what is enabled. This matters only when using persistent tun/tap and
> admin and user are two different people. If the admin is using queues
> and doesn't want to handle e.g. TSO packets (I'm not sure if they are
> properly accounted in all queuing disciplines), then the feature should
> not be enabled by user.
>
> > If we want to have the existing behaviour
> > I think the following would do this (untested). What do you think?
> >
> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > index 74e9405..1d6c7bc 100644
> > --- a/drivers/net/tun.c
> > +++ b/drivers/net/tun.c
> > @@ -1199,6 +1199,8 @@ static int set_offload(struct tun_struct *tun, unsigned long arg)
> > return -EINVAL;
> >
> > tun->set_features = features;
> > + tun->dev->features &= TUN_USER_FEATURES;
> > + tun->dev->features |= (features & TUN_USER_FEATURES);
> > netdev_update_features(tun->dev);
>
> tun->dev->features will be recalculated by netdev_update_features()
> anyway. For this to work as you described it would need to alter
> wanted_features. I don't like the idea that something other than one
> of ethtool_ops is changing this field, as it then becomes something
> else that what the admin wants (even if that is not what he gets).
>
> Best Regards,
> Michał Mirosław
Yes, with virtualization admin and the app are two different people
usually. The device doesn't have to be persistent though I think -
what limits this to persistent devices?
I agree this behaviour seems more consistent, I just hope this change
does not break any setups.
--
MST
^ permalink raw reply
* Re: [PATCH] net: tuntap: Fix tun_net_fix_features()
From: Michał Mirosław @ 2011-05-17 14:46 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, Herbert Xu, Ben Hutchings, Shan Wei
In-Reply-To: <20110517142943.GA926@redhat.com>
On Tue, May 17, 2011 at 05:29:43PM +0300, Michael S. Tsirkin wrote:
> On Tue, May 17, 2011 at 10:19:54AM +0200, Michał Mirosław wrote:
> > tun->set_features are meant to limit not force the features.
> >
> > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > ---
> > drivers/net/tun.c | 2 +-
> > 1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > index 74e9405..f77c6d0 100644
> > --- a/drivers/net/tun.c
> > +++ b/drivers/net/tun.c
> > @@ -458,7 +458,7 @@ static u32 tun_net_fix_features(struct net_device *dev, u32 features)
> > {
> > struct tun_struct *tun = netdev_priv(dev);
> >
> > - return (features & tun->set_features) | (features & ~TUN_USER_FEATURES);
> > + return features & (tun->set_features | ~TUN_USER_FEATURES);
> > }
> >
> > static const struct net_device_ops tun_netdev_ops = {
> > --
> > 1.7.2.5
>
> One thing that this will do though: previously, if
> ethtool disables offloads, then an application enables
> them, the application will have the last say.
> With this patch, the most conservative approach wins.
> Right?
Exactly.
On device creation, wanted_features default to all offloads
enabled, so unless an admin changes the flags, the application controls
what is enabled. This matters only when using persistent tun/tap and
admin and user are two different people. If the admin is using queues
and doesn't want to handle e.g. TSO packets (I'm not sure if they are
properly accounted in all queuing disciplines), then the feature should
not be enabled by user.
> If we want to have the existing behaviour
> I think the following would do this (untested). What do you think?
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 74e9405..1d6c7bc 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1199,6 +1199,8 @@ static int set_offload(struct tun_struct *tun, unsigned long arg)
> return -EINVAL;
>
> tun->set_features = features;
> + tun->dev->features &= TUN_USER_FEATURES;
> + tun->dev->features |= (features & TUN_USER_FEATURES);
> netdev_update_features(tun->dev);
tun->dev->features will be recalculated by netdev_update_features()
anyway. For this to work as you described it would need to alter
wanted_features. I don't like the idea that something other than one
of ethtool_ops is changing this field, as it then becomes something
else that what the admin wants (even if that is not what he gets).
Best Regards,
Michał Mirosław
^ permalink raw reply
* [PATCH v3 1/1] igmp: call ip_mc_clear_src() only when we have no users of ip_mc_list
From: Veaceslav Falico @ 2011-05-17 14:37 UTC (permalink / raw)
To: David Stevens
Cc: David Miller, jmorris, kaber, kuznet, linux-kbuild, linux-kernel,
mmarek, netdev, pekkas, yoshfuji
In-Reply-To: <20110517133801.GC30366@darkmag.usersys.redhat.com>
In igmp_group_dropped() we call ip_mc_clear_src(), which resets the number
of source filters per mulitcast. However, igmp_group_dropped() is also
called on NETDEV_DOWN, NETDEV_PRE_TYPE_CHANGE and NETDEV_UNREGISTER, which
means that the group might get added back on NETDEV_UP, NETDEV_REGISTER and
NETDEV_POST_TYPE_CHANGE respectively, leaving us with broken source
filters.
To fix that, we must clear the source filters only when there are no users
in the ip_mc_list, i.e. in ip_mc_dec_group().
Correct version of the patch.
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
diff --git a/net/ipv4/igmp.c b/net/ipv4/igmp.c
index 1fd3d9c..142ca0d 100644
--- a/net/ipv4/igmp.c
+++ b/net/ipv4/igmp.c
@@ -1169,20 +1169,18 @@ static void igmp_group_dropped(struct ip_mc_list *im)
if (!in_dev->dead) {
if (IGMP_V1_SEEN(in_dev))
- goto done;
+ return;
if (IGMP_V2_SEEN(in_dev)) {
if (reporter)
igmp_send_report(in_dev, im, IGMP_HOST_LEAVE_MESSAGE);
- goto done;
+ return;
}
/* IGMPv3 */
igmpv3_add_delrec(in_dev, im);
igmp_ifc_event(in_dev);
}
-done:
#endif
- ip_mc_clear_src(im);
}
static void igmp_group_added(struct ip_mc_list *im)
@@ -1319,6 +1317,7 @@ void ip_mc_dec_group(struct in_device *in_dev, __be32 addr)
*ip = i->next_rcu;
in_dev->mc_count--;
igmp_group_dropped(i);
+ ip_mc_clear_src(i);
if (!in_dev->dead)
ip_rt_multicast_event(in_dev);
^ permalink raw reply related
* Re: [PATCH 0/7] Network namespace manipulation with file descriptors
From: Eric W. Biederman @ 2011-05-17 14:33 UTC (permalink / raw)
To: David Lamparter
Cc: Alex Bligh, linux-arch, netdev, linux-kernel, Linux Containers,
linux-fsdevel
In-Reply-To: <20110517111148.GA3762520@jupiter.n2.diac24.net>
David Lamparter <equinox@diac24.net> writes:
> On Sat, May 07, 2011 at 07:18:44AM -0700, Eric W. Biederman wrote:
>> You can read the processes network namespace by opening
>> /proc/<pid>/ns/net. Unfortunately comparing the network
>> namespaces for identity is another matter. You will probably
>> be better off simply forcing the routing daemon to start
>> in the desired network namespace in it's initscript.
>>
>> For purposes of clarity please have a look at my work in
>> progress patch for iproute2. This demonstrates how I expect
>> userspace to work in a multi-network namespace world.
>>
> [...]
>> Subject: [PATCH] iproute2: Add processless netnwork namespace support.
> [...]
>> Configuration specific to a network namespace that
>> would ordinarily be stored under /etc/ is stored under
>> /etc/netns/<name>. For example if the dns server
>> configuration is different for your vpn you would
>> create a file /etc/netns/myvpn/resolv.conf.
>>
>> File descriptors that can be used to manipulate a
>> network namespace can be created by opening
>> /var/run/netns/<NAME>.
>>
>> This adds the following commands to iproute.
>> ip netns add NAME
>> ip netns delete NAME
>> ip netns monitor
>> ip netns list
>> ip netns exec NAME cmd ....
>> ip link set DEV netns NAME
>
> funny, this is almost exactly what my code does - though you're probably
> doing it better and have more features ;)
Well if it has more features it is only because I have managed to keep
everything simple enough that adding features was easy. I ignored all
of the hard bits.
> http://git.spaceboyz.net/equinox/vrf-tools.git/
> git://spaceboyz.net/equinox/vrf-tools.git
>
> It currently forks off a daemon to keep the namespace open; attaching is
> not possible yet, but opening a socket in a different namespace is.
I went the round of keeping a daemon open, saw how much code that
takes and how fragile that can be in the corner cases and decided to
patch the kernel to make the interfaces better.
> Most of the actual management (mounting things & co.) I offloaded to
> some shell scripts; I use it together with GNU screen (which makes it
> very nice to grab one of the namespaces and start/stop/manage/...
> things).
That does sound like a nice way of doing the management.
> I also have patches for OpenVPN and pptpd floating around that make it
> possible to 'cross' namespace boundaries, i.e. the VPN servers listen in
> one namespace and have their devices in another.
For openvpn I have managed to get away with simply using an up script.
Mostly the script is:
ip netns add $NSNAME || true
ip netns exec $NSNAME ip link set lo up
ip link set $dev netns $NSNAME
ip netns exec $NSNAME ip link set $dev up
ip netns exec $NSNAME ifconfig $dev $ifconfig_local netmask $ifconfig_netmask broadcast $ifconfig_broadcast
With a few extra bits for dns options and routes. If I had an openvpn
built with the iproute option I expect I could get away by just wrapping
iproute. Not that I would mind a patched openvpn.
Personally I think using a vpn in a network namespace seems like a
killer feature.
Eric
^ permalink raw reply
* Re: [PATCH] net: tuntap: Fix tun_net_fix_features()
From: Michael S. Tsirkin @ 2011-05-17 14:29 UTC (permalink / raw)
To: Michał Mirosław; +Cc: netdev, Herbert Xu, Ben Hutchings, Shan Wei
In-Reply-To: <20110517081954.A227013A6A@rere.qmqm.pl>
On Tue, May 17, 2011 at 10:19:54AM +0200, Michał Mirosław wrote:
> tun->set_features are meant to limit not force the features.
>
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
> drivers/net/tun.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 74e9405..f77c6d0 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -458,7 +458,7 @@ static u32 tun_net_fix_features(struct net_device *dev, u32 features)
> {
> struct tun_struct *tun = netdev_priv(dev);
>
> - return (features & tun->set_features) | (features & ~TUN_USER_FEATURES);
> + return features & (tun->set_features | ~TUN_USER_FEATURES);
> }
>
> static const struct net_device_ops tun_netdev_ops = {
> --
> 1.7.2.5
One thing that this will do though: previously, if
ethtool disables offloads, then an application enables
them, the application will have the last say.
With this patch, the most conservative approach wins.
Right?
If we want to have the existing behaviour
I think the following would do this (untested). What do you think?
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 74e9405..1d6c7bc 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1199,6 +1199,8 @@ static int set_offload(struct tun_struct *tun, unsigned long arg)
return -EINVAL;
tun->set_features = features;
+ tun->dev->features &= TUN_USER_FEATURES;
+ tun->dev->features |= (features & TUN_USER_FEATURES);
netdev_update_features(tun->dev);
return 0;
^ permalink raw reply related
* Re: pull request: wireless-next-2.6 2011-05-16-v2
From: Gustavo F. Padovan @ 2011-05-17 14:13 UTC (permalink / raw)
To: David Miller; +Cc: linville, linux-wireless, netdev
In-Reply-To: <20110516.230835.989629139579226478.davem@davemloft.net>
* David Miller <davem@davemloft.net> [2011-05-16 23:08:35 -0400]:
> From: "John W. Linville" <linville@tuxdriver.com>
> Date: Mon, 16 May 2011 20:08:09 -0400
>
> > I'm sorry, Dave! In my defense, I am on the verge of sickness and got
> > very little sleep last night... :-(
> >
> > I have corrected the problem and was sure to perform the correct build
> > test this time! :-)
>
> So these hunks made to net/bluetooth/l2cap_core.c:
>
> @@ -3745,11 +3758,14 @@ done:
> static inline int l2cap_conless_channel(struct l2cap_conn *conn, __le16 psm, struct sk_buff *skb)
> {
> struct sock *sk;
> + struct l2cap_chan *chan;
>
> - sk = l2cap_get_sock_by_psm(0, psm, conn->src);
> - if (!sk)
> + chan = l2cap_global_chan_by_psm(0, psm, conn->src);
> + if (!chan)
> goto drop;
>
> + sk = chan->sk;
> +
> bh_lock_sock(sk);
>
> BT_DBG("sk %p, len %d", sk, skb->len);
> @@ -3745,11 +3758,14 @@ done:
> static inline int l2cap_conless_channel(struct l2cap_conn *conn, __le16 psm, struct sk_buff *skb)
> {
> struct sock *sk;
> + struct l2cap_chan *chan;
>
> - sk = l2cap_get_sock_by_psm(0, psm, conn->src);
> - if (!sk)
> + chan = l2cap_global_chan_by_psm(0, psm, conn->src);
> + if (!chan)
> goto drop;
>
> + sk = chan->sk;
> +
> bh_lock_sock(sk);
>
> BT_DBG("sk %p, len %d", sk, skb->len);
>
> Doesn't generate the following warnings on your compiler?
>
> net/bluetooth/l2cap_core.c: In function ‘l2cap_recv_frame’:
> net/bluetooth/l2cap_core.c:3758:15: warning: ‘sk’ may be used uninitialized in this function
> net/bluetooth/l2cap_core.c:3758:15: note: ‘sk’ was declared here
> net/bluetooth/l2cap_core.c:3791:15: warning: ‘sk’ may be used uninitialized in this function
> net/bluetooth/l2cap_core.c:3791:15: note: ‘sk’ was declared here
>
I keep not seeing these warnings. I'll upgrade to gcc 4.6 and check this again.
padovan bluetooth-next-2.6 $ rm net/bluetooth/*.o net/bluetooth/*.ko
padovan bluetooth-next-2.6 $ make
CHK include/linux/version.h
CHK include/generated/utsrelease.h
UPD include/generated/utsrelease.h
CALL scripts/checksyscalls.sh
CHK include/generated/compile.h
UPD include/generated/compile.h
CC init/version.o
CC kernel/module.o
LD init/built-in.o
CC kernel/kexec.o
LD kernel/built-in.o
LD net/bluetooth/built-in.o
CC [M] net/bluetooth/af_bluetooth.o
CC [M] net/bluetooth/hci_core.o
CC [M] net/bluetooth/hci_conn.o
CC [M] net/bluetooth/hci_event.o
CC [M] net/bluetooth/mgmt.o
CC [M] net/bluetooth/hci_sock.o
CC [M] net/bluetooth/hci_sysfs.o
CC [M] net/bluetooth/lib.o
CC [M] net/bluetooth/l2cap_core.o
CC [M] net/bluetooth/l2cap_sock.o
CC [M] net/bluetooth/sco.o
LD [M] net/bluetooth/bluetooth.o
LD vmlinux.o
--
Gustavo F. Padovan
http://profusion.mobi
^ permalink raw reply
* bonding flaps between member interfaces
From: Patrick Schaaf @ 2011-05-17 13:27 UTC (permalink / raw)
To: netdev
Dear netdev,
I'm experiencing a regression with bonding. Bugzilla and cursory
searching of the list did not immediately show up anything that seems
related, so here's the report:
Short summary: bonding flips between members every second
bonding in active-backup mode with ARP monitoring
two members in the bond, both being VLAN interfaces on top of two
separate ethernet interfaces
bnx2 ethernet driver, but saw the same behaviour with a tigon box
concrete settings:
BONDING_MODULE_OPTS="mode=active-backup primary=eth0.24 arp_interval=250
arp_ip_target=192.168.x.x"
See below for a /proc/net/bonding/bond24 output reflecing the
configuration.
This setup I have in production on 2.6.36.2, and it works fine.
It also works fine, tested today, with 2.6.36,4 and 2.6.37.6
Starting with 2.6.38 (2.6.38.6 tested today), and still happening with
2.6.39-rc7, I experience problems. While I can still work over the
interface, it is flipping once per second between the two member
interfaces. There is no indication of the underlying interface going
up/down, but bonding seems to think so.
See below an excerpt of the kernel log for two back-and-forth flapping
cycles.
In /proc/net/bonding/bond24, I see the failure counter of the configured
primary interface counting up with each flap. The counter of the non
primary interface does not move. When I switch the primary interface by
echoing to /sys, the behaviour of the counters flips: always the
configured primary has the counter going up.
best regards
Patrick
Here is /proc/net/bonding/bond24 while running on 2.6.37.6, to show the
concrete configuration from this POV. Everything looks the same with the
failing kernels, except for the noted behaviour of the Failure Counts.
Ethernet Channel Bonding Driver: v3.7.0 (June 2, 2010)
Bonding Mode: fault-tolerance (active-backup)
Primary Slave: eth0.24 (primary_reselect always)
Currently Active Slave: eth0.24
MII Status: up
MII Polling Interval (ms): 0
Up Delay (ms): 0
Down Delay (ms): 0
ARP Polling Interval (ms): 250
ARP IP target/s (n.n.n.n form): 192.168.x.x
Slave Interface: eth0.24
MII Status: up
Speed: 1000 Mbps
Duplex: full
Link Failure Count: 0
Permanent HW addr: d4:85:64:ca:1c:12
Slave queue ID: 0
Slave Interface: eth1.24
MII Status: up
Speed: 1000 Mbps
Duplex: full
Link Failure Count: 0
Permanent HW addr: d4:85:64:ca:1c:14
Slave queue ID: 0
Here is kernel log output for two flapping cycles (booted kernel was
2.6.39-rc7):
May 17 14:58:22 myserver kernel: [ 1016.629155] bonding: bond24: link
status definitely down for interface eth0.24, disabling it
May 17 14:58:22 myserver kernel: [ 1016.629159] bonding: bond24: making
interface eth1.24 the new active one.
May 17 14:58:22 myserver kernel: [ 1016.629162] device eth0.24 left
promiscuous mode
May 17 14:58:22 myserver kernel: [ 1016.629164] device eth0 left
promiscuous mode
May 17 14:58:22 myserver kernel: [ 1016.629191] device eth1.24 entered
promiscuous mode
May 17 14:58:22 myserver kernel: [ 1016.629193] device eth1 entered
promiscuous mode
May 17 14:58:22 myserver kernel: [ 1016.878596] bonding: bond24: link
status definitely up for interface eth0.24.
May 17 14:58:22 myserver kernel: [ 1016.878600] bonding: bond24: making
interface eth0.24 the new active one.
May 17 14:58:22 myserver kernel: [ 1016.878603] device eth1.24 left
promiscuous mode
May 17 14:58:22 myserver kernel: [ 1016.878605] device eth1 left
promiscuous mode
May 17 14:58:22 myserver kernel: [ 1016.878631] device eth0.24 entered
promiscuous mode
May 17 14:58:22 myserver kernel: [ 1016.878633] device eth0 entered
promiscuous mode
May 17 14:58:23 myserver kernel: [ 1017.626919] bonding: bond24: link
status definitely down for interface eth0.24, disabling it
May 17 14:58:23 myserver kernel: [ 1017.626923] bonding: bond24: making
interface eth1.24 the new active one.
May 17 14:58:23 myserver kernel: [ 1017.626926] device eth0.24 left
promiscuous mode
May 17 14:58:23 myserver kernel: [ 1017.626928] device eth0 left
promiscuous mode
May 17 14:58:23 myserver kernel: [ 1017.626955] device eth1.24 entered
promiscuous mode
May 17 14:58:23 myserver kernel: [ 1017.626957] device eth1 entered
promiscuous mode
May 17 14:58:23 myserver kernel: [ 1017.876359] bonding: bond24: link
status definitely up for interface eth0.24.
May 17 14:58:23 myserver kernel: [ 1017.876363] bonding: bond24: making
interface eth0.24 the new active one.
May 17 14:58:23 myserver kernel: [ 1017.876366] device eth1.24 left
promiscuous mode
May 17 14:58:23 myserver kernel: [ 1017.876368] device eth1 left
promiscuous mode
May 17 14:58:23 myserver kernel: [ 1017.876394] device eth0.24 entered
promiscuous mode
May 17 14:58:23 myserver kernel: [ 1017.876396] device eth0 entered
promiscuous mode
^ permalink raw reply
* [PATCH v2 1/1] igmp: call ip_mc_clear_src() only when we have no users of ip_mc_list
From: Veaceslav Falico @ 2011-05-17 13:38 UTC (permalink / raw)
To: David Stevens
Cc: David Miller, jmorris, kaber, kuznet, linux-kbuild, linux-kernel,
mmarek, netdev, pekkas, yoshfuji
In-Reply-To: <OF948388D5.90B3F5A7-ON88257892.006C3228-88257892.0071B9D5@us.ibm.com>
In igmp_group_dropped() we call ip_mc_clear_src(), which resets the number
of source filters per mulitcast. However, igmp_group_dropped() is also
called on NETDEV_DOWN, NETDEV_PRE_TYPE_CHANGE and NETDEV_UNREGISTER, which
means that the group might get added back on NETDEV_UP, NETDEV_REGISTER and
NETDEV_POST_TYPE_CHANGE respectively, leaving us with broken source
filters.
To fix that, we must clear the source filters only when there are no users
in the ip_mc_list, i.e. in ip_mc_dec_group().
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
diff --git a/net/ipv4/igmp.c b/net/ipv4/igmp.c
index 1fd3d9c..732e30b 100644
--- a/net/ipv4/igmp.c
+++ b/net/ipv4/igmp.c
@@ -1182,7 +1182,6 @@ static void igmp_group_dropped(struct ip_mc_list *im)
}
done:
#endif
- ip_mc_clear_src(im);
}
static void igmp_group_added(struct ip_mc_list *im)
@@ -1319,6 +1318,7 @@ void ip_mc_dec_group(struct in_device *in_dev, __be32 addr)
*ip = i->next_rcu;
in_dev->mc_count--;
igmp_group_dropped(i);
+ ip_mc_clear_src(i);
if (!in_dev->dead)
ip_rt_multicast_event(in_dev);
^ permalink raw reply related
* Re: WARNING, net/core/dev.c, skb_gso_segment, 2.6.39-rc7-git11 inclusive
From: Denys Fedoryshchenko @ 2011-05-17 13:31 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1305638195.2850.97.camel@edumazet-laptop>
On Tue, 17 May 2011 15:16:35 +0200, Eric Dumazet wrote:
>
> Hi Denys
>
> net/sched/act_csum.c sets skb->ip_summed = CHECKSUM_NONE; after csum
> changes.
>
> and skb_gso_segment() barfs if skb->ip_summed != CHECKSUM_PARTIAL
>
> You could disable GRO for the time being.
It seems mistake from my side, seems, csum was not being called, i
forgot to add pipe before csum.
^ permalink raw reply
* Re: [PATCH 1/1] igmp: fix ip_mc_clear_src to not reset ip_mc_list->sf{mode,count}
From: Veaceslav Falico @ 2011-05-17 13:30 UTC (permalink / raw)
To: David Stevens
Cc: David Miller, jmorris, kaber, kuznet, linux-kbuild, linux-kernel,
mmarek, netdev, pekkas, yoshfuji
In-Reply-To: <OF948388D5.90B3F5A7-ON88257892.006C3228-88257892.0071B9D5@us.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 3249 bytes --]
On Mon, May 16, 2011 at 01:42:11PM -0700, David Stevens wrote:
>
> On NETDEV_DOWN, all group memberships are dropped.
> ip_mc_clear_src()
> is simply freeing all the source filters and turning it into an "EXCLUDE
> nobody"
> membership (ie, the same as an ordinary join without source filtering).
> This
> ordinarily happens when you are deleting the group entirely (when the
> reference
> count goes to 0), but is also called on device down.
> This patch is not appropriate; when the groups are deleted, the
> source
> filters are deleted, and the filter counts have to reflect the source
> filters
> in the list. If you had an "INCLUDE A" filter, for example, that would
> become
> an "INCLUDE nobody" filter and drop all traffic (from A or not). The
> number
> of source filters is not related to the number of listener sockets, and
> the
> function of ip_mc_clear_src() is to make it 0 (with the special case of 1
> for
> EXCLUDE), so setting the counts has to be done for proper functioning.
> I don't quite understand the problem you're trying to solve here
> --
> when the device comes back up, the group should be re-added with
> {EXCLUDE,nobody} and
> ip_check_mc() should therefore return 1. Of course, while the interface is
> down, the mc_list is empty and it'd return 0 in that case.
> Do you have a small test program to demonstrate the problem?
Yes, attached are two programs, one sender and one receiver, they bind both
to localhost and send each other traffic. To reproduce, start the sender
and two instances of receivers, then do an ifconfig lo up; ifconfig lo
down;, restart the sender program (both of the receivers should once again
receive the multicast traffic). Then kill one receiver (the MCAST_EXCLUDE
will become 0), and do an "ip route flush cache". The new route cache will
be without the local flag on, and the remaining receiver will stop
receiving traffic.
What happens:
1) When both receivers start, ip_mc_list->sfcount[MCAST_EXCLUDE] == 2
2) On NETDEV_DOWN event, groups are dropped and sfmode = MCAST_EXCLUDE,
sfcount[MCAST_EXCLUDE] = 1
3) On NETDEV_UP, the group is re-joined, but kernel thinks that there's
only one listener (sfcount[MCAST_EXCLUDE]).
4) On socket destroy (when one receiver is terminated), the count is 0.
5) On route cache flush, __mkroute_output() doesn't see the remaining
listener, and creates a route cache without RTCF_LOCAL flag, thus not
allowing any traffic on that group to local listeners.
The igmp_group_dropped() (the actual routine that drops a group) is called
when:
1) ip_mc_dec_group() is called and im->users == 0
2) ip_mc_unmap()
3) ip_mc_down()
4) ip_mc_destroy_dev()
The 1) we call either on socket destroy or when the socket actually asks to
leave a group. In this case, we need to "reset" the state on no listeners.
2),3),4) are called on various device modifications
(NETDEV_PRE_TYPE_CHANGE, NETDEV_DOWN and NETDEV_UNREGISTER) - but the group
can be rejoined on their next events - NETDEV_POST_TYPE_CHANGE, NETDEV_UP
and NETDEV_REGISTER, which will cause the ip_mc_list to loose track of
existing listeners.
So, I tend to think that we must clear the sources only on 1).
Will send the patch shortly.
Thank you!
[-- Attachment #2: mcsend.c --]
[-- Type: text/plain, Size: 3595 bytes --]
#include <sys/types.h> /* for type definitions */
#include <sys/socket.h> /* for socket API function calls */
#include <netinet/in.h> /* for address structs */
#include <arpa/inet.h> /* for sockaddr_in */
#include <stdio.h> /* for printf() */
#include <stdlib.h> /* for atoi() */
#include <string.h> /* for strlen() */
#include <unistd.h> /* for close() */
#define MAX_LEN 1024 /* maximum string size to send */
#define MIN_PORT 1024 /* minimum port allowed */
#define MAX_PORT 65535 /* maximum port allowed */
int main(int argc, char *argv[]) {
int sock; /* socket descriptor */
char send_str[MAX_LEN]; /* string to send */
struct sockaddr_in mc_addr; /* socket address structure */
unsigned int send_len; /* length of string to send */
char* mc_addr_str; /* multicast IP address */
unsigned short mc_port; /* multicast port */
unsigned char mc_ttl=1; /* time to live (hop count) */
/* validate number of arguments */
if (argc != 3) {
fprintf(stderr,
"Usage: %s <Multicast IP> <Multicast Port>\n",
argv[0]);
exit(1);
}
mc_addr_str = argv[1]; /* arg 1: multicast IP address */
mc_port = atoi(argv[2]); /* arg 2: multicast port number */
/* validate the port range */
if ((mc_port < MIN_PORT) || (mc_port > MAX_PORT)) {
fprintf(stderr, "Invalid port number argument %d.\n",
mc_port);
fprintf(stderr, "Valid range is between %d and %d.\n",
MIN_PORT, MAX_PORT);
exit(1);
}
/* create a socket for sending to the multicast address */
if ((sock = socket(PF_INET, SOCK_DGRAM, IPPROTO_UDP)) < 0) {
perror("socket() failed");
exit(1);
}
/* bind socket to "localhost" */
struct sockaddr_in lh_addr; /* socket address structure */
memset(&lh_addr, 0, sizeof(lh_addr));
lh_addr.sin_family = AF_INET;
lh_addr.sin_addr.s_addr = inet_addr("127.0.0.1");
lh_addr.sin_port = 0;
if ((bind(sock, (struct sockaddr *) &lh_addr,
sizeof(lh_addr))) < 0) {
perror("bind() failed");
exit(1);
}
/* enable loopback (should be the default) */
int mc_loopback = 1;
if ((setsockopt(sock, IPPROTO_IP, IP_MULTICAST_LOOP,
(void*) &mc_loopback, sizeof(mc_loopback))) < 0) {
perror("setsockopt(IP_MULTICAST_LOOP) failed");
exit(1);
}
/* set interface address */
if ((setsockopt(sock, IPPROTO_IP, IP_MULTICAST_IF,
(void*) &lh_addr.sin_addr, sizeof(lh_addr.sin_addr))) < 0) {
perror("setsockopt(IP_MULTICAST_IF) failed");
exit(1);
}
/* set the TTL (time to live/hop count) for the send
if ((setsockopt(sock, IPPROTO_IP, IP_MULTICAST_TTL,
(void*) &mc_ttl, sizeof(mc_ttl))) < 0) {
perror("setsockopt() failed");
exit(1);
} */
/* construct a multicast address structure */
memset(&mc_addr, 0, sizeof(mc_addr));
mc_addr.sin_family = AF_INET;
mc_addr.sin_addr.s_addr = inet_addr(mc_addr_str);
mc_addr.sin_port = htons(mc_port);
printf("Begin typing (return to send, ctrl-C to quit):\n");
/* clear send buffer */
memset(send_str, 0, sizeof(send_str));
while (fgets(send_str, MAX_LEN, stdin)) {
send_len = strlen(send_str);
/* send string to multicast address */
if ((sendto(sock, send_str, send_len, 0,
(struct sockaddr *) &mc_addr,
sizeof(mc_addr))) != send_len) {
perror("sendto() sent incorrect number of bytes");
exit(1);
}
/* clear send buffer */
memset(send_str, 0, sizeof(send_str));
}
close(sock);
exit(0);
}
[-- Attachment #3: mcreceive.c --]
[-- Type: text/plain, Size: 3821 bytes --]
#include <sys/types.h> /* for type definitions */
#include <sys/socket.h> /* for socket API calls */
#include <netinet/in.h> /* for address structs */
#include <arpa/inet.h> /* for sockaddr_in */
#include <stdio.h> /* for printf() and fprintf() */
#include <stdlib.h> /* for atoi() */
#include <string.h> /* for strlen() */
#include <unistd.h> /* for close() */
#define MAX_LEN 1024 /* maximum receive string size */
#define MIN_PORT 1024 /* minimum port allowed */
#define MAX_PORT 65535 /* maximum port allowed */
int main(int argc, char *argv[]) {
int sock; /* socket descriptor */
int flag_on = 1; /* socket option flag */
struct sockaddr_in mc_addr; /* socket address structure */
char recv_str[MAX_LEN+1]; /* buffer to receive string */
int recv_len; /* length of string received */
struct ip_mreq mc_req; /* multicast request structure */
char* mc_addr_str; /* multicast IP address */
unsigned short mc_port; /* multicast port */
struct sockaddr_in from_addr; /* packet source */
unsigned int from_len; /* source addr length */
unsigned int fl=1;
/* validate number of arguments */
if (argc != 3) {
fprintf(stderr,
"Usage: %s <Multicast IP> <Multicast Port>\n",
argv[0]);
exit(1);
}
mc_addr_str = argv[1]; /* arg 1: multicast ip address */
mc_port = atoi(argv[2]); /* arg 2: multicast port number */
/* validate the port range */
if ((mc_port < MIN_PORT) || (mc_port > MAX_PORT)) {
fprintf(stderr, "Invalid port number argument %d.\n",
mc_port);
fprintf(stderr, "Valid range is between %d and %d.\n",
MIN_PORT, MAX_PORT);
exit(1);
}
/* create socket to join multicast group on */
if ((sock = socket(PF_INET, SOCK_DGRAM, IPPROTO_UDP)) < 0) {
perror("socket() failed");
exit(1);
}
/* set reuse port to on to allow multiple binds per host */
if ((setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &flag_on,
sizeof(flag_on))) < 0) {
perror("setsockopt() failed");
exit(1);
}
/* construct a multicast address structure */
memset(&mc_addr, 0, sizeof(mc_addr));
mc_addr.sin_family = AF_INET;
// mc_addr.sin_addr.s_addr = htonl(INADDR_ANY);
mc_addr.sin_addr.s_addr = inet_addr(mc_addr_str);
mc_addr.sin_port = htons(mc_port);
/* bind to multicast address to socket */
if ((bind(sock, (struct sockaddr *) &mc_addr,
sizeof(mc_addr))) < 0) {
perror("bind() failed");
exit(1);
}
/* construct an IGMP join request structure */
mc_req.imr_multiaddr.s_addr = inet_addr(mc_addr_str);
// mc_req.imr_interface.s_addr = htonl(INADDR_ANY);
mc_req.imr_interface.s_addr = inet_addr("127.0.0.1");
/* send an ADD MEMBERSHIP message via setsockopt */
if (fl && (setsockopt(sock, IPPROTO_IP, IP_ADD_MEMBERSHIP,
(void*) &mc_req, sizeof(mc_req))) < 0) {
perror("setsockopt() failed");
exit(1);
}
for (;;) { /* loop forever */
/* clear the receive buffers & structs */
memset(recv_str, 0, sizeof(recv_str));
from_len = sizeof(from_addr);
memset(&from_addr, 0, from_len);
/* block waiting to receive a packet */
if ((recv_len = recvfrom(sock, recv_str, MAX_LEN, 0,
(struct sockaddr*)&from_addr, &from_len)) < 0) {
perror("recvfrom() failed");
exit(1);
}
/* output received string */
printf("Received %d bytes from %s: ", recv_len,
inet_ntoa(from_addr.sin_addr));
printf("%s", recv_str);
}
/* send a DROP MEMBERSHIP message via setsockopt */
if ((setsockopt(sock, IPPROTO_IP, IP_DROP_MEMBERSHIP,
(void*) &mc_req, sizeof(mc_req))) < 0) {
perror("setsockopt() failed");
exit(1);
}
close(sock);
}
^ permalink raw reply
* Re: WARNING, net/core/dev.c, skb_gso_segment, 2.6.39-rc7-git11 inclusive
From: Denys Fedoryshchenko @ 2011-05-17 13:25 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1305638195.2850.97.camel@edumazet-laptop>
>>
>> --
>
> Hi Denys
>
> net/sched/act_csum.c sets skb->ip_summed = CHECKSUM_NONE; after csum
> changes.
>
> and skb_gso_segment() barfs if skb->ip_summed != CHECKSUM_PARTIAL
>
> You could disable GRO for the time being.
Thanks a lot, i will, it is just test.
By the way, is it a bug and it will be fixed, or pedit is not intended
to be (ab)used like that?
^ 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