* [Patch v3 3/3] ipv4: Namespaceify tcp_fastopen_blackhole_timeout knob
From: Haishuang Yan @ 2017-09-19 9:38 UTC (permalink / raw)
To: David S. Miller, Alexey Kuznetsov, Eric Dumazet, Wei Wang,
Luca BRUNO
Cc: netdev, linux-kernel, Haishuang Yan
In-Reply-To: <1505813896-12121-1-git-send-email-yanhaishuang@cmss.chinamobile.com>
Different namespace application might require different time period in
second to disable Fastopen on active TCP sockets.
Tested:
Simulate following similar situation that the server's data gets dropped
after 3WHS.
C ---- syn-data ---> S
C <--- syn/ack ----- S
C ---- ack --------> S
S (accept & write)
C? X <- data ------ S
[retry and timeout]
And then print netstat of TCPFastOpenBlackhole, the counter increased as
expected when the firewall blackhole issue is detected and active TFO is
disabled.
# cat /proc/net/netstat | awk '{print $91}'
TCPFastOpenBlackhole
1
Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>
---
include/net/netns/ipv4.h | 3 +++
net/ipv4/sysctl_net_ipv4.c | 20 +++++++++++---------
net/ipv4/tcp_fastopen.c | 28 ++++++++++------------------
net/ipv4/tcp_ipv4.c | 2 ++
4 files changed, 26 insertions(+), 27 deletions(-)
diff --git a/include/net/netns/ipv4.h b/include/net/netns/ipv4.h
index 66b8335..d76edde 100644
--- a/include/net/netns/ipv4.h
+++ b/include/net/netns/ipv4.h
@@ -132,6 +132,9 @@ struct netns_ipv4 {
int sysctl_tcp_fastopen;
struct tcp_fastopen_context __rcu *tcp_fastopen_ctx;
spinlock_t tcp_fastopen_ctx_lock;
+ unsigned int sysctl_tcp_fastopen_blackhole_timeout;
+ atomic_t tfo_active_disable_times;
+ unsigned long tfo_active_disable_stamp;
#ifdef CONFIG_NET_L3_MASTER_DEV
int sysctl_udp_l3mdev_accept;
diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
index 20e19fe..cac8dd3 100644
--- a/net/ipv4/sysctl_net_ipv4.c
+++ b/net/ipv4/sysctl_net_ipv4.c
@@ -355,11 +355,13 @@ static int proc_tfo_blackhole_detect_timeout(struct ctl_table *table,
void __user *buffer,
size_t *lenp, loff_t *ppos)
{
+ struct net *net = container_of(table->data, struct net,
+ ipv4.sysctl_tcp_fastopen_blackhole_timeout);
int ret;
ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
if (write && ret == 0)
- tcp_fastopen_active_timeout_reset();
+ atomic_set(&net->ipv4.tfo_active_disable_times, 0);
return ret;
}
@@ -398,14 +400,6 @@ static int proc_tcp_available_ulp(struct ctl_table *ctl,
.proc_handler = proc_dointvec
},
{
- .procname = "tcp_fastopen_blackhole_timeout_sec",
- .data = &sysctl_tcp_fastopen_blackhole_timeout,
- .maxlen = sizeof(int),
- .mode = 0644,
- .proc_handler = proc_tfo_blackhole_detect_timeout,
- .extra1 = &zero,
- },
- {
.procname = "tcp_abort_on_overflow",
.data = &sysctl_tcp_abort_on_overflow,
.maxlen = sizeof(int),
@@ -1083,6 +1077,14 @@ static int proc_tcp_available_ulp(struct ctl_table *ctl,
.maxlen = ((TCP_FASTOPEN_KEY_LENGTH * 2) + 10),
.proc_handler = proc_tcp_fastopen_key,
},
+ {
+ .procname = "tcp_fastopen_blackhole_timeout_sec",
+ .data = &init_net.ipv4.sysctl_tcp_fastopen_blackhole_timeout,
+ .maxlen = sizeof(int),
+ .mode = 0644,
+ .proc_handler = proc_tfo_blackhole_detect_timeout,
+ .extra1 = &zero,
+ },
#ifdef CONFIG_IP_ROUTE_MULTIPATH
{
.procname = "fib_multipath_use_neigh",
diff --git a/net/ipv4/tcp_fastopen.c b/net/ipv4/tcp_fastopen.c
index 21783c7..f3b00a9 100644
--- a/net/ipv4/tcp_fastopen.c
+++ b/net/ipv4/tcp_fastopen.c
@@ -407,25 +407,16 @@ bool tcp_fastopen_defer_connect(struct sock *sk, int *err)
* TFO connection with data exchanges.
*/
-/* Default to 1hr */
-unsigned int sysctl_tcp_fastopen_blackhole_timeout __read_mostly = 60 * 60;
-static atomic_t tfo_active_disable_times __read_mostly = ATOMIC_INIT(0);
-static unsigned long tfo_active_disable_stamp __read_mostly;
-
/* Disable active TFO and record current jiffies and
* tfo_active_disable_times
*/
void tcp_fastopen_active_disable(struct sock *sk)
{
- atomic_inc(&tfo_active_disable_times);
- tfo_active_disable_stamp = jiffies;
- NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPFASTOPENBLACKHOLE);
-}
+ struct net *net = sock_net(sk);
-/* Reset tfo_active_disable_times to 0 */
-void tcp_fastopen_active_timeout_reset(void)
-{
- atomic_set(&tfo_active_disable_times, 0);
+ atomic_inc(&net->ipv4.tfo_active_disable_times);
+ net->ipv4.tfo_active_disable_stamp = jiffies;
+ NET_INC_STATS(net, LINUX_MIB_TCPFASTOPENBLACKHOLE);
}
/* Calculate timeout for tfo active disable
@@ -434,17 +425,18 @@ void tcp_fastopen_active_timeout_reset(void)
*/
bool tcp_fastopen_active_should_disable(struct sock *sk)
{
- int tfo_da_times = atomic_read(&tfo_active_disable_times);
+ int tfo_da_times = atomic_read(&sock_net(sk)->ipv4.tfo_active_disable_times);
int multiplier;
unsigned long timeout;
+ unsigned int tfo_bh_timeout = sock_net(sk)->ipv4.sysctl_tcp_fastopen_blackhole_timeout;
if (!tfo_da_times)
return false;
/* Limit timout to max: 2^6 * initial timeout */
multiplier = 1 << min(tfo_da_times - 1, 6);
- timeout = multiplier * sysctl_tcp_fastopen_blackhole_timeout * HZ;
- if (time_before(jiffies, tfo_active_disable_stamp + timeout))
+ timeout = multiplier * tfo_bh_timeout * HZ;
+ if (time_before(jiffies, sock_net(sk)->ipv4.tfo_active_disable_stamp + timeout))
return true;
/* Mark check bit so we can check for successful active TFO
@@ -480,10 +472,10 @@ void tcp_fastopen_active_disable_ofo_check(struct sock *sk)
}
}
} else if (tp->syn_fastopen_ch &&
- atomic_read(&tfo_active_disable_times)) {
+ atomic_read(&sock_net(sk)->ipv4.tfo_active_disable_times)) {
dst = sk_dst_get(sk);
if (!(dst && dst->dev && (dst->dev->flags & IFF_LOOPBACK)))
- tcp_fastopen_active_timeout_reset();
+ atomic_set(&sock_net(sk)->ipv4.tfo_active_disable_times, 0);
dst_release(dst);
}
}
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 29e042c..4b7aedb 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -2474,6 +2474,8 @@ static int __net_init tcp_sk_init(struct net *net)
net->ipv4.sysctl_tcp_fastopen = TFO_CLIENT_ENABLE;
spin_lock_init(&net->ipv4.tcp_fastopen_ctx_lock);
+ net->ipv4.sysctl_tcp_fastopen_blackhole_timeout = 60 * 60;
+ atomic_set(&net->ipv4.tfo_active_disable_times, 0);
return 0;
fail:
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH net-next 5/5] tls: Add generic NIC offload infrastructure.
From: Hannes Frederic Sowa @ 2017-09-19 9:40 UTC (permalink / raw)
To: Ilya Lesokhin
Cc: netdev@vger.kernel.org, davem@davemloft.net, davejwatson@fb.com,
tom@herbertland.com, Boris Pismenny, Aviad Yehezkel, Liran Liss
In-Reply-To: <AM4PR0501MB2723F6F5A22955102CDC2886D4600@AM4PR0501MB2723.eurprd05.prod.outlook.com>
Hello,
Ilya Lesokhin <ilyal@mellanox.com> writes:
> Hannes Frederic Sowa <hannes@stressinduktion.org> writes:
>
>> The user should be aware of that they can't migrate the socket to another
>> interface if they got hw offloaded. This is not the case for software offload.
>> Thus I think the user has to opt in and it shouldn't be a heuristic until we can
>> switch back to sw offload path.
>>
>> Maybe change flowi_oif to sk_bound_dev_if and somwhow lock it against
>> further changes if hw tls is in use?
>>
>
> I'm not sure I follow.
> We do set sk->sk_bound_dev_if to prevent further changes.
>
> Do you recommend we enable TLS offload only if SO_BINDTODEVICE
> was previously used on that socket?
> and prevent even users with CAP_NET_RAW from unbinding it?
>
> I would rather avoid requiring CAP_NET_RAW to use TLS offload.
> But admittedly I'm not sure setting sk->sk_bound_dev_if
> without CAP_NET_RAW like we do is legit either.
>
> Finally, the reason we made HW offload the default is that the user
> can use sudo ethtool -K enp0s4 tls-hw-tx-offload off to opt out of HW offload
> and we currently don't have anything equivalent for opting out of SW KTLS.
IMHO the decision if a TCP flow should be bounded to hw and thus never
push traffic to another interface should a decision the administrator
and the application should opt in. You might have your management
application which is accessible over multiple interfaces and your
production application which might want to use hw offloaded tls. Thus I
don't think only a single ethtool knob will do it.
I agree that SO_BINDTODEVICE is bad for this use case. First, the
CAP_NET_RAW limitation seems annoying and we don't want to enforce TLS
apps to have this capability. Second, the user space application doesn't
care which interface it should talk to (maybe?) but leave the routing
decision to the kernel and just opt in to TLS. SO_BINDTODEVICE doesn't
allow this.
sk_bound_dev_if can be rebound later with CAP_NET_RAW privileges, will
this be a problem?
Have you thought how the user space will configure the various
offloading features (sw, hw, none)? Will it in e.g. OpenSSL be part of
the Cipher Spec or will there be new functions around SSL_CTX to do so?
Maybe an enhancement of the TLS_TX setsockopt with a boolean for hw
offload is a solution?
Another question:
How is the dependency management done between socket layer and driver
layer? It seems a bit cyclic but judging from this code you don't hold
references to the device (dev_hold) (which is good, you don't want to
have users creating refs to devices). OTOH you somehow need to match
sockets from the device layer up to the socket. Will those be reference
counted or does that work without?
Thanks,
Hannes
^ permalink raw reply
* [PATCH net-next 1/3] virtio-net: remove unnecessary parameter of virtnet_xdp_xmit()
From: Jason Wang @ 2017-09-19 9:42 UTC (permalink / raw)
To: mst, jasowang, virtualization, netdev, linux-kernel; +Cc: John Fastabend
CC: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/net/virtio_net.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 511f833..a0ef4b0 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -373,7 +373,6 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
}
static bool virtnet_xdp_xmit(struct virtnet_info *vi,
- struct receive_queue *rq,
struct xdp_buff *xdp)
{
struct virtio_net_hdr_mrg_rxbuf *hdr;
@@ -542,7 +541,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
delta = orig_data - xdp.data;
break;
case XDP_TX:
- if (unlikely(!virtnet_xdp_xmit(vi, rq, &xdp)))
+ if (unlikely(!virtnet_xdp_xmit(vi, &xdp)))
trace_xdp_exception(vi->dev, xdp_prog, act);
rcu_read_unlock();
goto xdp_xmit;
@@ -677,7 +676,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
}
break;
case XDP_TX:
- if (unlikely(!virtnet_xdp_xmit(vi, rq, &xdp)))
+ if (unlikely(!virtnet_xdp_xmit(vi, &xdp)))
trace_xdp_exception(vi->dev, xdp_prog, act);
ewma_pkt_len_add(&rq->mrg_avg_pkt_len, len);
if (unlikely(xdp_page != page))
--
2.7.4
^ permalink raw reply related
* [PATCH net-next 2/3] virtio-net: add packet len average only when needed during XDP
From: Jason Wang @ 2017-09-19 9:42 UTC (permalink / raw)
To: mst, jasowang, virtualization, netdev, linux-kernel; +Cc: John Fastabend
In-Reply-To: <1505814163-7315-1-git-send-email-jasowang@redhat.com>
There's no need to add packet len average in the case of XDP_PASS
since it will be done soon after skb is created.
Cc: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/net/virtio_net.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index a0ef4b0..db5924c 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -656,6 +656,9 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
xdp.data_end = xdp.data + (len - vi->hdr_len);
act = bpf_prog_run_xdp(xdp_prog, &xdp);
+ if (act != XDP_PASS)
+ ewma_pkt_len_add(&rq->mrg_avg_pkt_len, len);
+
switch (act) {
case XDP_PASS:
/* recalculate offset to account for any header
@@ -671,14 +674,12 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
put_page(page);
head_skb = page_to_skb(vi, rq, xdp_page,
offset, len, PAGE_SIZE);
- ewma_pkt_len_add(&rq->mrg_avg_pkt_len, len);
return head_skb;
}
break;
case XDP_TX:
if (unlikely(!virtnet_xdp_xmit(vi, &xdp)))
trace_xdp_exception(vi->dev, xdp_prog, act);
- ewma_pkt_len_add(&rq->mrg_avg_pkt_len, len);
if (unlikely(xdp_page != page))
goto err_xdp;
rcu_read_unlock();
@@ -690,7 +691,6 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
case XDP_DROP:
if (unlikely(xdp_page != page))
__free_pages(xdp_page, 0);
- ewma_pkt_len_add(&rq->mrg_avg_pkt_len, len);
goto err_xdp;
}
}
--
2.7.4
^ permalink raw reply related
* [PATCH net-next 3/3] virtio-net: support XDP_REDIRECT
From: Jason Wang @ 2017-09-19 9:42 UTC (permalink / raw)
To: mst, jasowang, virtualization, netdev, linux-kernel; +Cc: John Fastabend
In-Reply-To: <1505814163-7315-1-git-send-email-jasowang@redhat.com>
This patch tries to add XDP_REDIRECT for virtio-net. The changes are
not complex as we could use exist XDP_TX helpers for most of the
work. The rest is passing the XDP_TX to NAPI handler for implementing
batching.
Cc: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/net/virtio_net. | 0
drivers/net/virtio_net.c | 77 ++++++++++++++++++++++++++++++++++++++----------
2 files changed, 62 insertions(+), 15 deletions(-)
create mode 100644 drivers/net/virtio_net.
diff --git a/drivers/net/virtio_net. b/drivers/net/virtio_net.
new file mode 100644
index 0000000..e69de29
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index db5924c..f6c1f13 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -29,6 +29,7 @@
#include <linux/slab.h>
#include <linux/cpu.h>
#include <linux/average.h>
+#include <linux/filter.h>
#include <net/route.h>
static int napi_weight = NAPI_POLL_WEIGHT;
@@ -372,8 +373,20 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
return skb;
}
-static bool virtnet_xdp_xmit(struct virtnet_info *vi,
- struct xdp_buff *xdp)
+static void virtnet_xdp_flush(struct net_device *dev)
+{
+ struct virtnet_info *vi = netdev_priv(dev);
+ struct send_queue *sq;
+ unsigned int qp;
+
+ qp = vi->curr_queue_pairs - vi->xdp_queue_pairs + smp_processor_id();
+ sq = &vi->sq[qp];
+
+ virtqueue_kick(sq->vq);
+}
+
+static bool __virtnet_xdp_xmit(struct virtnet_info *vi,
+ struct xdp_buff *xdp)
{
struct virtio_net_hdr_mrg_rxbuf *hdr;
unsigned int len;
@@ -407,10 +420,19 @@ static bool virtnet_xdp_xmit(struct virtnet_info *vi,
return false;
}
- virtqueue_kick(sq->vq);
return true;
}
+static int virtnet_xdp_xmit(struct net_device *dev, struct xdp_buff *xdp)
+{
+ struct virtnet_info *vi = netdev_priv(dev);
+ bool sent = __virtnet_xdp_xmit(vi, xdp);
+
+ if (!sent)
+ return -ENOSPC;
+ return 0;
+}
+
static unsigned int virtnet_get_headroom(struct virtnet_info *vi)
{
return vi->xdp_queue_pairs ? VIRTIO_XDP_HEADROOM : 0;
@@ -483,7 +505,8 @@ static struct sk_buff *receive_small(struct net_device *dev,
struct virtnet_info *vi,
struct receive_queue *rq,
void *buf, void *ctx,
- unsigned int len)
+ unsigned int len,
+ bool *xdp_xmit)
{
struct sk_buff *skb;
struct bpf_prog *xdp_prog;
@@ -493,7 +516,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
unsigned int buflen = SKB_DATA_ALIGN(GOOD_PACKET_LEN + headroom) +
SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
struct page *page = virt_to_head_page(buf);
- unsigned int delta = 0;
+ unsigned int delta = 0, err;
struct page *xdp_page;
len -= vi->hdr_len;
@@ -541,8 +564,16 @@ static struct sk_buff *receive_small(struct net_device *dev,
delta = orig_data - xdp.data;
break;
case XDP_TX:
- if (unlikely(!virtnet_xdp_xmit(vi, &xdp)))
+ if (unlikely(!__virtnet_xdp_xmit(vi, &xdp)))
trace_xdp_exception(vi->dev, xdp_prog, act);
+ else
+ *xdp_xmit = true;
+ rcu_read_unlock();
+ goto xdp_xmit;
+ case XDP_REDIRECT:
+ err = xdp_do_redirect(dev, &xdp, xdp_prog);
+ if (!err)
+ *xdp_xmit = true;
rcu_read_unlock();
goto xdp_xmit;
default:
@@ -603,7 +634,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
struct receive_queue *rq,
void *buf,
void *ctx,
- unsigned int len)
+ unsigned int len,
+ bool *xdp_xmit)
{
struct virtio_net_hdr_mrg_rxbuf *hdr = buf;
u16 num_buf = virtio16_to_cpu(vi->vdev, hdr->num_buffers);
@@ -613,6 +645,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
struct bpf_prog *xdp_prog;
unsigned int truesize;
unsigned int headroom = mergeable_ctx_to_headroom(ctx);
+ int err;
head_skb = NULL;
@@ -678,12 +711,20 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
}
break;
case XDP_TX:
- if (unlikely(!virtnet_xdp_xmit(vi, &xdp)))
+ if (unlikely(!__virtnet_xdp_xmit(vi, &xdp)))
trace_xdp_exception(vi->dev, xdp_prog, act);
+ else
+ *xdp_xmit = true;
if (unlikely(xdp_page != page))
goto err_xdp;
rcu_read_unlock();
goto xdp_xmit;
+ case XDP_REDIRECT:
+ err = xdp_do_redirect(dev, &xdp, xdp_prog);
+ if (err)
+ *xdp_xmit = true;
+ rcu_read_unlock();
+ goto xdp_xmit;
default:
bpf_warn_invalid_xdp_action(act);
case XDP_ABORTED:
@@ -788,7 +829,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
}
static int receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
- void *buf, unsigned int len, void **ctx)
+ void *buf, unsigned int len, void **ctx, bool *xdp_xmit)
{
struct net_device *dev = vi->dev;
struct sk_buff *skb;
@@ -809,11 +850,11 @@ static int receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
}
if (vi->mergeable_rx_bufs)
- skb = receive_mergeable(dev, vi, rq, buf, ctx, len);
+ skb = receive_mergeable(dev, vi, rq, buf, ctx, len, xdp_xmit);
else if (vi->big_packets)
skb = receive_big(dev, vi, rq, buf, len);
else
- skb = receive_small(dev, vi, rq, buf, ctx, len);
+ skb = receive_small(dev, vi, rq, buf, ctx, len, xdp_xmit);
if (unlikely(!skb))
return 0;
@@ -1071,7 +1112,7 @@ static void refill_work(struct work_struct *work)
}
}
-static int virtnet_receive(struct receive_queue *rq, int budget)
+static int virtnet_receive(struct receive_queue *rq, int budget, bool *xdp_xmit)
{
struct virtnet_info *vi = rq->vq->vdev->priv;
unsigned int len, received = 0, bytes = 0;
@@ -1083,13 +1124,13 @@ static int virtnet_receive(struct receive_queue *rq, int budget)
while (received < budget &&
(buf = virtqueue_get_buf_ctx(rq->vq, &len, &ctx))) {
- bytes += receive_buf(vi, rq, buf, len, ctx);
+ bytes += receive_buf(vi, rq, buf, len, ctx, xdp_xmit);
received++;
}
} else {
while (received < budget &&
(buf = virtqueue_get_buf(rq->vq, &len)) != NULL) {
- bytes += receive_buf(vi, rq, buf, len, NULL);
+ bytes += receive_buf(vi, rq, buf, len, NULL, xdp_xmit);
received++;
}
}
@@ -1161,15 +1202,19 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
struct receive_queue *rq =
container_of(napi, struct receive_queue, napi);
unsigned int received;
+ bool xdp_xmit = false;
virtnet_poll_cleantx(rq);
- received = virtnet_receive(rq, budget);
+ received = virtnet_receive(rq, budget, &xdp_xmit);
/* Out of packets? */
if (received < budget)
virtqueue_napi_complete(napi, rq->vq, received);
+ if (xdp_xmit)
+ xdp_do_flush_map();
+
return received;
}
@@ -2069,6 +2114,8 @@ static const struct net_device_ops virtnet_netdev = {
.ndo_poll_controller = virtnet_netpoll,
#endif
.ndo_xdp = virtnet_xdp,
+ .ndo_xdp_xmit = virtnet_xdp_xmit,
+ .ndo_xdp_flush = virtnet_xdp_flush,
.ndo_features_check = passthru_features_check,
};
--
2.7.4
^ permalink raw reply related
* [PATCH net] MAINTAINERS: Remove Yuval Mintz from maintainers list
From: aelior @ 2017-09-19 9:54 UTC (permalink / raw)
To: davem, aelior; +Cc: netdev, everest-linux-l2
From: Ariel Elior <aelior@cavium.com>
Remove Yuval from maintaining the bnx2x & qed* modules as he is no longer
working for the company. Thanks Yuval for your huge contributions and
tireless efforts over the many years and various companies.
Ariel
Signed-off-by: Ariel Elior <aelior@cavium.com>
---
MAINTAINERS | 2 --
1 file changed, 2 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 567343b..5d24dbf 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2801,7 +2801,6 @@ S: Supported
F: drivers/scsi/bnx2i/
BROADCOM BNX2X 10 GIGABIT ETHERNET DRIVER
-M: Yuval Mintz <Yuval.Mintz@cavium.com>
M: Ariel Elior <ariel.elior@cavium.com>
M: everest-linux-l2@cavium.com
L: netdev@vger.kernel.org
@@ -10827,7 +10826,6 @@ S: Supported
F: drivers/scsi/qedi/
QLOGIC QL4xxx ETHERNET DRIVER
-M: Yuval Mintz <Yuval.Mintz@cavium.com>
M: Ariel Elior <Ariel.Elior@cavium.com>
M: everest-linux-l2@cavium.com
L: netdev@vger.kernel.org
--
2.9.4
^ permalink raw reply related
* [PATCH net-next] udp: do rmem bulk free even if the rx sk queue is empty
From: Paolo Abeni @ 2017-09-19 10:11 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Eric Dumazet
The commit 6b229cf77d68 ("udp: add batching to udp_rmem_release()")
reduced greatly the cacheline contention between the BH and the US
reader batching the rmem updates in most scenarios.
Such optimization is explicitly avoided if the US reader is faster
then BH processing.
My fault, I initially suggested this kind of behavior due to concerns
of possible regressions with small sk_rcvbuf values. Tests showed
such concerns are misplaced, so this commit relaxes the condition
for rmem bulk updates, obtaining small but measurable performance
gain in the scenario described above.
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
net/ipv4/udp.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index ef29df8648e4..784ced0b9150 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1212,8 +1212,7 @@ static void udp_rmem_release(struct sock *sk, int size, int partial,
if (likely(partial)) {
up->forward_deficit += size;
size = up->forward_deficit;
- if (size < (sk->sk_rcvbuf >> 2) &&
- !skb_queue_empty(&up->reader_queue))
+ if (size < (sk->sk_rcvbuf >> 2))
return;
} else {
size += up->forward_deficit;
--
2.13.5
^ permalink raw reply related
* [PATCH] rhashtable: Documentation tweak
From: Andreas Gruenbacher @ 2017-09-19 10:41 UTC (permalink / raw)
To: Thomas Graf, Herbert Xu; +Cc: netdev, Andreas Gruenbacher
Clarify that rhashtable_walk_{stop,start} will not reset the iterator to
the beginning of the hash table. Confusion between rhashtable_walk_enter
and rhashtable_walk_start has already lead to a bug.
Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
lib/rhashtable.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/lib/rhashtable.c b/lib/rhashtable.c
index 707ca5d677c6..ddd7dde87c3c 100644
--- a/lib/rhashtable.c
+++ b/lib/rhashtable.c
@@ -735,9 +735,9 @@ EXPORT_SYMBOL_GPL(rhashtable_walk_exit);
* rhashtable_walk_start - Start a hash table walk
* @iter: Hash table iterator
*
- * Start a hash table walk. Note that we take the RCU lock in all
- * cases including when we return an error. So you must always call
- * rhashtable_walk_stop to clean up.
+ * Start a hash table walk at the current iterator position. Note that we take
+ * the RCU lock in all cases including when we return an error. So you must
+ * always call rhashtable_walk_stop to clean up.
*
* Returns zero if successful.
*
@@ -846,7 +846,8 @@ EXPORT_SYMBOL_GPL(rhashtable_walk_next);
* rhashtable_walk_stop - Finish a hash table walk
* @iter: Hash table iterator
*
- * Finish a hash table walk.
+ * Finish a hash table walk. Does not reset the iterator to the start of the
+ * hash table.
*/
void rhashtable_walk_stop(struct rhashtable_iter *iter)
__releases(RCU)
--
2.13.3
^ permalink raw reply related
* [PATCH net-next] net: avoid a full fib lookup when rp_filter is disabled.
From: Paolo Abeni @ 2017-09-19 10:46 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Hannes Frederic Sowa
Since commit 1dced6a85482 ("ipv4: Restore accept_local
behaviour in fib_validate_source()") a full fib lookup
is needed even if the rp_filter is disabled, if
accept_local is false - which is the default.
What we really need in the above scenario is just checking
that the source IP address is not local, and we can do
that is a cheaper way looking up the ifaddr hash table.
This commit adds an helper for such lookup and uses it
to validate the src address when rp_filter is disabled.
It also drops the checks to bail early from
__fib_validate_source, added by the commit 1dced6a85482
("ipv4: Restore accept_local behaviour in fib_validate_source()")
and now unneeded.
This improves UDP performances for unconnected sockets
when rp_filter is disabled by 5% and also gives small but
measurable performance improvement for TCP flood scenarios.
Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
include/linux/inetdevice.h | 1 +
net/ipv4/devinet.c | 14 ++++++++++++++
net/ipv4/fib_frontend.c | 11 +++++++----
3 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/include/linux/inetdevice.h b/include/linux/inetdevice.h
index fb3f809e34e4..751d051f0bc7 100644
--- a/include/linux/inetdevice.h
+++ b/include/linux/inetdevice.h
@@ -179,6 +179,7 @@ __be32 inet_confirm_addr(struct net *net, struct in_device *in_dev, __be32 dst,
__be32 local, int scope);
struct in_ifaddr *inet_ifa_byprefix(struct in_device *in_dev, __be32 prefix,
__be32 mask);
+struct in_ifaddr *inet_lookup_ifaddr_rcu(struct net *net, __be32 addr);
static __inline__ bool inet_ifa_match(__be32 addr, struct in_ifaddr *ifa)
{
return !((addr^ifa->ifa_address)&ifa->ifa_mask);
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index d7adc0616599..73bf09bcfe43 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -173,6 +173,20 @@ struct net_device *__ip_dev_find(struct net *net, __be32 addr, bool devref)
}
EXPORT_SYMBOL(__ip_dev_find);
+/* called under RCU lock */
+struct in_ifaddr *inet_lookup_ifaddr_rcu(struct net *net, __be32 addr)
+{
+ u32 hash = inet_addr_hash(net, addr);
+ struct in_ifaddr *ifa;
+
+ hlist_for_each_entry_rcu(ifa, &inet_addr_lst[hash], hash)
+ if (ifa->ifa_local == addr &&
+ net_eq(dev_net(ifa->ifa_dev->dev), net))
+ return ifa;
+
+ return NULL;
+}
+
static void rtmsg_ifa(int event, struct in_ifaddr *, struct nlmsghdr *, u32);
static BLOCKING_NOTIFIER_HEAD(inetaddr_chain);
diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
index 37819ab4cc74..1470d265a357 100644
--- a/net/ipv4/fib_frontend.c
+++ b/net/ipv4/fib_frontend.c
@@ -345,9 +345,6 @@ static int __fib_validate_source(struct sk_buff *skb, __be32 src, __be32 dst,
if (res.type != RTN_UNICAST &&
(res.type != RTN_LOCAL || !IN_DEV_ACCEPT_LOCAL(idev)))
goto e_inval;
- if (!rpf && !fib_num_tclassid_users(net) &&
- (dev->ifindex != oif || !IN_DEV_TX_REDIRECTS(idev)))
- goto last_resort;
fib_combine_itag(itag, &res);
dev_match = false;
@@ -404,8 +401,14 @@ int fib_validate_source(struct sk_buff *skb, __be32 src, __be32 dst,
int r = secpath_exists(skb) ? 0 : IN_DEV_RPFILTER(idev);
if (!r && !fib_num_tclassid_users(dev_net(dev)) &&
- IN_DEV_ACCEPT_LOCAL(idev) &&
(dev->ifindex != oif || !IN_DEV_TX_REDIRECTS(idev))) {
+ /* we need only to ensure that the src address is not a
+ * local one
+ */
+ if (!IN_DEV_ACCEPT_LOCAL(idev) &&
+ inet_lookup_ifaddr_rcu(dev_net(dev), src))
+ return -EINVAL;
+
*itag = 0;
return 0;
}
--
2.13.5
^ permalink raw reply related
* Re: [REGRESSION] Warning in tcp_fastretrans_alert() of net/ipv4/tcp_input.c
From: Oleksandr Natalenko @ 2017-09-19 11:04 UTC (permalink / raw)
To: Yuchung Cheng
Cc: Neal Cardwell, David S. Miller, Alexey Kuznetsov,
Hideaki YOSHIFUJI, Netdev
In-Reply-To: <CAK6E8=e7SE-M4r2nJ8K_FoDMVcN2xYdtw=MetvD5MvXMm9iA1Q@mail.gmail.com>
Hi.
18.09.2017 23:40, Yuchung Cheng wrote:
> I assume this kernel does not have the patch that Neal proposed in his
> first reply?
Correct.
> The main warning needs to be triggered by another peculiar SACK that
> kicks the sender into recovery again (after undo). Please let it run
> longer if possible to see if we can get both. But the new data does
> indicate the we can (validly) be in CA_Open with retrans_out > 0.
OK, here it is:
===
» LC_TIME=C jctl -kb | grep RIP
…
Sep 19 12:54:03 defiant kernel: RIP:
0010:tcp_undo_cwnd_reduction+0xbd/0xd0
Sep 19 12:54:22 defiant kernel: RIP:
0010:tcp_undo_cwnd_reduction+0xbd/0xd0
Sep 19 12:54:25 defiant kernel: RIP:
0010:tcp_undo_cwnd_reduction+0xbd/0xd0
Sep 19 12:56:00 defiant kernel: RIP:
0010:tcp_fastretrans_alert+0x7c8/0x990
Sep 19 12:57:07 defiant kernel: RIP:
0010:tcp_undo_cwnd_reduction+0xbd/0xd0
Sep 19 12:57:14 defiant kernel: RIP:
0010:tcp_undo_cwnd_reduction+0xbd/0xd0
Sep 19 12:58:04 defiant kernel: RIP:
0010:tcp_undo_cwnd_reduction+0xbd/0xd0
…
===
Note timestamps — two types of warning are distant in time, so didn't
happen at once.
While still running this kernel, anything else I can check for you?
^ permalink raw reply
* Re: [PATCHv2 iproute2 1/2] lib/libnetlink: re malloc buff if size is not enough
From: Michal Kubecek @ 2017-09-19 11:48 UTC (permalink / raw)
To: Hangbin Liu; +Cc: netdev, Stephen Hemminger, Phil Sutter
In-Reply-To: <20170919030520.GL5465@leo.usersys.redhat.com>
On Tue, Sep 19, 2017 at 11:05:20AM +0800, Hangbin Liu wrote:
> On Mon, Sep 18, 2017 at 09:55:05AM +0200, Michal Kubecek wrote:
> > > @@ -471,19 +516,23 @@ int rtnl_dump_filter_l(struct rtnl_handle *rth,
> > >
> > > if (h->nlmsg_type == NLMSG_ERROR) {
> > > rtnl_dump_error(rth, h);
> > > + free(buf);
> > > return -1;
> > > }
> > >
> > > if (!rth->dump_fp) {
> > > err = a->filter(&nladdr, h, a->arg1);
> > > - if (err < 0)
> > > + if (err < 0) {
> > > + free(buf);
> > > return err;
> > > + }
> > > }
> > >
> > > skip_it:
> > > h = NLMSG_NEXT(h, msglen);
> > > }
> > > }
> > > + free(buf);
> >
> > We only free the last buffer returned by rtnl_recvmsg() this way.
> > IMHO this free(buf) should be moved inside the loop.
>
> Do you mean the outside while loop or the for loop? I think we could
> not put it inside the for loop, because we may need the buf multi
> times based on arg.
Sorry for the confusion, you are right, this part is correct. I misread
the indentation.
Michal Kubecek
^ permalink raw reply
* Re: [PATCH net-next 13/14] gtp: Support for GRO
From: kbuild test robot @ 2017-09-19 11:57 UTC (permalink / raw)
To: Tom Herbert; +Cc: kbuild-all, davem, netdev, pablo, laforge, rohit, Tom Herbert
In-Reply-To: <20170919003904.5124-14-tom@quantonium.net>
Hi Tom,
[auto build test WARNING on net-next/master]
url: https://github.com/0day-ci/linux/commits/Tom-Herbert/gtp-Additional-feature-support/20170919-143920
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
vim +3958 include/linux/netdevice.h
5b33bc6e Tom Herbert 2017-09-18 3944
5b33bc6e Tom Herbert 2017-09-18 3945 /* rcu_read_lock() must be held */
5b33bc6e Tom Herbert 2017-09-18 3946 static inline struct skb_gso_app *skb_gso_app_lookup(struct sk_buff *skb,
5b33bc6e Tom Herbert 2017-09-18 3947 netdev_features_t features,
5b33bc6e Tom Herbert 2017-09-18 3948 unsigned int check_flags)
5b33bc6e Tom Herbert 2017-09-18 3949 {
5b33bc6e Tom Herbert 2017-09-18 3950 struct skb_gso_app *app;
5b33bc6e Tom Herbert 2017-09-18 3951 int type;
5b33bc6e Tom Herbert 2017-09-18 3952
5b33bc6e Tom Herbert 2017-09-18 3953 if (!(skb_shinfo(skb)->gso_type & SKB_GSO_APP_MASK))
5b33bc6e Tom Herbert 2017-09-18 3954 return false;
5b33bc6e Tom Herbert 2017-09-18 3955
5b33bc6e Tom Herbert 2017-09-18 3956 type = skb_gso_app_to_index(skb_shinfo(skb)->gso_type);
5b33bc6e Tom Herbert 2017-09-18 3957
5b33bc6e Tom Herbert 2017-09-18 @3958 app = rcu_dereference(skb_gso_apps[type]);
5b33bc6e Tom Herbert 2017-09-18 3959 if (app && app->gso_segment &&
5b33bc6e Tom Herbert 2017-09-18 3960 (check_flags & app->check_flags))
5b33bc6e Tom Herbert 2017-09-18 3961 return app;
5b33bc6e Tom Herbert 2017-09-18 3962
5b33bc6e Tom Herbert 2017-09-18 3963 return NULL;
5b33bc6e Tom Herbert 2017-09-18 3964 }
5b33bc6e Tom Herbert 2017-09-18 3965
:::::: The code at line 3958 was first introduced by commit
:::::: 5b33bc6e4fcae1113167c651a3d3a218c7e277c6 net: Add a facility to support application defined GSO
:::::: TO: Tom Herbert <tom@quantonium.net>
:::::: CC: 0day robot <fengguang.wu@intel.com>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
^ permalink raw reply
* Re: [PATCH net-next] net: avoid a full fib lookup when rp_filter is disabled.
From: Eric Dumazet @ 2017-09-19 12:00 UTC (permalink / raw)
To: Paolo Abeni; +Cc: netdev, David S. Miller, Hannes Frederic Sowa
In-Reply-To: <1967ebaa528c626ebd9682e927fd33770a396a0a.1505817863.git.pabeni@redhat.com>
On Tue, 2017-09-19 at 12:46 +0200, Paolo Abeni wrote:
..
> Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> +/* called under RCU lock */
> +struct in_ifaddr *inet_lookup_ifaddr_rcu(struct net *net, __be32 addr)
> +{
> + u32 hash = inet_addr_hash(net, addr);
> + struct in_ifaddr *ifa;
> +
> + hlist_for_each_entry_rcu(ifa, &inet_addr_lst[hash], hash)
> + if (ifa->ifa_local == addr &&
> + net_eq(dev_net(ifa->ifa_dev->dev), net))
> + return ifa;
> +
> + return NULL;
> +}
> +
Any particular reason you do not use this helper to replace the lookup
in __ip_dev_find() ?
In this case, copy paste can be avoided, please avoid a future patch
consolidating the things...
Thanks.
^ permalink raw reply
* Re: [PATCH net-next] net: sk_buff rbnode reorg
From: Eric Dumazet @ 2017-09-19 12:04 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <1505797616.29839.40.camel@edumazet-glaptop3.roam.corp.google.com>
On Mon, 2017-09-18 at 22:06 -0700, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
...
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -797,11 +797,6 @@ struct tcp_skb_cb {
> u16 tcp_gso_segs;
> u16 tcp_gso_size;
> };
> -
> - /* Used to stash the receive timestamp while this skb is in the
> - * out of order queue, as skb->tstamp is overwritten by the
> - * rbnode.
> - */
> ktime_t swtstamp;
> };
I forgot that in upstream kernels, swtstamp can also be removed.
It is used in our kernels to store the time at which the skb was moved
into socket receive queue (after a possible stay in out of order queue)
Will send a V2, removing this field from net-next, before we add it
again later if we upstream our instrumentation code.
^ permalink raw reply
* Re: [PATCH net-next 03/14] gtp: Call common functions to get tunnel routes and add dst_cache
From: Harald Welte @ 2017-09-19 12:09 UTC (permalink / raw)
To: David Miller; +Cc: tom, netdev, pablo, rohit
In-Reply-To: <20170918.211751.1871429944584121281.davem@davemloft.net>
Hi Dave,
On Mon, Sep 18, 2017 at 09:17:51PM -0700, David Miller wrote:
> This and the new dst caching code ignores any source address selection
> done by ip_route_output_key() or the new tunnel route lookup helpers.
>
> Either source address selection should be respected, or if saddr will
> never be modified by a route lookup for some specific reason here,
> that should be documented.
The IP source address is fixed by signaling on the GTP-C control plane
and nothing that the kernel can unilaterally decide to change. Such a
change of address would have to be decided by and first be signaled on
GTP-C to the peer by the userspace daemon, which would then update the
PDP context in the kernel.
So I guess you're asking us to document that rationale as form of a
source code comment ?
--
- Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
^ permalink raw reply
* Re: [PATCH net-next 05/14] gtp: Remove special mtu handling
From: Harald Welte @ 2017-09-19 11:42 UTC (permalink / raw)
To: Tom Herbert; +Cc: davem, netdev, pablo, rohit
In-Reply-To: <20170919003904.5124-6-tom@quantonium.net>
Hi Tom,
On Mon, Sep 18, 2017 at 05:38:55PM -0700, Tom Herbert wrote:
> Removes MTU handling in gtp_build_skb_ip4. This is non standard relative
> to how other tunneling protocols handle MTU. The model espoused is that
> the inner interface should set it's MTU to be less than the expected
> path MTU on the overlay network. Path MTU discovery is not typically
> used for modifying tunnel MTUs.
The point of the kernel GTP module is to interoperate with existing
other GTP implementations and the practises established by cellular
operators when operating GTP in their networks.
While what you describe (chose interface MTU to be less than the
expected path MTU) is generally best practise in the Linux IP/networking
world, this is not generally reflected in the cellular
universe. You see quite a bit of GTP fragmentation due to the fact
that the transport network simply has to deal with the MTU that has
been established via the control plane between SGSN and MS/UE, without
the GGSN even being part of that negotiation.
Also, you may very well have one "gtp0" tunnel device at the GGSN,
but you are establishing individual GTP tunnels to dozesn to hundreds of
different SGSNs at operators all over the world. You cannot reliably
set the "gtp0" interface MTU to "the path MTU of the overlay network",
as the overlay network is in fact different for each of the SGSNs you're
talking to - and each may have a different path MTU.
So unless I'm missing something, I would currently vote for staying with
the current code, which uses the path MTU to the specific destination IP
address (the SGSN).
Regards,
Harald
--
- Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
^ permalink raw reply
* Re: [PATCH net-next 07/14] gtp: Support encapsulation of IPv6 packets
From: Harald Welte @ 2017-09-19 11:53 UTC (permalink / raw)
To: Tom Herbert; +Cc: davem, netdev, pablo, rohit
In-Reply-To: <20170919003904.5124-8-tom@quantonium.net>
On Mon, Sep 18, 2017 at 05:38:57PM -0700, Tom Herbert wrote:
> Allow IPv6 mobile subscriber packets. This entails adding an IPv6 mobile
> subscriber address to pdp context and IPv6 specific variants to find pdp
> contexts by address.
Please note that there are three different PDP contexts for IP:
* IPv4 only (what gtp.c implements so far)
* IPv6 only
* dual IPv4+IPv6 (called IPv46)
This information will have to be provisioned by the control plane
via netlink for each PDP context. The kernel module then needs to
make sure that on a v4-only context no IPv6 packets are accepted
and vice-versa.
Your proposed patch is missing this kind of screening function and
I would imagine it could introduce all kinds of security problems :/
--
- Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
^ permalink raw reply
* Re: [PATCH net-next 04/14] gtp: udp recv clean up
From: Harald Welte @ 2017-09-19 11:32 UTC (permalink / raw)
To: Tom Herbert; +Cc: davem, netdev, pablo, rohit
In-Reply-To: <20170919003904.5124-5-tom@quantonium.net>
Hi Tom,
I think this patch does too many things at once:
* introduce separate rx functions
* convert from netif_rx to gro_cells_receive
* cosmetic changes like "return -1" to "goto drop"
In the context of reviewability and the "one patch per topic", I would
prefer to see those separated, thanks.
--
- Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
^ permalink raw reply
* Re: [PATCH net-next 13/14] gtp: Support for GRO
From: Harald Welte @ 2017-09-19 12:03 UTC (permalink / raw)
To: Tom Herbert; +Cc: davem, netdev, pablo, rohit
In-Reply-To: <20170919003904.5124-14-tom@quantonium.net>
On Mon, Sep 18, 2017 at 05:39:03PM -0700, Tom Herbert wrote:
> Populate GRO receive and GRO complete functions for GTP-Uv0 and v1.
looks fine to me, though I'm not the GRO expert here. Let's say what
the netdev gurus have to say in their review.
If you say it is tested with GRO-capable and non-GRO capable device
drivers, I'm fine with the patch.
--
- Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
^ permalink raw reply
* Re: [PATCH net-next 08/14] gtp: Support encpasulating over IPv6
From: Harald Welte @ 2017-09-19 11:59 UTC (permalink / raw)
To: Tom Herbert; +Cc: davem, netdev, pablo, rohit
In-Reply-To: <20170919003904.5124-9-tom@quantonium.net>
Hi Tom,
On Mon, Sep 18, 2017 at 05:38:58PM -0700, Tom Herbert wrote:
> Allow peers to be specified by IPv6 addresses.
> + u16 peer_af;
> + union {
> + struct in_addr peer_addr_ip4;
> + struct in6_addr peer_addr_ip6;
> + };
this will not really work, as an union means that a PDP context
will be either IPv4-only or IPV6-only, while in reality there
are three types, see my other mail. So you have to deal
with v4-only, v6-only or v4v6.
The v6-only is legacy by now, and all modern phones I've tested in
recent years can do v4v6 rather than having a v4-only and a v6-only
PDP context in parallel.
>From the operator point of view, v4v6 is very desirable, as it basically
halves the amount of PDP contexts compared to the old approach, which
significantly reduces signalling load across your network, as well as
the amount of memory (and thus capacity) in your core network elements.
I've recently implemented v6 + v4v6 support in osmo-ggsn (see
http://git.osmocom.org/osmo-ggsn/) in case you would like to see another
FOSS implementation for v6 + v4v6 - though in userspace, of course.
--
- Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
^ permalink raw reply
* Re: [PATCH net-next 07/14] gtp: Support encapsulation of IPv6 packets
From: Harald Welte @ 2017-09-19 12:12 UTC (permalink / raw)
To: David Miller; +Cc: tom, netdev, pablo, rohit
In-Reply-To: <20170918.211908.2152170523885516973.davem@davemloft.net>
Hi Dave,
On Mon, Sep 18, 2017 at 09:19:08PM -0700, David Miller wrote:
> > +static inline u32 ipv6_hashfn(const struct in6_addr *a)
> > +{
> > + return __ipv6_addr_jhash(a, gtp_h_initval);
> > +}
>
> I know you are just following the pattern of the existing "ipv4_hashfn()" here
> but this kind of stuff is not very global namespace friendly. Even simply
> adding a "gtp_" prefix to these hash functions would be a lot better.
I would agree if this was an inline function defined in a header file or
a non-static function. But where is the global namespace concern in
case of static inline functions defined and used in the same .c file?
If it makes you happy, I'm all for adding the prefix - I just would like
to understand the rationale better, thanks :)
Regards,
Harald
--
- Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
^ permalink raw reply
* [PATCH v2 net-next] net: sk_buff rbnode reorg
From: Eric Dumazet @ 2017-09-19 12:14 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Soheil Hassas Yeganeh, Wei Wang, Willem de Bruijn
In-Reply-To: <1505797616.29839.40.camel@edumazet-glaptop3.roam.corp.google.com>
From: Eric Dumazet <edumazet@google.com>
skb->rbnode shares space with skb->next, skb->prev and skb->tstamp
Current uses (TCP receive ofo queue and netem) need to save/restore
tstamp, while skb->dev is either NULL (TCP) or a constant for a given
queue (netem).
Since we plan using an RB tree for TCP retransmit queue to speedup SACK
processing with large BDP, this patch exchanges skb->dev and
skb->tstamp.
This saves some overhead in both TCP and netem.
v2: removes the swtstamp field from struct tcp_skb_cb
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Soheil Hassas Yeganeh <soheil@google.com>
Cc: Wei Wang <weiwan@google.com>
Cc: Willem de Bruijn <willemb@google.com>
---
include/linux/skbuff.h | 16 ++++++++--------
include/net/tcp.h | 6 ------
net/ipv4/tcp_input.c | 27 +++++----------------------
net/sched/sch_netem.c | 7 ++++---
4 files changed, 17 insertions(+), 39 deletions(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 72299ef00061db1ce70d34b96ae1639ecde08837..492828801acba42ac6bccb287d3cc5080039135c 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -661,8 +661,12 @@ struct sk_buff {
struct sk_buff *prev;
union {
- ktime_t tstamp;
- u64 skb_mstamp;
+ struct net_device *dev;
+ /* Some protocols might use this space to store information,
+ * while device pointer would be NULL.
+ * UDP receive path is one user.
+ */
+ unsigned long dev_scratch;
};
};
struct rb_node rbnode; /* used in netem & tcp stack */
@@ -670,12 +674,8 @@ struct sk_buff {
struct sock *sk;
union {
- struct net_device *dev;
- /* Some protocols might use this space to store information,
- * while device pointer would be NULL.
- * UDP receive path is one user.
- */
- unsigned long dev_scratch;
+ ktime_t tstamp;
+ u64 skb_mstamp;
};
/*
* This is the control buffer. It is free to use for every
diff --git a/include/net/tcp.h b/include/net/tcp.h
index b510f284427aabc1f508d24d29d0f812e5e0aa61..49a8a46466f32bd98b0c38a776fddc40bc621337 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -797,12 +797,6 @@ struct tcp_skb_cb {
u16 tcp_gso_segs;
u16 tcp_gso_size;
};
-
- /* Used to stash the receive timestamp while this skb is in the
- * out of order queue, as skb->tstamp is overwritten by the
- * rbnode.
- */
- ktime_t swtstamp;
};
__u8 tcp_flags; /* TCP header flags. (tcp[13]) */
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index bddf724f5c02abe1d11110cffe2517f6376e440d..db9bb46b5776f9ee332298c0e95afb0a5966b938 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -4266,11 +4266,6 @@ static void tcp_sack_remove(struct tcp_sock *tp)
tp->rx_opt.num_sacks = num_sacks;
}
-enum tcp_queue {
- OOO_QUEUE,
- RCV_QUEUE,
-};
-
/**
* tcp_try_coalesce - try to merge skb to prior one
* @sk: socket
@@ -4286,7 +4281,6 @@ enum tcp_queue {
* Returns true if caller should free @from instead of queueing it
*/
static bool tcp_try_coalesce(struct sock *sk,
- enum tcp_queue dest,
struct sk_buff *to,
struct sk_buff *from,
bool *fragstolen)
@@ -4311,10 +4305,7 @@ static bool tcp_try_coalesce(struct sock *sk,
if (TCP_SKB_CB(from)->has_rxtstamp) {
TCP_SKB_CB(to)->has_rxtstamp = true;
- if (dest == OOO_QUEUE)
- TCP_SKB_CB(to)->swtstamp = TCP_SKB_CB(from)->swtstamp;
- else
- to->tstamp = from->tstamp;
+ to->tstamp = from->tstamp;
}
return true;
@@ -4351,9 +4342,6 @@ static void tcp_ofo_queue(struct sock *sk)
}
p = rb_next(p);
rb_erase(&skb->rbnode, &tp->out_of_order_queue);
- /* Replace tstamp which was stomped by rbnode */
- if (TCP_SKB_CB(skb)->has_rxtstamp)
- skb->tstamp = TCP_SKB_CB(skb)->swtstamp;
if (unlikely(!after(TCP_SKB_CB(skb)->end_seq, tp->rcv_nxt))) {
SOCK_DEBUG(sk, "ofo packet was already received\n");
@@ -4365,8 +4353,7 @@ static void tcp_ofo_queue(struct sock *sk)
TCP_SKB_CB(skb)->end_seq);
tail = skb_peek_tail(&sk->sk_receive_queue);
- eaten = tail && tcp_try_coalesce(sk, RCV_QUEUE,
- tail, skb, &fragstolen);
+ eaten = tail && tcp_try_coalesce(sk, tail, skb, &fragstolen);
tcp_rcv_nxt_update(tp, TCP_SKB_CB(skb)->end_seq);
fin = TCP_SKB_CB(skb)->tcp_flags & TCPHDR_FIN;
if (!eaten)
@@ -4420,10 +4407,6 @@ static void tcp_data_queue_ofo(struct sock *sk, struct sk_buff *skb)
return;
}
- /* Stash tstamp to avoid being stomped on by rbnode */
- if (TCP_SKB_CB(skb)->has_rxtstamp)
- TCP_SKB_CB(skb)->swtstamp = skb->tstamp;
-
/* Disable header prediction. */
tp->pred_flags = 0;
inet_csk_schedule_ack(sk);
@@ -4451,7 +4434,7 @@ static void tcp_data_queue_ofo(struct sock *sk, struct sk_buff *skb)
/* In the typical case, we are adding an skb to the end of the list.
* Use of ooo_last_skb avoids the O(Log(N)) rbtree lookup.
*/
- if (tcp_try_coalesce(sk, OOO_QUEUE, tp->ooo_last_skb,
+ if (tcp_try_coalesce(sk, tp->ooo_last_skb,
skb, &fragstolen)) {
coalesce_done:
tcp_grow_window(sk, skb);
@@ -4502,7 +4485,7 @@ static void tcp_data_queue_ofo(struct sock *sk, struct sk_buff *skb)
__kfree_skb(skb1);
goto merge_right;
}
- } else if (tcp_try_coalesce(sk, OOO_QUEUE, skb1,
+ } else if (tcp_try_coalesce(sk, skb1,
skb, &fragstolen)) {
goto coalesce_done;
}
@@ -4554,7 +4537,7 @@ static int __must_check tcp_queue_rcv(struct sock *sk, struct sk_buff *skb, int
__skb_pull(skb, hdrlen);
eaten = (tail &&
- tcp_try_coalesce(sk, RCV_QUEUE, tail,
+ tcp_try_coalesce(sk, tail,
skb, fragstolen)) ? 1 : 0;
tcp_rcv_nxt_update(tcp_sk(sk), TCP_SKB_CB(skb)->end_seq);
if (!eaten) {
diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index b1266e75ca43cf5a66b951ecabccfc5b24069444..063a4bdb9ee6f26b01387959e8f6ccd15ec16191 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -146,7 +146,6 @@ struct netem_sched_data {
*/
struct netem_skb_cb {
psched_time_t time_to_send;
- ktime_t tstamp_save;
};
@@ -561,7 +560,6 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
}
cb->time_to_send = now + delay;
- cb->tstamp_save = skb->tstamp;
++q->counter;
tfifo_enqueue(skb, sch);
} else {
@@ -629,7 +627,10 @@ static struct sk_buff *netem_dequeue(struct Qdisc *sch)
qdisc_qstats_backlog_dec(sch, skb);
skb->next = NULL;
skb->prev = NULL;
- skb->tstamp = netem_skb_cb(skb)->tstamp_save;
+ /* skb->dev shares skb->rbnode area,
+ * we need to restore its value.
+ */
+ skb->dev = qdisc_dev(sch);
#ifdef CONFIG_NET_CLS_ACT
/*
^ permalink raw reply related
* Re: [PATCH 1/2] mac80211: Add rcu read side critical sections
From: Ville Syrjälä @ 2017-09-19 12:35 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless, David S. Miller, netdev
In-Reply-To: <1505765477.13691.20.camel@sipsolutions.net>
On Mon, Sep 18, 2017 at 10:11:17PM +0200, Johannes Berg wrote:
> > I got the following lockdep warning about the rcu_dereference()s in
> > ieee80211_tx_h_select_key(). After tracing all callers of
> > ieee80211_tx_h_select_key() I discovered that
> > ieee80211_get_buffered_bc()
> > and ieee80211_build_data_template() had the rcu_read_lock/unlock()
> > but
> > three other places did not. So I just blindly added them and made the
> > read side critical section extend as far as the lifetime of 'tx'
> > which
> > is where we seem to be stuffing the rcu protected pointers. No real
> > clue whether this is correct or not.
>
> Heh.
>
> I think we should do it in ieee80211_tx_dequeue(),
Oh, I guess I didn't trace the call chains far enough. ieee80211_tx()
does indeed look OK. But unless I made another mistake in my analysis
ieee80211_tx_prepare_skb() is still busted.
> if not even in the
> driver (and document that it's required)
>
> johannes
>
> > @@ -3411,6 +3430,8 @@ struct sk_buff *ieee80211_tx_dequeue(struct
> > ieee80211_hw *hw,
> > ieee80211_tx_result r;
> > struct ieee80211_vif *vif;
> >
> > + rcu_read_lock();
> > +
> > spin_lock_bh(&fq->lock);
> >
> > if (test_bit(IEEE80211_TXQ_STOP, &txqi->flags))
> > @@ -3513,6 +3534,8 @@ struct sk_buff *ieee80211_tx_dequeue(struct
> > ieee80211_hw *hw,
> > out:
> > spin_unlock_bh(&fq->lock);
> >
> > + rcu_read_unlock();
> >
>
> i.e. this in itself should be sufficient, though you should probably
> reorder and acquire the spinlock first since that might spin, and you
> want to keep the RCU section minimal (it's trivial here, after all)
Good point. I'll respin with that change.
--
Ville Syrjälä
Intel OTC
^ permalink raw reply
* [PATCH net-next] selftests: rtnetlink.sh: add test case for device ifalias
From: Florian Westphal @ 2017-09-19 12:42 UTC (permalink / raw)
To: netdev; +Cc: Florian Westphal
Signed-off-by: Florian Westphal <fw@strlen.de>
---
tools/testing/selftests/net/rtnetlink.sh | 57 ++++++++++++++++++++++++++++++++
1 file changed, 57 insertions(+)
diff --git a/tools/testing/selftests/net/rtnetlink.sh b/tools/testing/selftests/net/rtnetlink.sh
index 57b5ff576240..4b48de565cae 100755
--- a/tools/testing/selftests/net/rtnetlink.sh
+++ b/tools/testing/selftests/net/rtnetlink.sh
@@ -15,6 +15,14 @@ check_err()
fi
}
+# same but inverted -- used when command must fail for test to pass
+check_fail()
+{
+ if [ $1 -eq 0 ]; then
+ ret=1
+ fi
+}
+
kci_add_dummy()
{
ip link add name "$devdummy" type dummy
@@ -235,6 +243,54 @@ kci_test_addrlabel()
echo "PASS: ipv6 addrlabel"
}
+kci_test_ifalias()
+{
+ ret=0
+ namewant=$(uuidgen)
+ syspathname="/sys/class/net/$devdummy/ifalias"
+
+ ip link set dev "$devdummy" alias "$namewant"
+ check_err $?
+
+ if [ $ret -ne 0 ]; then
+ echo "FAIL: cannot set interface alias of $devdummy to $namewant"
+ return 1
+ fi
+
+ ip link show "$devdummy" | grep -q "alias $namewant"
+ check_err $?
+
+ if [ -r "$syspathname" ] ; then
+ read namehave < "$syspathname"
+ if [ "$namewant" != "$namehave" ]; then
+ echo "FAIL: did set ifalias $namewant but got $namehave"
+ return 1
+ fi
+
+ namewant=$(uuidgen)
+ echo "$namewant" > "$syspathname"
+ ip link show "$devdummy" | grep -q "alias $namewant"
+ check_err $?
+
+ # sysfs interface allows to delete alias again
+ echo "" > "$syspathname"
+
+ ip link show "$devdummy" | grep -q "alias $namewant"
+ check_fail $?
+
+ # re-add the alias -- kernel should free mem when dummy dev is removed
+ ip link set dev "$devdummy" alias "$namewant"
+ check_err $?
+ fi
+
+ if [ $ret -ne 0 ]; then
+ echo "FAIL: set interface alias $devdummy to $namewant"
+ return 1
+ fi
+
+ echo "PASS: set ifalias $namewant for $devdummy"
+}
+
kci_test_rtnl()
{
kci_add_dummy
@@ -249,6 +305,7 @@ kci_test_rtnl()
kci_test_gre
kci_test_bridge
kci_test_addrlabel
+ kci_test_ifalias
kci_del_dummy
}
--
2.13.5
^ permalink raw reply related
* Re: [PATCH net-next 00/14] gtp: Additional feature support
From: Harald Welte @ 2017-09-19 12:43 UTC (permalink / raw)
To: Tom Herbert; +Cc: davem, netdev, pablo, rohit
In-Reply-To: <20170919003904.5124-1-tom@quantonium.net>
Hi Tom,
first of all, thanks a lot for your patch series. It makes me happy to
see contributions on the GTP code :)
On Mon, Sep 18, 2017 at 05:38:50PM -0700, Tom Herbert wrote:
> - IPv6 support
see my detailed comments in other mails. It's unfortunately only
support for the already "deprecated" IPv6-only PDP contexts, not the
more modern v4v6 type. In order to interoperate with old and new
approach, all three cases (v4, v6 and v4v6) should be supported from one
code base.
> - Configurable networking interfaces so that GTP kernel can be used
> and tested without needing GSN network emulation (i.e. no user space
> daemon needed).
We have some pretty decent userspace utilities for configuring the GTP
interfaces and tunnels in the libgtpnl repository, but if it helps
people to have another way of configuration, I won't be against it.
What we have to keep in mind is that the current model of 1:1 mapping of
a "UDP socket' to a GTP netdevice is conceptually broken and needs to be
refactored soon (without breaking backwards compatibility). See related
earlier discussions with patches submitted by Andreas Schultz.
Summary:
In real-world GGSNs you often want to host multiple virtual GGSNs on a
single GGSN (= UDP socket). Each virtual GGSN terminates into one
external PDN (packet data network), which can be a private corporate vpn
or any other IP network, with no routing between those networks.
Naively one would assume you "simply" run another virtual GGSN
instance on another IP address, and then differentiate like that.
However, the problem is that adding a new GGSN IP address will require
manual configuration changes at each of your roaming partners (easily
hundreds of operators!) and hence it is avoided at all cost due to the
related long schedule, requirement for interop testing with each of them,
etc.
So what you do in reality at operators is that you operate many of those
virtual GGSNs on the same IP:Port combination (and hence UDP socket),
which means you have PDP contexts for vGGSN A which terminate on e.g.
gtp0 and PDP contexts for vGGSN B on gtp1, and so on. The decision
which gtp-device a given PDP context is a member is made by the GTP-C
instance. In the kenel we'll have to decouple net-devices from sockets.
So whatever new configuration mechanism or architectural changes we
introduce, we need to make sure that those will accomodate the "new
model" rather than introducing further dependencies for which we will
have to maintain backwards compatibility workaronds later on.
> - Port numbers are configurable
I'm not sure if this is a useful feature. GTP is used only in
operator-controlled networks and only on standard ports. It's not
possible to negotiate any non-standard ports on the signaling plane
either.
> - Addition of a dst_cache in the GTP structure and other cleanup
looks fine to me.
> - GSO,GRO
> - Control of zero UDP checksums
[...]
> Additionally, this patch set also includes a couple of general support
> capabilities:
>
> - A facility that allows application specific GSO callbacks
> - Common functions to get a route fo for an IP tunnel
This is where the "core netdev" folks will have to comment. I'm too
remote from mainline kernel development these days and will focus on
reviewing the GTP specific bits of your patch series.
> For IPv6 support, the mobile subscriber needs to allow IPv6 addresses,
> and the remote enpoint can be IPv6.
Minor correction: The mobile subscriber specifically requests a PDP Type
when establishing the PDP context via Session Management related
signaling from MS/UE to SGSN. The SGSN simply translates this to GTP
and then forwards it to the GGSN. So it's acutally not "allow" but
"specifically request".
> Configured the matrix of IPv4/IPv6 mobile subscriber, IPv4/IPv6 remote
> peer, and GTP version 0 and 1 (eight combinations). Observed
> connectivity and proper GSO/GRO. Also, tested VXLAN for
> regression.
I presume those tests were done with manually configured GTP-devices and
PDP contexts to the (patched) kernel GTP module? If so, I would like to
strongly suggest interop testing with a different implementation, such
as real phones on the MS/UE side and e.g. OsmoSGSN. That would,
however, of course mean that the netlink related bits would have to be
added to libgtpnl and OsmoGGSN (or ergw) so that you have a daemon for
the control plane.
For IPv6 (and v4v6) PDP contexts there is quite a bit of extra headache
related to the way how router solicitation/advertisements are modified
in the 3GPP world.
The address allocation in v4 is simple:
* MS/UE requests dynamic or fixed IPv4 address via EUA IE of PDP context
activation
* GGSN responds with IPv4 address in EUA of Activate PDP context
response (and then uses netlink to tell the kernel about that
IPv4 address)
In v6 or the v6 portion of v4v6 it works differently:
* MS/UE requests dynamic or fixed IPv4 address in EUA IE of PDP context
activation
* GGSN responds with an IPv6 address, but that address is *not* used
for communication, but simply used as an "interface identifier" to
build a link-local address.
* MS then uses router solicitation using that link-local address
* GGSN responds with router advertisement, allocating a single /64
prefix, from which the MS then generates a fully-qualified IPv6
source address for communication.
How did you envision this to be done with the v6 support you just added?
At the very least, the /64 prefix matching would have to be implemented
so that in fact all addresses within that /64 prefix are matched +
encapsulated for a given PDP context in the downlink (to phone)
direction.
Also, I think the responsibility for the router advertisements would be
in the kernel, too. Otherwise, a GTP-C userspace implementation would
have to inject packets into the user plane (which is otherwise handled
completely inside the kernel). Injecting packets would mean that in caes
GTP sequence numbers are used, that userspace implementation would have
to alter the sequence numbers of the kernel gtp.ko code using netlink,
but therre would be race conditions, ...
The router advertisements and neighbor advertisements basically have the
semantics of one link per PDP context. Each of them is a point-to-point
link, and it's not one router advertisement that's sent to all of the
PDP contexts on that gtp-device.
I know it all sucks. I'm still happy to see somebody tackling v6
support in gtp.c :)
Regards,
Harald
--
- Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
^ 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