* [PATCH 16/17] netfilter: Deletion of unnecessary checks before two function calls
From: Pablo Neira Ayuso @ 2014-11-24 13:27 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev
In-Reply-To: <1416835674-11871-1-git-send-email-pablo@netfilter.org>
From: Markus Elfring <elfring@users.sourceforge.net>
The functions free_percpu() and module_put() test whether their argument
is NULL and then return immediately. Thus the test around the call is
not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
Acked-by: Julian Anastasov <ja@ssi.bg>
Acked-by: Simon Horman <horms@verge.net.au>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/netfilter/ipvs/ip_vs_ctl.c | 3 +--
net/netfilter/ipvs/ip_vs_pe.c | 3 +--
net/netfilter/ipvs/ip_vs_sched.c | 3 +--
net/netfilter/ipvs/ip_vs_sync.c | 3 +--
net/netfilter/nf_tables_api.c | 3 +--
5 files changed, 5 insertions(+), 10 deletions(-)
diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c
index ac7ba68..b8295a4 100644
--- a/net/netfilter/ipvs/ip_vs_ctl.c
+++ b/net/netfilter/ipvs/ip_vs_ctl.c
@@ -465,8 +465,7 @@ __ip_vs_bind_svc(struct ip_vs_dest *dest, struct ip_vs_service *svc)
static void ip_vs_service_free(struct ip_vs_service *svc)
{
- if (svc->stats.cpustats)
- free_percpu(svc->stats.cpustats);
+ free_percpu(svc->stats.cpustats);
kfree(svc);
}
diff --git a/net/netfilter/ipvs/ip_vs_pe.c b/net/netfilter/ipvs/ip_vs_pe.c
index 1a82b29..0df17ca 100644
--- a/net/netfilter/ipvs/ip_vs_pe.c
+++ b/net/netfilter/ipvs/ip_vs_pe.c
@@ -37,8 +37,7 @@ struct ip_vs_pe *__ip_vs_pe_getbyname(const char *pe_name)
rcu_read_unlock();
return pe;
}
- if (pe->module)
- module_put(pe->module);
+ module_put(pe->module);
}
rcu_read_unlock();
diff --git a/net/netfilter/ipvs/ip_vs_sched.c b/net/netfilter/ipvs/ip_vs_sched.c
index 4dbcda6..199760c 100644
--- a/net/netfilter/ipvs/ip_vs_sched.c
+++ b/net/netfilter/ipvs/ip_vs_sched.c
@@ -104,8 +104,7 @@ static struct ip_vs_scheduler *ip_vs_sched_getbyname(const char *sched_name)
mutex_unlock(&ip_vs_sched_mutex);
return sched;
}
- if (sched->module)
- module_put(sched->module);
+ module_put(sched->module);
}
mutex_unlock(&ip_vs_sched_mutex);
diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
index 7162c86..c47ffd7 100644
--- a/net/netfilter/ipvs/ip_vs_sync.c
+++ b/net/netfilter/ipvs/ip_vs_sync.c
@@ -820,8 +820,7 @@ ip_vs_conn_fill_param_sync(struct net *net, int af, union ip_vs_sync_conn *sc,
p->pe_data = kmemdup(pe_data, pe_data_len, GFP_ATOMIC);
if (!p->pe_data) {
- if (p->pe->module)
- module_put(p->pe->module);
+ module_put(p->pe->module);
return -ENOMEM;
}
p->pe_data_len = pe_data_len;
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 1ffb253..18a9dae 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -3674,8 +3674,7 @@ static int nf_tables_abort(struct sk_buff *skb)
break;
case NFT_MSG_NEWCHAIN:
if (nft_trans_chain_update(trans)) {
- if (nft_trans_chain_stats(trans))
- free_percpu(nft_trans_chain_stats(trans));
+ free_percpu(nft_trans_chain_stats(trans));
nft_trans_destroy(trans);
} else {
--
1.7.10.4
^ permalink raw reply related
* [PATCH net-next] tipc: add tipc_netlink.h to uapi Kbuild
From: richard.alpe @ 2014-11-24 13:24 UTC (permalink / raw)
To: netdev; +Cc: Richard Alpe
From: Richard Alpe <richard.alpe@ericsson.com>
tipc_netlink.h is the user-space header for the new netlink api. It
was accidentally left out of the uapi Kbuild list when the api was
added.
Signed-off-by: Richard Alpe <richard.alpe@ericsson.com>
---
include/uapi/linux/Kbuild | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
index 72298b6..a1e8175 100644
--- a/include/uapi/linux/Kbuild
+++ b/include/uapi/linux/Kbuild
@@ -389,6 +389,7 @@ header-y += times.h
header-y += timex.h
header-y += tiocl.h
header-y += tipc_config.h
+header-y += tipc_netlink.h
header-y += tipc.h
header-y += toshiba.h
header-y += tty_flags.h
--
2.1.3
^ permalink raw reply related
* net_tx_action race condition?
From: Angelo Rizzi @ 2014-11-24 13:29 UTC (permalink / raw)
To: netdev
[-- Attachment #1: Type: text/plain, Size: 936 bytes --]
Hi Daniel,
Here attached the patch file you required.
The problem i've found is on the declaration of 'struct softnet_data
*sd' in function 'net_tx_action'
What happens to me (i have an embedded system based on FPGA and a NIOS2
cpu) is that, due to compiler optimization, the content of
'sd->completion_queue' is saved in a CPU register before interrupt
disabling (when the instruction 'if (sd->completion_queue) {' is
executed) and then the register contents is used for interrupt-disabled
assignment ('clist = sd->completion_queue') instead of re-read the
variable contents.
This seems to lead to a race condition when an interrupt modifies the
content of 'sd->completion_queue' between these two instructions.
What i have done to avoid this situation is to change the declaration of
'struct softnet_data *sd' to 'volatile struct softnet_data *sd' and now
everything seems to be ok.
I hope this will help.
Regards,
Angelo
[-- Attachment #2: net_tx_action_patch --]
[-- Type: text/plain, Size: 438 bytes --]
--- linux-2.6.32.64/net/core/dev.c.orig 2014-11-24 12:52:36.103335551 +0100
+++ linux-2.6.32.64/net/core/dev.c 2014-11-24 12:52:58.863949715 +0100
@@ -2069,7 +2069,7 @@ EXPORT_SYMBOL(netif_rx_ni);
static void net_tx_action(struct softirq_action *h)
{
- struct softnet_data *sd = &__get_cpu_var(softnet_data);
+ volatile struct softnet_data *sd = &__get_cpu_var(softnet_data);
if (sd->completion_queue) {
struct sk_buff *clist;
^ permalink raw reply
* Re: [patch net-next] tc_vlan: fix type of tcfv_push_vid
From: Jiri Pirko @ 2014-11-24 13:33 UTC (permalink / raw)
To: Jamal Hadi Salim; +Cc: netdev, davem
In-Reply-To: <547331E0.6030305@mojatatu.com>
Mon, Nov 24, 2014 at 02:25:52PM CET, jhs@mojatatu.com wrote:
>On 11/24/14 05:30, Jiri Pirko wrote:
>>Should be u16. So fix it to kill the sparse warning.
>>
>>Fixes: c7e2b9689ef8136 "sched: introduce vlan action"
>>Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>>---
>> include/net/tc_act/tc_vlan.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>>diff --git a/include/net/tc_act/tc_vlan.h b/include/net/tc_act/tc_vlan.h
>>index c809c1d..93b70ad 100644
>>--- a/include/net/tc_act/tc_vlan.h
>>+++ b/include/net/tc_act/tc_vlan.h
>>@@ -18,7 +18,7 @@
>> struct tcf_vlan {
>> struct tcf_common common;
>> int tcfv_action;
>>- __be16 tcfv_push_vid;
>>+ u16 tcfv_push_vid;
>> __be16 tcfv_push_proto;
>
>
>User space sends you this in be, no? I thought it was fine
>the way it is. Isnt __be16 just an annotation?
No, userspace sends this in host. Therefore we should store it in host,
not in be. It is just an annotation.
>
>cheers,
>jamal
>
^ permalink raw reply
* Re: [PATCH net-next v4 0/4] netns: allow to identify peer netns
From: Nicolas Dichtel @ 2014-11-24 13:45 UTC (permalink / raw)
To: Eric W. Biederman
Cc: netdev-u79uwXL29TY76Z2rM5mHXA,
containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, luto-kltTT9wpgjJwATOyAt5JVQ,
stephen-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ,
cwang-xCSkyg8dI+0RB7SZvlqPiA, linux-api-u79uwXL29TY76Z2rM5mHXA,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <871tpph03k.fsf-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org>
Le 30/10/2014 19:41, Eric W. Biederman a écrit :
> Nicolas Dichtel <nicolas.dichtel@6wind.com> writes:
>
>> The goal of this serie is to be able to multicast netlink messages with an
>> attribute that identify a peer netns.
>> This is needed by the userland to interpret some informations contained in
>> netlink messages (like IFLA_LINK value, but also some other attributes in case
>> of x-netns netdevice (see also
>> http://thread.gmane.org/gmane.linux.network/315933/focus=316064 and
>> http://thread.gmane.org/gmane.linux.kernel.containers/28301/focus=4239)).
>>
>> Ids of peer netns are set by userland via a new genl messages. These ids are
>> stored per netns and are local (ie only valid in the netns where they are set).
>> To avoid allocating an int for each peer netns, I use idr_for_each() to retrieve
>> the id of a peer netns. Note that it will be possible to add a table (struct net
>> -> id) later to optimize this lookup if needed.
>>
>> Patch 1/4 introduces the netlink API mechanism to set and get these ids.
>> Patch 2/4 and 3/4 implements an example of how to use these ids in rtnetlink
>> messages. And patch 4/4 shows that the netlink messages can be symetric between
>> a GET and a SET.
>>
>> iproute2 patches are available, I can send them on demand.
>
> A quick reply. I think this patchset is in the right general direction.
> There are some oddball details that seem odd/awkward to me such as using
> genetlink instead of rtnetlink to get and set the ids, and not having
> ids if they are not set (that feels like a maintenance/usability challenge).
>
> I would like to give your patches a deep review, but I won't be able to
> do that for a couple of weeks. I am deep in the process of moving,
> and will be mostly offline until about the Nov 11th.
Eric, did you have a chance to look at this?
Regards,
Nicolas
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/containers
^ permalink raw reply
* Re: [PATCH] ip_tunnel: the lack of vti_link_ops' dellink() cause kernel panic
From: Nicolas Dichtel @ 2014-11-24 14:18 UTC (permalink / raw)
To: Xin Long, network dev, Steffen Klassert; +Cc: Cong Wang
In-Reply-To: <1416726251-13088-1-git-send-email-lucien.xin@gmail.com>
Le 23/11/2014 08:04, Xin Long a écrit :
> Now the vti_link_ops do not point the .dellink, for fb tunnel device
> (ip_vti0), the net_device will be removed as the default .dellink is
> unregister_netdevice_queue,but the tunnel still in the tunnel list,
> then if we add a new vti tunnel, in ip_tunnel_find():
>
> hlist_for_each_entry_rcu(t, head, hash_node) {
> if (local == t->parms.iph.saddr &&
> remote == t->parms.iph.daddr &&
> link == t->parms.link &&
> ==> type == t->dev->type &&
> ip_tunnel_key_match(&t->parms, flags, key))
> break;
> }
>
> the panic will happen, cause dev of ip_tunnel *t is null:
> [ 3835.072977] IP: [<ffffffffa04103fd>] ip_tunnel_find+0x9d/0xc0 [ip_tunnel]
> [ 3835.073008] PGD b2c21067 PUD b7277067 PMD 0
> [ 3835.073008] Oops: 0000 [#1] SMP
> .....
> [ 3835.073008] Stack:
> [ 3835.073008] ffff8800b72d77f0 ffffffffa0411924 ffff8800bb956000 ffff8800b72d78e0
> [ 3835.073008] ffff8800b72d78a0 0000000000000000 ffffffffa040d100 ffff8800b72d7858
> [ 3835.073008] ffffffffa040b2e3 0000000000000000 0000000000000000 0000000000000000
> [ 3835.073008] Call Trace:
> [ 3835.073008] [<ffffffffa0411924>] ip_tunnel_newlink+0x64/0x160 [ip_tunnel]
> [ 3835.073008] [<ffffffffa040b2e3>] vti_newlink+0x43/0x70 [ip_vti]
> [ 3835.073008] [<ffffffff8150d4da>] rtnl_newlink+0x4fa/0x5f0
> [ 3835.073008] [<ffffffff812f68bb>] ? nla_strlcpy+0x5b/0x70
> [ 3835.073008] [<ffffffff81508fb0>] ? rtnl_link_ops_get+0x40/0x60
> [ 3835.073008] [<ffffffff8150d11f>] ? rtnl_newlink+0x13f/0x5f0
> [ 3835.073008] [<ffffffff81509cf4>] rtnetlink_rcv_msg+0xa4/0x270
> [ 3835.073008] [<ffffffff8126adf5>] ? sock_has_perm+0x75/0x90
> [ 3835.073008] [<ffffffff81509c50>] ? rtnetlink_rcv+0x30/0x30
> [ 3835.073008] [<ffffffff81529e39>] netlink_rcv_skb+0xa9/0xc0
> [ 3835.073008] [<ffffffff81509c48>] rtnetlink_rcv+0x28/0x30
> ....
>
> modprobe ip_vti
> ip link del ip_vti0 type vti
> ip link add ip_vti0 type vti
> rmmod ip_vti
>
> do that one or more times, kernel will panic.
>
> fix it by assigning ip_tunnel_dellink to vti_link_ops' dellink, in
> which we skip the unregister of fb tunnel device. do the same on ip6_vti.
>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> Signed-off-by: Cong Wang <cwang@twopensource.com>
> ---
> net/ipv4/ip_vti.c | 1 +
> net/ipv6/ip6_vti.c | 11 +++++++++++
> 2 files changed, 12 insertions(+)
>
> diff --git a/net/ipv4/ip_vti.c b/net/ipv4/ip_vti.c
> index 3e86101..1a7e979 100644
> --- a/net/ipv4/ip_vti.c
> +++ b/net/ipv4/ip_vti.c
> @@ -528,6 +528,7 @@ static struct rtnl_link_ops vti_link_ops __read_mostly = {
> .validate = vti_tunnel_validate,
> .newlink = vti_newlink,
> .changelink = vti_changelink,
> + .dellink = ip_tunnel_dellink,
Nitpicking: please, use a tab instead of space to align the '=', like other
lines.
^ permalink raw reply
* Re: [PATCH net-net 0/4] Increase the limit of tuntap queues
From: Pankaj Gupta @ 2014-11-24 14:28 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: David Miller, linux-kernel, netdev, jasowang, dgibson, vfalico,
edumazet, vyasevic, hkchu, wuzhy, xemul, therbert, bhutchings,
xii, stephen, jiri, sergei shtylyov
In-Reply-To: <20141124080243.GD6286@redhat.com>
> On Sun, Nov 23, 2014 at 08:23:21PM -0500, David Miller wrote:
> > From: "Michael S. Tsirkin" <mst@redhat.com>
> > Date: Sun, 23 Nov 2014 22:30:32 +0200
> >
> > > qemu runs in the host, but it's unpriveledged: it gets
> > > passed tun FDs by a priveledged daemon, and it only
> > > has the rights to some operations,
> > > in particular to attach and detach queues.
> > >
> > > The assumption always was that this operation is safe
> > > and can't make kernel run out of resources.
> >
> > This creates a rather rediculous situation in my opinion.
> >
> > Configuring a network device is a privileged operation, the daemon
> > should be setting this thing up.
> >
> > In no other context would we have to worry about something like this.
>
> Right. Jason corrected me. I got it wrong:
> what qemu does is TUNSETQUEUE and that needs to get a queue
> that's already initialized by the daemon.
>
> To create new queues daemon calls TUNSETIFF,
> and that already can be used to create new devices,
> so it's a priveledged operation.
>
> This means it's safe to just drop the restriction,
> exactly as you suggested originally.
I will drop patch2 to add sysctl entry and and will send a v2 with other
patches.
Thanks,
Pankaj
> --
> MST
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH v3 26/41] vhost: virtio 1.0 endian-ness support
From: Cedric Le Goater @ 2014-11-24 14:28 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel
Cc: David Miller, cornelia.huck, rusty, nab, pbonzini, kvm,
virtualization, netdev
In-Reply-To: <1416829787-14252-27-git-send-email-mst@redhat.com>
Hi Michael,
Do you have a tree from where I could pull these patches ?
Thanks,
C.
On 11/24/2014 12:54 PM, Michael S. Tsirkin wrote:
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/vhost/vhost.c | 93 +++++++++++++++++++++++++++++++--------------------
> 1 file changed, 56 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index c90f437..4d379ed 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -33,8 +33,8 @@ enum {
> VHOST_MEMORY_F_LOG = 0x1,
> };
>
> -#define vhost_used_event(vq) ((u16 __user *)&vq->avail->ring[vq->num])
> -#define vhost_avail_event(vq) ((u16 __user *)&vq->used->ring[vq->num])
> +#define vhost_used_event(vq) ((__virtio16 __user *)&vq->avail->ring[vq->num])
> +#define vhost_avail_event(vq) ((__virtio16 __user *)&vq->used->ring[vq->num])
>
> static void vhost_poll_func(struct file *file, wait_queue_head_t *wqh,
> poll_table *pt)
> @@ -1001,7 +1001,7 @@ EXPORT_SYMBOL_GPL(vhost_log_write);
> static int vhost_update_used_flags(struct vhost_virtqueue *vq)
> {
> void __user *used;
> - if (__put_user(vq->used_flags, &vq->used->flags) < 0)
> + if (__put_user(cpu_to_vhost16(vq, vq->used_flags), &vq->used->flags) < 0)
> return -EFAULT;
> if (unlikely(vq->log_used)) {
> /* Make sure the flag is seen before log. */
> @@ -1019,7 +1019,7 @@ static int vhost_update_used_flags(struct vhost_virtqueue *vq)
>
> static int vhost_update_avail_event(struct vhost_virtqueue *vq, u16 avail_event)
> {
> - if (__put_user(vq->avail_idx, vhost_avail_event(vq)))
> + if (__put_user(cpu_to_vhost16(vq, vq->avail_idx), vhost_avail_event(vq)))
> return -EFAULT;
> if (unlikely(vq->log_used)) {
> void __user *used;
> @@ -1038,6 +1038,7 @@ static int vhost_update_avail_event(struct vhost_virtqueue *vq, u16 avail_event)
>
> int vhost_init_used(struct vhost_virtqueue *vq)
> {
> + __virtio16 last_used_idx;
> int r;
> if (!vq->private_data)
> return 0;
> @@ -1046,7 +1047,13 @@ int vhost_init_used(struct vhost_virtqueue *vq)
> if (r)
> return r;
> vq->signalled_used_valid = false;
> - return get_user(vq->last_used_idx, &vq->used->idx);
> + if (!access_ok(VERIFY_READ, &vq->used->idx, sizeof vq->used->idx))
> + return -EFAULT;
> + r = __get_user(last_used_idx, &vq->used->idx);
> + if (r)
> + return r;
> + vq->last_used_idx = vhost16_to_cpu(vq, last_used_idx);
> + return 0;
> }
> EXPORT_SYMBOL_GPL(vhost_init_used);
>
> @@ -1087,16 +1094,16 @@ static int translate_desc(struct vhost_virtqueue *vq, u64 addr, u32 len,
> /* Each buffer in the virtqueues is actually a chain of descriptors. This
> * function returns the next descriptor in the chain,
> * or -1U if we're at the end. */
> -static unsigned next_desc(struct vring_desc *desc)
> +static unsigned next_desc(struct vhost_virtqueue *vq, struct vring_desc *desc)
> {
> unsigned int next;
>
> /* If this descriptor says it doesn't chain, we're done. */
> - if (!(desc->flags & VRING_DESC_F_NEXT))
> + if (!(desc->flags & cpu_to_vhost16(vq, VRING_DESC_F_NEXT)))
> return -1U;
>
> /* Check they're not leading us off end of descriptors. */
> - next = desc->next;
> + next = vhost16_to_cpu(vq, desc->next);
> /* Make sure compiler knows to grab that: we don't want it changing! */
> /* We will use the result as an index in an array, so most
> * architectures only need a compiler barrier here. */
> @@ -1113,18 +1120,19 @@ static int get_indirect(struct vhost_virtqueue *vq,
> {
> struct vring_desc desc;
> unsigned int i = 0, count, found = 0;
> + u32 len = vhost32_to_cpu(vq, indirect->len);
> int ret;
>
> /* Sanity check */
> - if (unlikely(indirect->len % sizeof desc)) {
> + if (unlikely(len % sizeof desc)) {
> vq_err(vq, "Invalid length in indirect descriptor: "
> "len 0x%llx not multiple of 0x%zx\n",
> - (unsigned long long)indirect->len,
> + (unsigned long long)vhost32_to_cpu(vq, indirect->len),
> sizeof desc);
> return -EINVAL;
> }
>
> - ret = translate_desc(vq, indirect->addr, indirect->len, vq->indirect,
> + ret = translate_desc(vq, vhost64_to_cpu(vq, indirect->addr), len, vq->indirect,
> UIO_MAXIOV);
> if (unlikely(ret < 0)) {
> vq_err(vq, "Translation failure %d in indirect.\n", ret);
> @@ -1135,7 +1143,7 @@ static int get_indirect(struct vhost_virtqueue *vq,
> * architectures only need a compiler barrier here. */
> read_barrier_depends();
>
> - count = indirect->len / sizeof desc;
> + count = len / sizeof desc;
> /* Buffers are chained via a 16 bit next field, so
> * we can have at most 2^16 of these. */
> if (unlikely(count > USHRT_MAX + 1)) {
> @@ -1155,16 +1163,17 @@ static int get_indirect(struct vhost_virtqueue *vq,
> if (unlikely(memcpy_fromiovec((unsigned char *)&desc,
> vq->indirect, sizeof desc))) {
> vq_err(vq, "Failed indirect descriptor: idx %d, %zx\n",
> - i, (size_t)indirect->addr + i * sizeof desc);
> + i, (size_t)vhost64_to_cpu(vq, indirect->addr) + i * sizeof desc);
> return -EINVAL;
> }
> - if (unlikely(desc.flags & VRING_DESC_F_INDIRECT)) {
> + if (unlikely(desc.flags & cpu_to_vhost16(vq, VRING_DESC_F_INDIRECT))) {
> vq_err(vq, "Nested indirect descriptor: idx %d, %zx\n",
> - i, (size_t)indirect->addr + i * sizeof desc);
> + i, (size_t)vhost64_to_cpu(vq, indirect->addr) + i * sizeof desc);
> return -EINVAL;
> }
>
> - ret = translate_desc(vq, desc.addr, desc.len, iov + iov_count,
> + ret = translate_desc(vq, vhost64_to_cpu(vq, desc.addr),
> + vhost32_to_cpu(vq, desc.len), iov + iov_count,
> iov_size - iov_count);
> if (unlikely(ret < 0)) {
> vq_err(vq, "Translation failure %d indirect idx %d\n",
> @@ -1172,11 +1181,11 @@ static int get_indirect(struct vhost_virtqueue *vq,
> return ret;
> }
> /* If this is an input descriptor, increment that count. */
> - if (desc.flags & VRING_DESC_F_WRITE) {
> + if (desc.flags & cpu_to_vhost16(vq, VRING_DESC_F_WRITE)) {
> *in_num += ret;
> if (unlikely(log)) {
> - log[*log_num].addr = desc.addr;
> - log[*log_num].len = desc.len;
> + log[*log_num].addr = vhost64_to_cpu(vq, desc.addr);
> + log[*log_num].len = vhost32_to_cpu(vq, desc.len);
> ++*log_num;
> }
> } else {
> @@ -1189,7 +1198,7 @@ static int get_indirect(struct vhost_virtqueue *vq,
> }
> *out_num += ret;
> }
> - } while ((i = next_desc(&desc)) != -1);
> + } while ((i = next_desc(vq, &desc)) != -1);
> return 0;
> }
>
> @@ -1209,15 +1218,18 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq,
> struct vring_desc desc;
> unsigned int i, head, found = 0;
> u16 last_avail_idx;
> + __virtio16 avail_idx;
> + __virtio16 ring_head;
> int ret;
>
> /* Check it isn't doing very strange things with descriptor numbers. */
> last_avail_idx = vq->last_avail_idx;
> - if (unlikely(__get_user(vq->avail_idx, &vq->avail->idx))) {
> + if (unlikely(__get_user(avail_idx, &vq->avail->idx))) {
> vq_err(vq, "Failed to access avail idx at %p\n",
> &vq->avail->idx);
> return -EFAULT;
> }
> + vq->avail_idx = vhost16_to_cpu(vq, avail_idx);
>
> if (unlikely((u16)(vq->avail_idx - last_avail_idx) > vq->num)) {
> vq_err(vq, "Guest moved used index from %u to %u",
> @@ -1234,7 +1246,7 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq,
>
> /* Grab the next descriptor number they're advertising, and increment
> * the index we've seen. */
> - if (unlikely(__get_user(head,
> + if (unlikely(__get_user(ring_head,
> &vq->avail->ring[last_avail_idx % vq->num]))) {
> vq_err(vq, "Failed to read head: idx %d address %p\n",
> last_avail_idx,
> @@ -1242,6 +1254,8 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq,
> return -EFAULT;
> }
>
> + head = vhost16_to_cpu(vq, ring_head);
> +
> /* If their number is silly, that's an error. */
> if (unlikely(head >= vq->num)) {
> vq_err(vq, "Guest says index %u > %u is available",
> @@ -1274,7 +1288,7 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq,
> i, vq->desc + i);
> return -EFAULT;
> }
> - if (desc.flags & VRING_DESC_F_INDIRECT) {
> + if (desc.flags & cpu_to_vhost16(vq, VRING_DESC_F_INDIRECT)) {
> ret = get_indirect(vq, iov, iov_size,
> out_num, in_num,
> log, log_num, &desc);
> @@ -1286,20 +1300,21 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq,
> continue;
> }
>
> - ret = translate_desc(vq, desc.addr, desc.len, iov + iov_count,
> + ret = translate_desc(vq, vhost64_to_cpu(vq, desc.addr),
> + vhost32_to_cpu(vq, desc.len), iov + iov_count,
> iov_size - iov_count);
> if (unlikely(ret < 0)) {
> vq_err(vq, "Translation failure %d descriptor idx %d\n",
> ret, i);
> return ret;
> }
> - if (desc.flags & VRING_DESC_F_WRITE) {
> + if (desc.flags & cpu_to_vhost16(vq, VRING_DESC_F_WRITE)) {
> /* If this is an input descriptor,
> * increment that count. */
> *in_num += ret;
> if (unlikely(log)) {
> - log[*log_num].addr = desc.addr;
> - log[*log_num].len = desc.len;
> + log[*log_num].addr = vhost64_to_cpu(vq, desc.addr);
> + log[*log_num].len = vhost32_to_cpu(vq, desc.len);
> ++*log_num;
> }
> } else {
> @@ -1312,7 +1327,7 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq,
> }
> *out_num += ret;
> }
> - } while ((i = next_desc(&desc)) != -1);
> + } while ((i = next_desc(vq, &desc)) != -1);
>
> /* On success, increment avail index. */
> vq->last_avail_idx++;
> @@ -1335,7 +1350,10 @@ EXPORT_SYMBOL_GPL(vhost_discard_vq_desc);
> * want to notify the guest, using eventfd. */
> int vhost_add_used(struct vhost_virtqueue *vq, unsigned int head, int len)
> {
> - struct vring_used_elem heads = { head, len };
> + struct vring_used_elem heads = {
> + cpu_to_vhost32(vq, head),
> + cpu_to_vhost32(vq, len)
> + };
>
> return vhost_add_used_n(vq, &heads, 1);
> }
> @@ -1404,7 +1422,7 @@ int vhost_add_used_n(struct vhost_virtqueue *vq, struct vring_used_elem *heads,
>
> /* Make sure buffer is written before we update index. */
> smp_wmb();
> - if (put_user(vq->last_used_idx, &vq->used->idx)) {
> + if (__put_user(cpu_to_vhost16(vq, vq->last_used_idx), &vq->used->idx)) {
> vq_err(vq, "Failed to increment used idx");
> return -EFAULT;
> }
> @@ -1422,7 +1440,8 @@ EXPORT_SYMBOL_GPL(vhost_add_used_n);
>
> static bool vhost_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
> {
> - __u16 old, new, event;
> + __u16 old, new;
> + __virtio16 event;
> bool v;
> /* Flush out used index updates. This is paired
> * with the barrier that the Guest executes when enabling
> @@ -1434,12 +1453,12 @@ static bool vhost_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
> return true;
>
> if (!vhost_has_feature(vq, VIRTIO_RING_F_EVENT_IDX)) {
> - __u16 flags;
> + __virtio16 flags;
> if (__get_user(flags, &vq->avail->flags)) {
> vq_err(vq, "Failed to get flags");
> return true;
> }
> - return !(flags & VRING_AVAIL_F_NO_INTERRUPT);
> + return !(flags & cpu_to_vhost16(vq, VRING_AVAIL_F_NO_INTERRUPT));
> }
> old = vq->signalled_used;
> v = vq->signalled_used_valid;
> @@ -1449,11 +1468,11 @@ static bool vhost_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
> if (unlikely(!v))
> return true;
>
> - if (get_user(event, vhost_used_event(vq))) {
> + if (__get_user(event, vhost_used_event(vq))) {
> vq_err(vq, "Failed to get used event idx");
> return true;
> }
> - return vring_need_event(event, new, old);
> + return vring_need_event(vhost16_to_cpu(vq, event), new, old);
> }
>
> /* This actually signals the guest, using eventfd. */
> @@ -1488,7 +1507,7 @@ EXPORT_SYMBOL_GPL(vhost_add_used_and_signal_n);
> /* OK, now we need to know about added descriptors. */
> bool vhost_enable_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
> {
> - u16 avail_idx;
> + __virtio16 avail_idx;
> int r;
>
> if (!(vq->used_flags & VRING_USED_F_NO_NOTIFY))
> @@ -1519,7 +1538,7 @@ bool vhost_enable_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
> return false;
> }
>
> - return avail_idx != vq->avail_idx;
> + return vhost16_to_cpu(vq, avail_idx) != vq->avail_idx;
> }
> EXPORT_SYMBOL_GPL(vhost_enable_notify);
>
^ permalink raw reply
* Re: [PATCH 0/6] kernel tinification: optionally compile out splice family of syscalls (splice, vmsplice, tee and sendfile)
From: Josh Triplett @ 2014-11-24 14:54 UTC (permalink / raw)
To: Pieter Smith
Cc: Jeff Layton, David Miller,
alexander.h.duyck-ral2JQCrhuEAvxtiuMwx3w,
viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn, ast-uqk4Ao+rVK5Wk0Htik3J/w,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
beber-2YnHqweIUXrk1uMJSBkQmQ,
catalina.mocanu-Re5JQEeQqe8AvxtiuMwx3w,
dborkman-H+wXaHxf7aLQT0dZR+AlfA, edumazet-hpIqsD4AKlfQT0dZR+AlfA,
ebiederm-aS9lmoZGLiVWk0Htik3J/w, fabf-AgBVmzD5pcezQB+pC5nmwQ,
fuse-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
geert-Td1EMuHUCqxL1ZNQvxDV9g, hughd-hpIqsD4AKlfQT0dZR+AlfA,
iulia.manda21-Re5JQEeQqe8AvxtiuMwx3w, JBeulich-IBi9RG/b67k,
bfields-uC3wQj2KruNg9hUCZPvPmw, linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, mcgrof-IBi9RG/b67k,
mattst88-Re5JQEeQqe8AvxtiuMwx3w, mgorman-l3A5Bk7waGM,
mst-H+wXaHxf7aLQT0dZR+AlfA, miklos-sUDqSbJrdHQHWmgEVkV9KA,
netdev-u79uwXL29TY76Z2rM5mHXA, oleg-H+wXaHxf7aLQT0dZR+AlfA,
Paul.Durrant-Sxgqhf6Nn4DQT0dZR+AlfA,
paulmck-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8,
pefoley2-lY0TAiDIAFlBDgjK7y7TUQ, tgraf-G/eBtMaohhA,
therbert-hpIqsD4AKlfQT0dZR+AlfA, willemb-hpIqsD4AKlfQT0dZR+AlfA,
xiaoguangrong-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8,
zhenglong.cai-TJRtMXcVgQTM1kAEIRd3EQ
In-Reply-To: <20141124100138.GB1055@smipidev>
On Mon, Nov 24, 2014 at 11:01:38AM +0100, Pieter Smith wrote:
> On Sun, Nov 23, 2014 at 04:32:51PM -0800, Josh Triplett wrote:
> > On Sun, Nov 23, 2014 at 07:28:10PM -0500, Jeff Layton wrote:
> > > On Sun, 23 Nov 2014 15:36:37 -0800
> > > Josh Triplett <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org> wrote:
> > >
> > > > On Sun, Nov 23, 2014 at 09:30:40PM +0100, Pieter Smith wrote:
> > > > > On Sun, Nov 23, 2014 at 11:43:26AM -0800, Josh Triplett wrote:
> > > > > > On Sun, Nov 23, 2014 at 01:46:23PM -0500, David Miller wrote:
> > > > > > > Truly removing sendfile/sendpage means that you can't even compile NFS
> > > > > > > into the tree.
> > > > > >
> > > > > > If you mean the in-kernel nfsd (CONFIG_NFSD), that already has a large
> > > > > > stack of "select" and "depends on", both directly and indirectly; adding
> > > > > > a "select SPLICE_SYSCALL" to it seems fine. (That select does need
> > > > > > adding, though. Pieter, you need to test-compile more than just
> > > > > > tinyconfig and defconfig. Try an allyesconfig with *just* splice turned
> > > > > > off, and make sure that compiles.)
> > > > >
> > > > > Did exacly that. Took forever on my hardware, but no problems.
> > > >
> > > > Ah, I see. Looking more closely at nfsd, it looks like it already has a
> > > > code path for filesystems that don't do splice. I think, rather than
> > > > making nfsd select SPLICE_SYSCALL, that it would suffice to change the
> > > > "rqstp->rq_splice_ok = true;" in svc_process_common (net/sunrpc/svc.c)
> > > > to:
> > > >
> > > > rqstp->rq_splice_ok = IS_ENABLED(CONFIG_SPLICE_SYSCALL);
> > > >
> > > > Then nfsd should simply *always* fall back to its non-splice support.
> > > >
> > >
> > > I'd probably prefer the above, actually. We have to keep supporting
> > > non-splice enabled fs' for the forseeable future, so we may as well
> > > allow people to run nfsd in such configurations. It could even be
> > > useful for testing the non-splice-enabled codepaths.
> >
> > Good point!
> >
> > - Josh Triplett
>
> I'll add this to svc_process_common. I can squash this into PATCH 3, which is
> where the syscalls can be compiled out. The log entry may however get a little
> crowded and multi-functional.
>
> Should I keep this as a separate patch?
I'd keep it as a separate patch, yes. It doesn't become necessary until
patch 6, so you can add it as a new patch between patches 3 and 6.
- Josh Triplett
^ permalink raw reply
* Re: [RFC PATCH 0/4] switch device: offload policy attributes
From: Roopa Prabhu @ 2014-11-24 14:55 UTC (permalink / raw)
To: Scott Feldman
Cc: Jiří Pírko, Jamal Hadi Salim, Benjamin LaHaise,
Thomas Graf, john.fastabend, stephen, John Linville, nhorman,
Nicolas Dichtel, vyasevic, Florian Fainelli, buytenh, Aviad Raveh,
Netdev, David S. Miller, shrijeet, Andy Gospodarek
In-Reply-To: <CAE4R7bCDp2Cf=azOJvnm3_YRdsD5vwCafbcVFuseUaPuYKDA1g@mail.gmail.com>
On 11/24/14, 2:18 AM, Scott Feldman wrote:
> Hi Roopa,
>
> I have a patch pending against Jiri's v2 that's uses existing
> ndo_bridge_setlink/getlink to push policy settings down to port driver
> for controlling HW offload. I had to make a few tweaks, but for the
> most part setlink/getlink already has the master/self semantics so
> users can set policy flags on bridge's SW version of the port (master)
> or on the offloaded version of the port (self).
> I added the new
> hwmode option "swdev" to the existing "vepa"|"veb" choices. When you
> specify hwmode, SELF is set and the port driver's setlink get's
> called. Did you look at setlink/getlink? It looks like the kernel
> and iproute2 where going down this route of using setlink/getlink for
> SELF policy, so I'm wondering if we need more?
If i understand you correctly, this will mean the port driver implements
the
setlink/getlink with the bridge port flags and attributes. And, it also
means
the port driver will parse the netlink msg instead of the bridge driver
parsing all attributes and then calling offloads.
But, in cases where we want bridge port flags and attributes set both in
hw and sw,
the user (iproute2) will have to set both MASTER and SELF flags, and the
netlink parsing will now happen in two places, the bridge driver and the
bridge port
driver ?
For bridge stp state updates, the bridge driver will call the
ports->setlink ndo op ?
(We should probably rename the ndo_bridge_setlink to ndo_setlink)
>
> On FDB entries, using master/self semantics that exist, it's clear
> which are owned by offloaded device and which are owned by bridge.
> The one missing annotation was a flag indicating FDB entry in bridge
> was synced from device. And a policy flag to turn on/off syncing from
> the device. The policy flag is just another IFLA_BRPORT flags passed
> with setlink/getlink.
>
> The setlink/getlink patch will go out in v3 once I finish testing it
> and push it to Jiri. Hopefully tomorrow.
In my patches, I used newlink..., but in most cases all attributes set
via newlink can be
used with setlink and hence getlink. So, i think we are close here.
But, Oh wait, i am talking about rtnl_link_ops
->newlink/changelink/getlink/dellink. I did not
realize there was a parallel ndo op to this. But thats probably because,
rtnl_link_ops can be
used only for logical devices. But, i see bridge driver implementing
both the
rtnl_link_ops changelink and ndo op setlink. hmm..seems like a lot of
duplication.
Will look closely some more.
Coming back to my series, i was trying to get a common set of flags for
all netdevs
(bridge, bond, vxlans so far), and hence the common flag in 'struct
ifiinfomsg'.
But, I am all for using existing infrastructure if it fits well.
For the fdb offloads, the NTF_SELF and NTF_MASTER is in 'struct
ndmsg->ndm_flags', which is also
what i was proposing. So, ack there.
For the bridge netdev, using the flag in IFLA_BRIDGE_FLAGS, is also ok.
I will look at your patches when they are out.
Thanks,
Roopa
On Fri, Nov 21, 2014 at 12:49 PM, <roopa@cumulusnetworks.com> wrote:
>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>
>>
>> This series aims at introducing new policy attibutes/flags to enable
>> selective offloading of kernel network objects.
>> This is in the context of supporting switch devices in the linux kernel.
>>
>> Assumption:
>> - All kernel network objects (routes, neighs, bridges, bonds, vxlans)
>> can be offloaded (This is true today with a few exceptions maybe)
>>
>> policy points:
>> - By default all objects exist in software (kernel)
>> - Per object flag to add/del/show in kernel, hardware or both
>> - System global option to turn on/off offloads for all network objects.
>> This is for systems who want to turn offloading on for all network objects
>> by default. us :). Apps dont need to know about offloading in this
>> model. (TBD)
>>
>> Patches are based on jiri/sfeldma's rocker work.
>>
>> Apologize for the incomplete and untested code. This is a sample patch
>> to get some initial feedback.
>>
>> Roopa Prabhu (4):
>> rtnetlink: new flag NLM_F_HW_OFFLOAD to indicate kernel object
>> offload to hardware
>> netdev: new feature flag NETIF_F_HW_OFFLOAD to indicate netdev object
>> offload to hardware
>> swdevice: new generic op to set bridge port attr
>> bridge: make hw offload conditional on bridge and bridge port offload
>> flags
>>
>> include/linux/netdev_features.h | 1 +
>> include/net/switchdev.h | 8 ++++++-
>> include/uapi/linux/netlink.h | 2 ++
>> net/bridge/br_netlink.c | 50 +++++++++++++++++++++++++++++++--------
>> net/bridge/br_private.h | 2 ++
>> net/bridge/br_stp.c | 9 ++++---
>> net/bridge/br_stp_if.c | 8 +++++--
>> net/core/rtnetlink.c | 7 ++++++
>> net/switchdev/switchdev.c | 17 +++++++++++++
>> 9 files changed, 88 insertions(+), 16 deletions(-)
>>
>> --
>> 1.7.10.4
>>
^ permalink raw reply
* Re: net_tx_action race condition?
From: Herbert Xu @ 2014-11-24 15:08 UTC (permalink / raw)
To: Angelo Rizzi; +Cc: netdev
In-Reply-To: <547332D7.7050104@3dautomazione.it>
Angelo Rizzi <angelo.rizzi@3dautomazione.it> wrote:
> [-- text/plain, encoding 7bit, charset: iso-8859-15, 22 lines --]
>
> Hi Daniel,
> Here attached the patch file you required.
> The problem i've found is on the declaration of 'struct softnet_data
> *sd' in function 'net_tx_action'
> What happens to me (i have an embedded system based on FPGA and a NIOS2
> cpu) is that, due to compiler optimization, the content of
> 'sd->completion_queue' is saved in a CPU register before interrupt
> disabling (when the instruction 'if (sd->completion_queue) {' is
> executed) and then the register contents is used for interrupt-disabled
> assignment ('clist = sd->completion_queue') instead of re-read the
> variable contents.
> This seems to lead to a race condition when an interrupt modifies the
> content of 'sd->completion_queue' between these two instructions.
> What i have done to avoid this situation is to change the declaration of
> 'struct softnet_data *sd' to 'volatile struct softnet_data *sd' and now
> everything seems to be ok.
> I hope this will help.
local_irq_disable is supposed to contain a compiler barrier. So
this is either a buggy arch or a buggy compiler.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: net_tx_action race condition?
From: Eric Dumazet @ 2014-11-24 15:33 UTC (permalink / raw)
To: Angelo Rizzi; +Cc: netdev, linux-kernel@vger.kernel.org
In-Reply-To: <547332D7.7050104@3dautomazione.it>
On Mon, 2014-11-24 at 14:29 +0100, Angelo Rizzi wrote:
> Hi Daniel,
> Here attached the patch file you required.
> The problem i've found is on the declaration of 'struct softnet_data
> *sd' in function 'net_tx_action'
> What happens to me (i have an embedded system based on FPGA and a NIOS2
> cpu) is that, due to compiler optimization, the content of
> 'sd->completion_queue' is saved in a CPU register before interrupt
> disabling (when the instruction 'if (sd->completion_queue) {' is
> executed) and then the register contents is used for interrupt-disabled
> assignment ('clist = sd->completion_queue') instead of re-read the
> variable contents.
> This seems to lead to a race condition when an interrupt modifies the
> content of 'sd->completion_queue' between these two instructions.
> What i have done to avoid this situation is to change the declaration of
> 'struct softnet_data *sd' to 'volatile struct softnet_data *sd' and now
> everything seems to be ok.
> I hope this will help.
>
> Regards,
> Angelo
>
Do not add volatile in the kernel, this is not how we solve this kind of
problems. Documentation/memory-barriers.txt and
Documentation/00-INDEX:476:volatile-considered-harmful.txt
Documentation/00-INDEX:477: - Why the "volatile" type class should not be used
I am surprised this patch is needed. Many other 'bugs' would need
similar fixes.
local_irq_disable() MUST have a memory barrier. This looks like a bug in
one particular arch implementation.
On x86 for example, native_irq_disable() is really :
asm volatile("cli": : :"memory");
diff --git a/net/core/dev.c b/net/core/dev.c
index ac4836241a965a71952469ba054f87d8dfca2d32..fa73072e515aa07fa8ae1bc39174b7d59c7a31a5 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3406,7 +3406,7 @@ static void net_tx_action(struct softirq_action *h)
struct sk_buff *clist;
local_irq_disable();
- clist = sd->completion_queue;
+ clist = ACCESS_ONCE(sd->completion_queue);
sd->completion_queue = NULL;
local_irq_enable();
^ permalink raw reply related
* Re: [fuse-devel] [PATCH 4/6] fs/fuse: support compiling out splice
From: Josh Triplett @ 2014-11-24 16:05 UTC (permalink / raw)
To: Pieter Smith
Cc: Richard Weinberger, Michael S. Tsirkin, Bertrand Jacquin,
Oleg Nesterov, J. Bruce Fields, Eric Dumazet,
蔡正龙, Jeff Layton, Tom Herbert,
Alexei Starovoitov, Miklos Szeredi, Peter Foley, Hugh Dickins,
Xiao Guangrong, Geert Uytterhoeven, Mel Gorman, Matt Turner,
Paul E. McKenney, Alexander Duyck, open list:FUSE: FILESYSTEM...,
Luis R. Rodriguez
In-Reply-To: <20141124094931.GA1055@smipidev>
On Mon, Nov 24, 2014 at 10:49:31AM +0100, Pieter Smith wrote:
> On Sun, Nov 23, 2014 at 03:23:02PM -0800, Josh Triplett wrote:
> > On Sun, Nov 23, 2014 at 11:29:08PM +0100, Richard Weinberger wrote:
> > > On Sun, Nov 23, 2014 at 3:20 PM, Pieter Smith <pieter@boesman.nl> wrote:
> > > > To implement splice support, fs/fuse makes use of nosteal_pipe_buf_ops. This
> > > > struct is exported by fs/splice. The goal of the larger patch set is to
> > > > completely compile out fs/splice, so uses of the exported struct need to be
> > > > compiled out along with fs/splice.
> > > >
> > > > This patch therefore compiles out splice support in fs/fuse when
> > > > CONFIG_SYSCALL_SPLICE is undefined.
> > > >
> > > > Signed-off-by: Pieter Smith <pieter@boesman.nl>
> > > > ---
> > > > fs/fuse/dev.c | 4 ++--
> > > > include/linux/fs.h | 6 ++++++
> > > > 2 files changed, 8 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> > > > index ca88731..f8f92a4 100644
> > > > --- a/fs/fuse/dev.c
> > > > +++ b/fs/fuse/dev.c
> > > > @@ -1291,7 +1291,7 @@ static ssize_t fuse_dev_read(struct kiocb *iocb, const struct iovec *iov,
> > > > return fuse_dev_do_read(fc, file, &cs, iov_length(iov, nr_segs));
> > > > }
> > > >
> > > > -static ssize_t fuse_dev_splice_read(struct file *in, loff_t *ppos,
> > > > +static ssize_t __maybe_unused fuse_dev_splice_read(struct file *in, loff_t *ppos,
> > > > struct pipe_inode_info *pipe,
> > > > size_t len, unsigned int flags)
> > > > {
> > > > @@ -2144,7 +2144,7 @@ const struct file_operations fuse_dev_operations = {
> > > > .llseek = no_llseek,
> > > > .read = do_sync_read,
> > > > .aio_read = fuse_dev_read,
> > > > - .splice_read = fuse_dev_splice_read,
> > > > + .splice_read = __splice_p(fuse_dev_splice_read),
> > > > .write = do_sync_write,
> > > > .aio_write = fuse_dev_write,
> > > > .splice_write = fuse_dev_splice_write,
> > > > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > > > index a957d43..04c0975 100644
> > > > --- a/include/linux/fs.h
> > > > +++ b/include/linux/fs.h
> > > > @@ -2443,6 +2443,12 @@ extern int blkdev_fsync(struct file *filp, loff_t start, loff_t end,
> > > > int datasync);
> > > > extern void block_sync_page(struct page *page);
> > > >
> > > > +#ifdef CONFIG_SYSCALL_SPLICE
> > > > +#define __splice_p(x) x
> > > > +#else
> > > > +#define __splice_p(x) NULL
> > > > +#endif
> > > > +
> > >
> > > This needs to go into a different patch.
> > > One logical change per patch please. :-)
> >
> > Easy enough to merge this one into the patch introducing
> > CONFIG_SYSCALL_SPLICE, then.
> >
> > - Josh Triplett
>
> The patch introducing CONFIG_SYSCALL_SPLICE (PATCH 3) only compiles out the
> syscalls. PATCH 6 on the other hand, compiles out fs/splice.c. This patch
> allows fs/fuse to be compiled when fs/splice.c is compiled out. If I am to
> squash it, it would be logical to include it in PATCH 6, not 3.
The suggestion wasn't to move the fs/fuse/dev.c bits; those should
definitely stay in this patch. The suggestion was just to move the bit
of the patch defining __splice_p from this patch to patch 3. (Note that
you need to define it before you use it, so it can't go in patch 6.)
- Josh Triplett
^ permalink raw reply
* GRE with GRO very slow when forwarding starting with 3.14.24
From: Wolfgang Walter @ 2014-11-24 16:13 UTC (permalink / raw)
To: netdev; +Cc: Stable Kernel
Hello,
starting with 3.14.24 GRE with GRO on is very slow. To be more specific:
yyyy <--> GRO_endpoint <-_> .... <--> |eth0<->GRO-endpoint | eth1 |<-> xxxx
routing (IPv4) between xxxx and yyyy is very slow when GRO is enabled on eth0
and/or eth1 starting with stable kernel 3.14.24
Regards,
--
Wolfgang Walter
Studentenwerk München
Anstalt des öffentlichen Rechts
^ permalink raw reply
* RE: [linux-nics] FIX ME in e1000_82575.c
From: Fujinaka, Todd @ 2014-11-24 16:32 UTC (permalink / raw)
To: nick, Kirsher, Jeffrey T
Cc: Linux NICS, e1000-devel@lists.sourceforge.net, Vick, Matthew,
netdev@vger.kernel.org, Wyborny, Carolyn,
linux-kernel@vger.kernel.or
In-Reply-To: <547165BC.2000508@gmail.com>
I'm not sure there's much need to actually change the value. It's been the same since 2007 when the log message says, "Removed FIXME comment."
I think someone forgot to remove the second FIXME comment. I'll send a patch.
Todd Fujinaka
Software Application Engineer
Networking Division (ND)
Intel Corporation
todd.fujinaka@intel.com
(503) 712-4565
-----Original Message-----
From: linux-nics-bounces@isotope.jf.intel.com [mailto:linux-nics-bounces@isotope.jf.intel.com] On Behalf Of nick
Sent: Saturday, November 22, 2014 8:43 PM
To: Kirsher, Jeffrey T
Cc: Linux NICS; e1000-devel@lists.sourceforge.net; Vick, Matthew; netdev@vger.kernel.org; Wyborny, Carolyn; linux-kernel@vger.kernel.or
Subject: [linux-nics] FIX ME in e1000_82575.c
Greetings Again Jeff,
I am wondering about sending in a patch to FIX ME in the file, e1000_82575 due to incorrect values in the function, igdb_acquire_swfw_sync_82575. If you or one of the other maintainers at Intel can send me the correct values or a hardware reference for me to find the values, I would be glad to send it a patch fixing these outstanding issues.
Regards Nick
_______________________________________________
Linux-nics mailing list
Linux-nics@intel.com
^ permalink raw reply
* [PATCH iproute2] iplink: allow to show ip addresses
From: Nicolas Dichtel @ 2014-11-24 16:42 UTC (permalink / raw)
To: shemminger; +Cc: netdev, Nicolas Dichtel
This patch adds a new option (-addresses) to the 'ip link' command so that the
user can display link details and IP addresses with the same command.
Example:
$ ip -d -a l ls gre1
9: gre1@NONE: <POINTOPOINT,NOARP,UP,LOWER_UP> mtu 1468 qdisc noqueue state UNKNOWN mode DEFAULT group default
link/gre 10.16.0.249 peer 10.16.0.121 promiscuity 0
gre remote 10.16.0.121 local 10.16.0.249 ttl inherit ikey 0.0.0.10 okey 0.0.0.10 icsum ocsum
inet 192.168.0.249 peer 192.168.0.121/32 scope global gre1
valid_lft forever preferred_lft forever
inet6 fe80::5efe:a10:f9/64 scope link
valid_lft forever preferred_lft forever
Suggested-by: Christophe Gouault <christophe.gouault@6wind.com>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
include/utils.h | 1 +
ip/ip.c | 3 +++
ip/ipaddress.c | 5 +++--
3 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/include/utils.h b/include/utils.h
index eef9c42d2fd5..9d0f2ad83e49 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -15,6 +15,7 @@ extern int human_readable;
extern int use_iec;
extern int show_stats;
extern int show_details;
+extern int show_addresses;
extern int show_raw;
extern int resolve_hosts;
extern int oneline;
diff --git a/ip/ip.c b/ip/ip.c
index 5f759d5424aa..ad53d14a1fd9 100644
--- a/ip/ip.c
+++ b/ip/ip.c
@@ -28,6 +28,7 @@ int human_readable = 0;
int use_iec = 0;
int show_stats = 0;
int show_details = 0;
+int show_addresses = 0;
int resolve_hosts = 0;
int oneline = 0;
int timestamp = 0;
@@ -226,6 +227,8 @@ int main(int argc, char **argv)
++show_stats;
} else if (matches(opt, "-details") == 0) {
++show_details;
+ } else if (matches(opt, "-addresses") == 0) {
+ ++show_addresses;
} else if (matches(opt, "-resolve") == 0) {
++resolve_hosts;
} else if (matches(opt, "-oneline") == 0) {
diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index db39437305a9..10cbb4248a67 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -1323,7 +1323,7 @@ static int ipaddr_list_flush_or_save(int argc, char **argv, int action)
* link filters are present, use RTM_GETLINK to get
* the link device
*/
- if (filter_dev && filter.group == -1 && do_link == 1) {
+ if (filter_dev && filter.group == -1 && do_link == 1 && show_addresses == 0) {
if (iplink_get(0, filter_dev, RTEXT_FILTER_VF) < 0) {
perror("Cannot send link get request");
exit(1);
@@ -1437,7 +1437,8 @@ void ipaddr_get_vf_rate(int vfnum, int *min, int *max, int idx)
int ipaddr_list_link(int argc, char **argv)
{
- preferred_family = AF_PACKET;
+ if (show_addresses == 0)
+ preferred_family = AF_PACKET;
do_link = 1;
return ipaddr_list_flush_or_save(argc, argv, IPADD_LIST);
}
--
2.1.0
^ permalink raw reply related
* Re: GRE with GRO very slow when forwarding starting with 3.14.24
From: Eric Dumazet @ 2014-11-24 16:48 UTC (permalink / raw)
To: Wolfgang Walter; +Cc: netdev, Tom Herbert, Alexander Duyck
In-Reply-To: <2340533.MickblD7NF@h2o.as.studentenwerk.mhn.de>
On Mon, 2014-11-24 at 17:13 +0100, Wolfgang Walter wrote:
> Hello,
>
> starting with 3.14.24 GRE with GRO on is very slow. To be more specific:
>
> yyyy <--> GRO_endpoint <-_> .... <--> |eth0<->GRO-endpoint | eth1 |<-> xxxx
>
> routing (IPv4) between xxxx and yyyy is very slow when GRO is enabled on eth0
> and/or eth1 starting with stable kernel 3.14.24
>
> Regards,
tcpdump might help, but I presume GSO is no longer working properly on
egress.
Can you try to revert :
commit abe640984aa492652232b65d3579361cf6d461f5
Author: Tom Herbert <therbert@google.com>
Date: Thu Oct 30 08:40:56 2014 -0700
gre: Use inner mac length when computing tunnel length
[ Upstream commit 14051f0452a2c26a3f4791e6ad6a435e8f1945ff ]
Currently, skb_inner_network_header is used but this does not account
for Ethernet header for ETH_P_TEB. Use skb_inner_mac_header which
handles TEB and also should work with IP encapsulation in which case
inner mac and inner network headers are the same.
Tested: Ran TCP_STREAM over GRE, worked as expected.
Signed-off-by: Tom Herbert <therbert@google.com>
Acked-by: Alexander Duyck <alexander.h.duyck@redhat.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
diff --git a/net/ipv4/gre_offload.c b/net/ipv4/gre_offload.c
index 2d24f293f977..8c8493ea6b1c 100644
--- a/net/ipv4/gre_offload.c
+++ b/net/ipv4/gre_offload.c
@@ -50,7 +50,7 @@ static struct sk_buff *gre_gso_segment(struct sk_buff *skb,
greh = (struct gre_base_hdr *)skb_transport_header(skb);
- ghl = skb_inner_network_header(skb) - skb_transport_header(skb);
+ ghl = skb_inner_mac_header(skb) - skb_transport_header(skb);
if (unlikely(ghl < sizeof(*greh)))
goto out;
^ permalink raw reply related
* RE: [net-next 03/17] i40e: allow various base numbers in debugfs aq commands
From: David Laight @ 2014-11-24 16:50 UTC (permalink / raw)
To: 'Jeff Kirsher', davem@davemloft.net
Cc: Shannon Nelson, netdev@vger.kernel.org, nhorman@redhat.com,
sassmann@redhat.com, jogreene@redhat.com
In-Reply-To: <1416635708-4765-5-git-send-email-jeffrey.t.kirsher@intel.com>
From: Jeff Kirsher
> From: Shannon Nelson <shannon.nelson@intel.com>
>
> Use the 'i' rather than the more restrictive 'x' or 'd' in the aq_cmd
> arguments. This makes the user interface much more forgiving and user
> friendly.
>
> Change-ID: I5dcd57b9befc047e06b74cf1152a25a3fa9e1309
> Signed-off-by: Shannon Nelson <shannon.nelson@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> ---
> drivers/net/ethernet/intel/i40e/i40e_debugfs.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> b/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> index 3a3c237..16ac3f8 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> @@ -1493,7 +1493,7 @@ static ssize_t i40e_dbg_command_write(struct file *filp,
> if (!desc)
> goto command_write_done;
> cnt = sscanf(&cmd_buf[11],
> - "%hx %hx %hx %hx %x %x %x %x %x %x",
> + "%hi %hi %hi %hi %i %i %i %i %i %i",
Isn't that an API change?
Anything that used to specify "10" will now get 10 instead of 16.
So if this has been in a release kernel you probably shouldn't change it.
David
> &desc->flags,
> &desc->opcode, &desc->datalen, &desc->retval,
> &desc->cookie_high, &desc->cookie_low,
> @@ -1541,7 +1541,7 @@ static ssize_t i40e_dbg_command_write(struct file *filp,
> if (!desc)
> goto command_write_done;
> cnt = sscanf(&cmd_buf[20],
> - "%hx %hx %hx %hx %x %x %x %x %x %x %hd",
> + "%hi %hi %hi %hi %i %i %i %i %i %i %hi",
> &desc->flags,
> &desc->opcode, &desc->datalen, &desc->retval,
> &desc->cookie_high, &desc->cookie_low,
> --
> 1.9.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* qstats update in packet scheduling
From: Josh Clark @ 2014-11-24 17:06 UTC (permalink / raw)
To: netdev
Hi everyone,
I'm working on a research project to add some extra functionality to
RED. Right now, I'm tracking qstats to look at the factors that RED
uses to calculate marking probability. Using standard RED with printk
debug statements, I'm seeing that there are some drops, but no change
in qlen or backlog when I send 12Mbps through a 10Mbps link.
Does RED use some other means to update qstats and track backlog, or
do I need to change how I'm sending the test traffic though?
Thank you for all your help.
-Josh Clark
^ permalink raw reply
* Re: net_tx_action race condition?
From: Angelo Rizzi @ 2014-11-24 17:19 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, linux-kernel@vger.kernel.org
In-Reply-To: <1416843200.17888.53.camel@edumazet-glaptop2.roam.corp.google.com>
Hi Eric,
Thanks for your reply
You are right, it seems a bug in the NIOS2 architecture port.
I will check how local_irq_disable()/local_irq_enable() is implemented
on this kind of architecture.
Regards,
Angelo
Il 24/11/2014 16:33, Eric Dumazet ha scritto:
> On Mon, 2014-11-24 at 14:29 +0100, Angelo Rizzi wrote:
>> Hi Daniel,
>> Here attached the patch file you required.
>> The problem i've found is on the declaration of 'struct softnet_data
>> *sd' in function 'net_tx_action'
>> What happens to me (i have an embedded system based on FPGA and a NIOS2
>> cpu) is that, due to compiler optimization, the content of
>> 'sd->completion_queue' is saved in a CPU register before interrupt
>> disabling (when the instruction 'if (sd->completion_queue) {' is
>> executed) and then the register contents is used for interrupt-disabled
>> assignment ('clist = sd->completion_queue') instead of re-read the
>> variable contents.
>> This seems to lead to a race condition when an interrupt modifies the
>> content of 'sd->completion_queue' between these two instructions.
>> What i have done to avoid this situation is to change the declaration of
>> 'struct softnet_data *sd' to 'volatile struct softnet_data *sd' and now
>> everything seems to be ok.
>> I hope this will help.
>>
>> Regards,
>> Angelo
>>
> Do not add volatile in the kernel, this is not how we solve this kind of
> problems. Documentation/memory-barriers.txt and
>
> Documentation/00-INDEX:476:volatile-considered-harmful.txt
> Documentation/00-INDEX:477: - Why the "volatile" type class should not be used
>
>
> I am surprised this patch is needed. Many other 'bugs' would need
> similar fixes.
>
> local_irq_disable() MUST have a memory barrier. This looks like a bug in
> one particular arch implementation.
>
>
> On x86 for example, native_irq_disable() is really :
>
> asm volatile("cli": : :"memory");
>
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index ac4836241a965a71952469ba054f87d8dfca2d32..fa73072e515aa07fa8ae1bc39174b7d59c7a31a5 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3406,7 +3406,7 @@ static void net_tx_action(struct softirq_action *h)
> struct sk_buff *clist;
>
> local_irq_disable();
> - clist = sd->completion_queue;
> + clist = ACCESS_ONCE(sd->completion_queue);
> sd->completion_queue = NULL;
> local_irq_enable();
>
>
>
>
^ permalink raw reply
* RE: [net-next 03/17] i40e: allow various base numbers in debugfs aq commands
From: Nelson, Shannon @ 2014-11-24 17:26 UTC (permalink / raw)
To: David Laight, Kirsher, Jeffrey T, davem@davemloft.net
Cc: netdev@vger.kernel.org, nhorman@redhat.com, sassmann@redhat.com,
jogreene@redhat.com
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6D1C9FA07E@AcuExch.aculab.com>
> -----Original Message-----
> From: David Laight [mailto:David.Laight@ACULAB.COM]
> Sent: Monday, November 24, 2014 8:51 AM
> To: Kirsher, Jeffrey T; davem@davemloft.net
> Cc: Nelson, Shannon; netdev@vger.kernel.org; nhorman@redhat.com;
> sassmann@redhat.com; jogreene@redhat.com
> Subject: RE: [net-next 03/17] i40e: allow various base numbers in
> debugfs aq commands
>
> From: Jeff Kirsher
> > From: Shannon Nelson <shannon.nelson@intel.com>
> >
> > Use the 'i' rather than the more restrictive 'x' or 'd' in the aq_cmd
> > arguments. This makes the user interface much more forgiving and user
> > friendly.
> >
> > Change-ID: I5dcd57b9befc047e06b74cf1152a25a3fa9e1309
> > Signed-off-by: Shannon Nelson <shannon.nelson@intel.com>
> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> > ---
> > drivers/net/ethernet/intel/i40e/i40e_debugfs.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> > b/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> > index 3a3c237..16ac3f8 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> > @@ -1493,7 +1493,7 @@ static ssize_t i40e_dbg_command_write(struct
> file *filp,
> > if (!desc)
> > goto command_write_done;
> > cnt = sscanf(&cmd_buf[11],
> > - "%hx %hx %hx %hx %x %x %x %x %x %x",
> > + "%hi %hi %hi %hi %i %i %i %i %i %i",
>
> Isn't that an API change?
> Anything that used to specify "10" will now get 10 instead of 16.
>
> So if this has been in a release kernel you probably shouldn't change
> it.
>
> David
Thanks, David, for looking through our code.
If this were in any other part of the kernel, I would agree with you. However, this is in our debugfs module, which is not intended for general use. Following the original definition, there is no expectation of a stable ABI - of course, as discussed in http://lwn.net/Articles/309298/ this is a bit of a pipe dream, but the concept does remain. As it is, we have already removed a couple of commands, and will likely remove more in the near future as more features are exposed in ethtool and other more standard tools.
The other mitigating factors in my mind are that (a) this is still a very new device, only been in customer hands for a short time, and (b) this particular command is painfully obtuse. Probably not many have figured out that it is there or how to use it, and the input specs were inconsistent (note the %hd at the end of the second sscanf()). This patch gives the UI more consistency with standard UI concepts for the aspect of least-surprise, eg forces express base-16 and base-8 format, and it gets this fix in before others likely have run into it.
sln
^ permalink raw reply
* Re: qstats update in packet scheduling
From: Eric Dumazet @ 2014-11-24 17:31 UTC (permalink / raw)
To: Josh Clark; +Cc: netdev
In-Reply-To: <CAHmvzZQnBJbx3hfdvpXg0TZM-UooKxjM+-1HO6nSSpx4qJLcaw@mail.gmail.com>
On Mon, 2014-11-24 at 12:06 -0500, Josh Clark wrote:
> Hi everyone,
>
> I'm working on a research project to add some extra functionality to
> RED. Right now, I'm tracking qstats to look at the factors that RED
> uses to calculate marking probability. Using standard RED with printk
> debug statements, I'm seeing that there are some drops, but no change
> in qlen or backlog when I send 12Mbps through a 10Mbps link.
>
> Does RED use some other means to update qstats and track backlog, or
> do I need to change how I'm sending the test traffic though?
With steady traffic, there is no reason qlen should vary.
It should reach some equilibrium as well.
^ permalink raw reply
* Re: [PATCH] mdio-mux-gpio: Use GPIO descriptor interface and new gpiod_set_array function
From: David Miller @ 2014-11-24 18:06 UTC (permalink / raw)
To: imr; +Cc: f.fainelli, netdev, david.daney, linux-gpio, linus.walleij,
acourbot
In-Reply-To: <1671914.Rot4bT9ZsP@pcimr>
From: Rojhalat Ibrahim <imr@rtschenk.de>
Date: Mon, 24 Nov 2014 09:35:02 +0100
> On Friday 21 November 2014 15:13:01 David Miller wrote:
>> From: Rojhalat Ibrahim <imr@rtschenk.de>
>> Date: Thu, 20 Nov 2014 13:24:44 +0100
>>
>> > Convert mdio-mux-gpio to the GPIO descriptor interface and use the new
>> > gpiod_set_array function to set all output signals simultaneously.
>> >
>> > Signed-off-by: Rojhalat Ibrahim <imr@rtschenk.de>
>> > --
>> > This patch depends on the gpiod_set_array function, which is available in
>> > the linux-gpio devel tree.
>>
>> Then I really can't apply it to the networking GIT tree.
>>
>
> Then maybe you could ack the patch and then Linus Walleij could apply it to
> the GPIO tree, right?
Yep, I can do that:
Acked-by: David S. Miller <davem@davemloft.net>
^ permalink raw reply
* Re: GRE with GRO very slow when forwarding starting with 3.14.24
From: Tom Herbert @ 2014-11-24 18:16 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Wolfgang Walter, Linux Netdev List, Alexander Duyck
In-Reply-To: <1416847696.17888.64.camel@edumazet-glaptop2.roam.corp.google.com>
On Mon, Nov 24, 2014 at 8:48 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> On Mon, 2014-11-24 at 17:13 +0100, Wolfgang Walter wrote:
> > Hello,
> >
> > starting with 3.14.24 GRE with GRO on is very slow. To be more specific:
> >
> > yyyy <--> GRO_endpoint <-_> .... <--> |eth0<->GRO-endpoint | eth1 |<-> xxxx
> >
> > routing (IPv4) between xxxx and yyyy is very slow when GRO is enabled on eth0
> > and/or eth1 starting with stable kernel 3.14.24
> >
> > Regards,
>
>
> tcpdump might help, but I presume GSO is no longer working properly on
> egress.
>
Inner mac header is probably not being set in GRO->GSO GRE path.
Please try this also:
diff --git a/net/ipv4/gre_offload.c b/net/ipv4/gre_offload.c
index bb5947b..51973dd 100644
--- a/net/ipv4/gre_offload.c
+++ b/net/ipv4/gre_offload.c
@@ -247,6 +247,9 @@ static int gre_gro_complete(struct sk_buff *skb, int nhoff)
err = ptype->callbacks.gro_complete(skb, nhoff + grehlen);
rcu_read_unlock();
+
+ skb_set_inner_mac_header(skb, nhoff + grehlen);
+
return err;
}
> Can you try to revert :
>
> commit abe640984aa492652232b65d3579361cf6d461f5
> Author: Tom Herbert <therbert@google.com>
> Date: Thu Oct 30 08:40:56 2014 -0700
>
> gre: Use inner mac length when computing tunnel length
>
> [ Upstream commit 14051f0452a2c26a3f4791e6ad6a435e8f1945ff ]
>
> Currently, skb_inner_network_header is used but this does not account
> for Ethernet header for ETH_P_TEB. Use skb_inner_mac_header which
> handles TEB and also should work with IP encapsulation in which case
> inner mac and inner network headers are the same.
>
> Tested: Ran TCP_STREAM over GRE, worked as expected.
>
> Signed-off-by: Tom Herbert <therbert@google.com>
> Acked-by: Alexander Duyck <alexander.h.duyck@redhat.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> diff --git a/net/ipv4/gre_offload.c b/net/ipv4/gre_offload.c
> index 2d24f293f977..8c8493ea6b1c 100644
> --- a/net/ipv4/gre_offload.c
> +++ b/net/ipv4/gre_offload.c
> @@ -50,7 +50,7 @@ static struct sk_buff *gre_gso_segment(struct sk_buff *skb,
>
> greh = (struct gre_base_hdr *)skb_transport_header(skb);
>
> - ghl = skb_inner_network_header(skb) - skb_transport_header(skb);
> + ghl = skb_inner_mac_header(skb) - skb_transport_header(skb);
> if (unlikely(ghl < sizeof(*greh)))
> goto out;
>
>
^ permalink raw reply related
* Re: qstats update in packet scheduling
From: Josh Clark @ 2014-11-24 18:17 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1416850289.17888.67.camel@edumazet-glaptop2.roam.corp.google.com>
On Mon, Nov 24, 2014 at 12:31 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Mon, 2014-11-24 at 12:06 -0500, Josh Clark wrote:
>> Hi everyone,
>>
>> I'm working on a research project to add some extra functionality to
>> RED. Right now, I'm tracking qstats to look at the factors that RED
>> uses to calculate marking probability. Using standard RED with printk
>> debug statements, I'm seeing that there are some drops, but no change
>> in qlen or backlog when I send 12Mbps through a 10Mbps link.
>>
>> Does RED use some other means to update qstats and track backlog, or
>> do I need to change how I'm sending the test traffic though?
>
> With steady traffic, there is no reason qlen should vary.
>
> It should reach some equilibrium as well.
>
But I'm seeing no change at all in qlen. qlen and backlog are both
zero, which results in zero qavg, meaning I can't test the RED
functionality at all.
^ 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