* Re: [net-next RFC 0/4] SO_BINDTOSUBNET
From: Gilberto @ 2016-03-25 22:29 UTC (permalink / raw)
To: Tom Herbert; +Cc: Linux Kernel Network Developers
In-Reply-To: <CALx6S35+Lv85SQvqMOe4anj6z=qXB+5dUjhEw1x=jQRQGLjD2w@mail.gmail.com>
On 03/25/2016 12:25 AM, Tom Herbert wrote:
> On Wed, Mar 16, 2016 at 6:19 AM, Gilberto Bertin
> <gilberto.bertin@gmail.com> wrote:
>> This is my second attempt to submit an RFC for this patch.
>>
>> Some arguments for and against it since the first submission:
>> * SO_BINDTOSUBNET is an arbitrary option and can be seens as nother use
>> * case of the SO_REUSEPORT BPF patch
>> * but at the same time using BPF requires more work/code on the server
>> and since the bind to subnet use case could potentially become a
>> common one maybe there is some value in having it as an option instead
>> of having to code (either manually or with clang) an eBPF program that
>> would do the same
>
> Gilberto, I'm not sure I understand this argument. Have you
> implemented the BPF bind solution?
>
> Thanks,
> Tom
Yes, I wrote up a very basic draft for this feature (I didn't know there
was already some work going on with SO_ATTACH_REUSEPORT_[CE]BPF).
Thanks,
Gilberto
^ permalink raw reply
* [RFC net-next 0/2] udp: use standard RCU rules
From: Eric Dumazet @ 2016-03-25 22:29 UTC (permalink / raw)
To: David S . Miller; +Cc: netdev, Eric Dumazet, Eric Dumazet, Tom Herbert
Add a generic facility for sockets to be freed afer an RCU grace period.
Then UDP is changed to no longer use SLAB_DESTROY_BY_RCU,
in order to speedup rx processing for traffic encapsulated in UDP.
I prepared a patch to convert TCP listeners to this infrastructure,
but will post it later, since Tom was mostly interested in UDP.
Eric Dumazet (2):
net: add SOCK_RCU_FREE socket flag
udp: No longer use SLAB_DESTROY_BY_RCU
include/linux/udp.h | 8 +-
include/net/sock.h | 14 +--
include/net/udp.h | 2 +-
net/core/sock.c | 14 ++-
net/ipv4/udp.c | 290 +++++++++++++++-------------------------------------
net/ipv4/udp_diag.c | 18 ++--
net/ipv6/udp.c | 194 +++++++++++------------------------
7 files changed, 177 insertions(+), 363 deletions(-)
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply
* [RFC net-next 1/2] net: add SOCK_RCU_FREE socket flag
From: Eric Dumazet @ 2016-03-25 22:29 UTC (permalink / raw)
To: David S . Miller; +Cc: netdev, Eric Dumazet, Eric Dumazet, Tom Herbert
In-Reply-To: <1458944964-12890-1-git-send-email-edumazet@google.com>
We want a generic way to insert an RCU grace period before socket
freeing for cases where RCU_SLAB_DESTROY_BY_RCU is adding too
much overhead.
SLAB_DESTROY_BY_RCU strict rules force us to take a reference
on the socket sk_refcnt, and it is a performance problem for UDP
encapsulation, or TCP synflood behavior, as many CPUs might
attempt the atomic operations on a shared sk_refcnt
UDP sockets and TCP listeners can set SOCK_RCU_FREE so that their
lookup can use traditional RCU rules, without refcount changes.
They can set the flag only once hashed and visible by other cpus.
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Tom Herbert <tom@herbertland.com>
---
include/net/sock.h | 2 ++
net/core/sock.c | 14 +++++++++++++-
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/include/net/sock.h b/include/net/sock.h
index 255d3e03727b..c88785a3e76c 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -438,6 +438,7 @@ struct sock {
struct sk_buff *skb);
void (*sk_destruct)(struct sock *sk);
struct sock_reuseport __rcu *sk_reuseport_cb;
+ struct rcu_head sk_rcu;
};
#define __sk_user_data(sk) ((*((void __rcu **)&(sk)->sk_user_data)))
@@ -720,6 +721,7 @@ enum sock_flags {
*/
SOCK_FILTER_LOCKED, /* Filter cannot be changed anymore */
SOCK_SELECT_ERR_QUEUE, /* Wake select on error queue */
+ SOCK_RCU_FREE, /* wait rcu grace period in sk_destruct() */
};
#define SK_FLAGS_TIMESTAMP ((1UL << SOCK_TIMESTAMP) | (1UL << SOCK_TIMESTAMPING_RX_SOFTWARE))
diff --git a/net/core/sock.c b/net/core/sock.c
index b67b9aedb230..238a94f879ca 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1418,8 +1418,12 @@ struct sock *sk_alloc(struct net *net, int family, gfp_t priority,
}
EXPORT_SYMBOL(sk_alloc);
-void sk_destruct(struct sock *sk)
+/* Sockets having SOCK_RCU_FREE will call this function after one RCU
+ * grace period. This is the case for UDP sockets and TCP listeners.
+ */
+static void __sk_destruct(struct rcu_head *head)
{
+ struct sock *sk = container_of(head, struct sock, sk_rcu);
struct sk_filter *filter;
if (sk->sk_destruct)
@@ -1448,6 +1452,14 @@ void sk_destruct(struct sock *sk)
sk_prot_free(sk->sk_prot_creator, sk);
}
+void sk_destruct(struct sock *sk)
+{
+ if (sock_flag(sk, SOCK_RCU_FREE))
+ call_rcu(&sk->sk_rcu, __sk_destruct);
+ else
+ __sk_destruct(&sk->sk_rcu);
+}
+
static void __sk_free(struct sock *sk)
{
if (unlikely(sock_diag_has_destroy_listeners(sk) && sk->sk_net_refcnt))
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [RFC net-next 2/2] udp: No longer use SLAB_DESTROY_BY_RCU
From: Eric Dumazet @ 2016-03-25 22:29 UTC (permalink / raw)
To: David S . Miller; +Cc: netdev, Eric Dumazet, Eric Dumazet, Tom Herbert
In-Reply-To: <1458944964-12890-1-git-send-email-edumazet@google.com>
Tom Herbert would like to avoid touching UDP socket refcnt for encapsulated
traffic. For this to happen, we need to use normal RCU rules, with a grace
period before freeing a socket. UDP sockets are not short lived in the
high usage case, so the added cost of call_rcu() should not be a concern.
This actually removes a lot of complexity in UDP stack
Multicast receives no longer need to hold a bucket lock.
Note that ip early demux still needs to take a reference on the socket.
Same remark for functions used by xt_socket and xt_PROXY netfilter modules,
but this might be changed later.
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Tom Herbert <tom@herbertland.com>
---
include/linux/udp.h | 8 +-
include/net/sock.h | 12 +--
include/net/udp.h | 2 +-
net/ipv4/udp.c | 290 +++++++++++++++-------------------------------------
net/ipv4/udp_diag.c | 18 ++--
net/ipv6/udp.c | 194 +++++++++++------------------------
6 files changed, 162 insertions(+), 362 deletions(-)
diff --git a/include/linux/udp.h b/include/linux/udp.h
index 87c094961bd5..32342754643a 100644
--- a/include/linux/udp.h
+++ b/include/linux/udp.h
@@ -98,11 +98,11 @@ static inline bool udp_get_no_check6_rx(struct sock *sk)
return udp_sk(sk)->no_check6_rx;
}
-#define udp_portaddr_for_each_entry(__sk, node, list) \
- hlist_nulls_for_each_entry(__sk, node, list, __sk_common.skc_portaddr_node)
+#define udp_portaddr_for_each_entry(__sk, list) \
+ hlist_for_each_entry(__sk, list, __sk_common.skc_portaddr_node)
-#define udp_portaddr_for_each_entry_rcu(__sk, node, list) \
- hlist_nulls_for_each_entry_rcu(__sk, node, list, __sk_common.skc_portaddr_node)
+#define udp_portaddr_for_each_entry_rcu(__sk, list) \
+ hlist_for_each_entry_rcu(__sk, list, __sk_common.skc_portaddr_node)
#define IS_UDPLITE(__sk) (udp_sk(__sk)->pcflag)
diff --git a/include/net/sock.h b/include/net/sock.h
index c88785a3e76c..5b9562bc478e 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -178,7 +178,7 @@ struct sock_common {
int skc_bound_dev_if;
union {
struct hlist_node skc_bind_node;
- struct hlist_nulls_node skc_portaddr_node;
+ struct hlist_node skc_portaddr_node;
};
struct proto *skc_prot;
possible_net_t skc_net;
@@ -670,18 +670,18 @@ static inline void sk_add_bind_node(struct sock *sk,
hlist_for_each_entry(__sk, list, sk_bind_node)
/**
- * sk_nulls_for_each_entry_offset - iterate over a list at a given struct offset
+ * sk_for_each_entry_offset - iterate over a list at a given struct offset
* @tpos: the type * to use as a loop cursor.
* @pos: the &struct hlist_node to use as a loop cursor.
* @head: the head for your list.
* @offset: offset of hlist_node within the struct.
*
*/
-#define sk_nulls_for_each_entry_offset(tpos, pos, head, offset) \
- for (pos = (head)->first; \
- (!is_a_nulls(pos)) && \
+#define sk_for_each_entry_offset_rcu(tpos, pos, head, offset) \
+ for (pos = rcu_dereference((head)->first); \
+ pos != NULL && \
({ tpos = (typeof(*tpos) *)((void *)pos - offset); 1;}); \
- pos = pos->next)
+ pos = rcu_dereference(pos->next))
static inline struct user_namespace *sk_user_ns(struct sock *sk)
{
diff --git a/include/net/udp.h b/include/net/udp.h
index 92927f729ac8..d870ec1611c4 100644
--- a/include/net/udp.h
+++ b/include/net/udp.h
@@ -59,7 +59,7 @@ struct udp_skb_cb {
* @lock: spinlock protecting changes to head/count
*/
struct udp_hslot {
- struct hlist_nulls_head head;
+ struct hlist_head head;
int count;
spinlock_t lock;
} __attribute__((aligned(2 * sizeof(long))));
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 08eed5e16df0..3ebca8445d35 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -143,10 +143,9 @@ static int udp_lib_lport_inuse(struct net *net, __u16 num,
unsigned int log)
{
struct sock *sk2;
- struct hlist_nulls_node *node;
kuid_t uid = sock_i_uid(sk);
- sk_nulls_for_each(sk2, node, &hslot->head) {
+ sk_for_each(sk2, &hslot->head) {
if (net_eq(sock_net(sk2), net) &&
sk2 != sk &&
(bitmap || udp_sk(sk2)->udp_port_hash == num) &&
@@ -177,12 +176,11 @@ static int udp_lib_lport_inuse2(struct net *net, __u16 num,
bool match_wildcard))
{
struct sock *sk2;
- struct hlist_nulls_node *node;
kuid_t uid = sock_i_uid(sk);
int res = 0;
spin_lock(&hslot2->lock);
- udp_portaddr_for_each_entry(sk2, node, &hslot2->head) {
+ udp_portaddr_for_each_entry(sk2, &hslot2->head) {
if (net_eq(sock_net(sk2), net) &&
sk2 != sk &&
(udp_sk(sk2)->udp_port_hash == num) &&
@@ -207,11 +205,10 @@ static int udp_reuseport_add_sock(struct sock *sk, struct udp_hslot *hslot,
bool match_wildcard))
{
struct net *net = sock_net(sk);
- struct hlist_nulls_node *node;
kuid_t uid = sock_i_uid(sk);
struct sock *sk2;
- sk_nulls_for_each(sk2, node, &hslot->head) {
+ sk_for_each(sk2, &hslot->head) {
if (net_eq(sock_net(sk2), net) &&
sk2 != sk &&
sk2->sk_family == sk->sk_family &&
@@ -333,17 +330,18 @@ found:
goto fail_unlock;
}
- sk_nulls_add_node_rcu(sk, &hslot->head);
+ sk_add_node_rcu(sk, &hslot->head);
hslot->count++;
sock_prot_inuse_add(sock_net(sk), sk->sk_prot, 1);
hslot2 = udp_hashslot2(udptable, udp_sk(sk)->udp_portaddr_hash);
spin_lock(&hslot2->lock);
- hlist_nulls_add_head_rcu(&udp_sk(sk)->udp_portaddr_node,
+ hlist_add_head_rcu(&udp_sk(sk)->udp_portaddr_node,
&hslot2->head);
hslot2->count++;
spin_unlock(&hslot2->lock);
}
+ sock_set_flag(sk, SOCK_RCU_FREE);
error = 0;
fail_unlock:
spin_unlock_bh(&hslot->lock);
@@ -497,37 +495,27 @@ static struct sock *udp4_lib_lookup2(struct net *net,
struct sk_buff *skb)
{
struct sock *sk, *result;
- struct hlist_nulls_node *node;
int score, badness, matches = 0, reuseport = 0;
- bool select_ok = true;
u32 hash = 0;
-begin:
result = NULL;
badness = 0;
- udp_portaddr_for_each_entry_rcu(sk, node, &hslot2->head) {
+ udp_portaddr_for_each_entry_rcu(sk, &hslot2->head) {
score = compute_score2(sk, net, saddr, sport,
daddr, hnum, dif);
if (score > badness) {
- result = sk;
- badness = score;
reuseport = sk->sk_reuseport;
if (reuseport) {
hash = udp_ehashfn(net, daddr, hnum,
saddr, sport);
- if (select_ok) {
- struct sock *sk2;
-
- sk2 = reuseport_select_sock(sk, hash, skb,
+ result = reuseport_select_sock(sk, hash, skb,
sizeof(struct udphdr));
- if (sk2) {
- result = sk2;
- select_ok = false;
- goto found;
- }
- }
+ if (result)
+ return result;
matches = 1;
}
+ badness = score;
+ result = sk;
} else if (score == badness && reuseport) {
matches++;
if (reciprocal_scale(hash, matches) == 0)
@@ -535,23 +523,6 @@ begin:
hash = next_pseudo_random32(hash);
}
}
- /*
- * if the nulls value we got at the end of this lookup is
- * not the expected one, we must restart lookup.
- * We probably met an item that was moved to another chain.
- */
- if (get_nulls_value(node) != slot2)
- goto begin;
- if (result) {
-found:
- if (unlikely(!atomic_inc_not_zero_hint(&result->sk_refcnt, 2)))
- result = NULL;
- else if (unlikely(compute_score2(result, net, saddr, sport,
- daddr, hnum, dif) < badness)) {
- sock_put(result);
- goto begin;
- }
- }
return result;
}
@@ -563,15 +534,12 @@ struct sock *__udp4_lib_lookup(struct net *net, __be32 saddr,
int dif, struct udp_table *udptable, struct sk_buff *skb)
{
struct sock *sk, *result;
- struct hlist_nulls_node *node;
unsigned short hnum = ntohs(dport);
unsigned int hash2, slot2, slot = udp_hashfn(net, hnum, udptable->mask);
struct udp_hslot *hslot2, *hslot = &udptable->hash[slot];
int score, badness, matches = 0, reuseport = 0;
- bool select_ok = true;
u32 hash = 0;
- rcu_read_lock();
if (hslot->count > 10) {
hash2 = udp4_portaddr_hash(net, daddr, hnum);
slot2 = hash2 & udptable->mask;
@@ -593,35 +561,27 @@ struct sock *__udp4_lib_lookup(struct net *net, __be32 saddr,
htonl(INADDR_ANY), hnum, dif,
hslot2, slot2, skb);
}
- rcu_read_unlock();
return result;
}
begin:
result = NULL;
badness = 0;
- sk_nulls_for_each_rcu(sk, node, &hslot->head) {
+ sk_for_each_rcu(sk, &hslot->head) {
score = compute_score(sk, net, saddr, hnum, sport,
daddr, dport, dif);
if (score > badness) {
- result = sk;
- badness = score;
reuseport = sk->sk_reuseport;
if (reuseport) {
hash = udp_ehashfn(net, daddr, hnum,
saddr, sport);
- if (select_ok) {
- struct sock *sk2;
-
- sk2 = reuseport_select_sock(sk, hash, skb,
+ result = reuseport_select_sock(sk, hash, skb,
sizeof(struct udphdr));
- if (sk2) {
- result = sk2;
- select_ok = false;
- goto found;
- }
- }
+ if (result)
+ return result;
matches = 1;
}
+ result = sk;
+ badness = score;
} else if (score == badness && reuseport) {
matches++;
if (reciprocal_scale(hash, matches) == 0)
@@ -629,25 +589,6 @@ begin:
hash = next_pseudo_random32(hash);
}
}
- /*
- * if the nulls value we got at the end of this lookup is
- * not the expected one, we must restart lookup.
- * We probably met an item that was moved to another chain.
- */
- if (get_nulls_value(node) != slot)
- goto begin;
-
- if (result) {
-found:
- if (unlikely(!atomic_inc_not_zero_hint(&result->sk_refcnt, 2)))
- result = NULL;
- else if (unlikely(compute_score(result, net, saddr, hnum, sport,
- daddr, dport, dif) < badness)) {
- sock_put(result);
- goto begin;
- }
- }
- rcu_read_unlock();
return result;
}
EXPORT_SYMBOL_GPL(__udp4_lib_lookup);
@@ -663,13 +604,24 @@ static inline struct sock *__udp4_lib_lookup_skb(struct sk_buff *skb,
udptable, skb);
}
+/* Must be called under rcu_read_lock().
+ * Does increment socket refcount.
+ */
+#if IS_ENABLED(CONFIG_NETFILTER_XT_MATCH_SOCKET) || \
+ IS_ENABLED(CONFIG_NETFILTER_XT_TARGET_TPROXY)
struct sock *udp4_lib_lookup(struct net *net, __be32 saddr, __be16 sport,
__be32 daddr, __be16 dport, int dif)
{
- return __udp4_lib_lookup(net, saddr, sport, daddr, dport, dif,
- &udp_table, NULL);
+ struct sock *sk;
+
+ sk = __udp4_lib_lookup(net, saddr, sport, daddr, dport,
+ dif, &udp_table, NULL);
+ if (sk && !atomic_inc_not_zero(&sk->sk_refcnt))
+ sk = NULL;
+ return sk;
}
EXPORT_SYMBOL_GPL(udp4_lib_lookup);
+#endif
static inline bool __udp_is_mcast_sock(struct net *net, struct sock *sk,
__be16 loc_port, __be32 loc_addr,
@@ -771,7 +723,7 @@ void __udp4_lib_err(struct sk_buff *skb, u32 info, struct udp_table *udptable)
sk->sk_err = err;
sk->sk_error_report(sk);
out:
- sock_put(sk);
+ return;
}
void udp_err(struct sk_buff *skb, u32 info)
@@ -1474,13 +1426,13 @@ void udp_lib_unhash(struct sock *sk)
spin_lock_bh(&hslot->lock);
if (rcu_access_pointer(sk->sk_reuseport_cb))
reuseport_detach_sock(sk);
- if (sk_nulls_del_node_init_rcu(sk)) {
+ if (sk_del_node_init_rcu(sk)) {
hslot->count--;
inet_sk(sk)->inet_num = 0;
sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
spin_lock(&hslot2->lock);
- hlist_nulls_del_init_rcu(&udp_sk(sk)->udp_portaddr_node);
+ hlist_del_init_rcu(&udp_sk(sk)->udp_portaddr_node);
hslot2->count--;
spin_unlock(&hslot2->lock);
}
@@ -1513,12 +1465,12 @@ void udp_lib_rehash(struct sock *sk, u16 newhash)
if (hslot2 != nhslot2) {
spin_lock(&hslot2->lock);
- hlist_nulls_del_init_rcu(&udp_sk(sk)->udp_portaddr_node);
+ hlist_del_init_rcu(&udp_sk(sk)->udp_portaddr_node);
hslot2->count--;
spin_unlock(&hslot2->lock);
spin_lock(&nhslot2->lock);
- hlist_nulls_add_head_rcu(&udp_sk(sk)->udp_portaddr_node,
+ hlist_add_head_rcu(&udp_sk(sk)->udp_portaddr_node,
&nhslot2->head);
nhslot2->count++;
spin_unlock(&nhslot2->lock);
@@ -1697,35 +1649,6 @@ drop:
return -1;
}
-static void flush_stack(struct sock **stack, unsigned int count,
- struct sk_buff *skb, unsigned int final)
-{
- unsigned int i;
- struct sk_buff *skb1 = NULL;
- struct sock *sk;
-
- for (i = 0; i < count; i++) {
- sk = stack[i];
- if (likely(!skb1))
- skb1 = (i == final) ? skb : skb_clone(skb, GFP_ATOMIC);
-
- if (!skb1) {
- atomic_inc(&sk->sk_drops);
- UDP_INC_STATS_BH(sock_net(sk), UDP_MIB_RCVBUFERRORS,
- IS_UDPLITE(sk));
- UDP_INC_STATS_BH(sock_net(sk), UDP_MIB_INERRORS,
- IS_UDPLITE(sk));
- }
-
- if (skb1 && udp_queue_rcv_skb(sk, skb1) <= 0)
- skb1 = NULL;
-
- sock_put(sk);
- }
- if (unlikely(skb1))
- kfree_skb(skb1);
-}
-
/* For TCP sockets, sk_rx_dst is protected by socket lock
* For UDP, we use xchg() to guard against concurrent changes.
*/
@@ -1749,14 +1672,14 @@ static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
struct udp_table *udptable,
int proto)
{
- struct sock *sk, *stack[256 / sizeof(struct sock *)];
- struct hlist_nulls_node *node;
+ struct sock *sk, *first = NULL;
unsigned short hnum = ntohs(uh->dest);
struct udp_hslot *hslot = udp_hashslot(udptable, net, hnum);
- int dif = skb->dev->ifindex;
- unsigned int count = 0, offset = offsetof(typeof(*sk), sk_nulls_node);
unsigned int hash2 = 0, hash2_any = 0, use_hash2 = (hslot->count > 10);
- bool inner_flushed = false;
+ unsigned int offset = offsetof(typeof(*sk), sk_node);
+ int dif = skb->dev->ifindex;
+ struct hlist_node *node;
+ struct sk_buff *nskb;
if (use_hash2) {
hash2_any = udp4_portaddr_hash(net, htonl(INADDR_ANY), hnum) &
@@ -1767,23 +1690,27 @@ start_lookup:
offset = offsetof(typeof(*sk), __sk_common.skc_portaddr_node);
}
- spin_lock(&hslot->lock);
- sk_nulls_for_each_entry_offset(sk, node, &hslot->head, offset) {
- if (__udp_is_mcast_sock(net, sk,
- uh->dest, daddr,
- uh->source, saddr,
- dif, hnum)) {
- if (unlikely(count == ARRAY_SIZE(stack))) {
- flush_stack(stack, count, skb, ~0);
- inner_flushed = true;
- count = 0;
- }
- stack[count++] = sk;
- sock_hold(sk);
+ sk_for_each_entry_offset_rcu(sk, node, &hslot->head, offset) {
+ if (!__udp_is_mcast_sock(net, sk, uh->dest, daddr,
+ uh->source, saddr, dif, hnum))
+ continue;
+
+ if (!first) {
+ first = sk;
+ continue;
}
- }
+ nskb = skb_clone(skb, GFP_ATOMIC);
- spin_unlock(&hslot->lock);
+ if (unlikely(!nskb)) {
+ atomic_inc(&sk->sk_drops);
+ UDP_INC_STATS_BH(sock_net(sk), UDP_MIB_RCVBUFERRORS,
+ IS_UDPLITE(sk));
+ UDP_INC_STATS_BH(sock_net(sk), UDP_MIB_INERRORS,
+ IS_UDPLITE(sk));
+ continue;
+ }
+ udp_queue_rcv_skb(sk, nskb);
+ }
/* Also lookup *:port if we are using hash2 and haven't done so yet. */
if (use_hash2 && hash2 != hash2_any) {
@@ -1791,17 +1718,10 @@ start_lookup:
goto start_lookup;
}
- /*
- * do the slow work with no lock held
- */
- if (count) {
- flush_stack(stack, count, skb, count - 1);
- } else {
- if (!inner_flushed)
- UDP_INC_STATS_BH(net, UDP_MIB_IGNOREDMULTI,
- proto == IPPROTO_UDPLITE);
- consume_skb(skb);
- }
+ if (first)
+ udp_queue_rcv_skb(first, skb);
+ else
+ kfree_skb(skb);
return 0;
}
@@ -1897,7 +1817,6 @@ int __udp4_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
inet_compute_pseudo);
ret = udp_queue_rcv_skb(sk, skb);
- sock_put(sk);
/* a return value > 0 means to resubmit the input, but
* it wants the return to be -protocol, or 0
@@ -1958,49 +1877,24 @@ static struct sock *__udp4_lib_mcast_demux_lookup(struct net *net,
int dif)
{
struct sock *sk, *result;
- struct hlist_nulls_node *node;
unsigned short hnum = ntohs(loc_port);
- unsigned int count, slot = udp_hashfn(net, hnum, udp_table.mask);
+ unsigned int slot = udp_hashfn(net, hnum, udp_table.mask);
struct udp_hslot *hslot = &udp_table.hash[slot];
/* Do not bother scanning a too big list */
if (hslot->count > 10)
return NULL;
- rcu_read_lock();
-begin:
- count = 0;
result = NULL;
- sk_nulls_for_each_rcu(sk, node, &hslot->head) {
- if (__udp_is_mcast_sock(net, sk,
- loc_port, loc_addr,
- rmt_port, rmt_addr,
- dif, hnum)) {
+ sk_for_each_rcu(sk, &hslot->head) {
+ if (__udp_is_mcast_sock(net, sk, loc_port, loc_addr,
+ rmt_port, rmt_addr, dif, hnum)) {
+ if (result)
+ return NULL;
result = sk;
- ++count;
}
}
- /*
- * if the nulls value we got at the end of this lookup is
- * not the expected one, we must restart lookup.
- * We probably met an item that was moved to another chain.
- */
- if (get_nulls_value(node) != slot)
- goto begin;
-
- if (result) {
- if (count != 1 ||
- unlikely(!atomic_inc_not_zero_hint(&result->sk_refcnt, 2)))
- result = NULL;
- else if (unlikely(!__udp_is_mcast_sock(net, result,
- loc_port, loc_addr,
- rmt_port, rmt_addr,
- dif, hnum))) {
- sock_put(result);
- result = NULL;
- }
- }
- rcu_read_unlock();
+
return result;
}
@@ -2013,37 +1907,20 @@ static struct sock *__udp4_lib_demux_lookup(struct net *net,
__be16 rmt_port, __be32 rmt_addr,
int dif)
{
- struct sock *sk, *result;
- struct hlist_nulls_node *node;
unsigned short hnum = ntohs(loc_port);
unsigned int hash2 = udp4_portaddr_hash(net, loc_addr, hnum);
unsigned int slot2 = hash2 & udp_table.mask;
struct udp_hslot *hslot2 = &udp_table.hash2[slot2];
INET_ADDR_COOKIE(acookie, rmt_addr, loc_addr);
const __portpair ports = INET_COMBINED_PORTS(rmt_port, hnum);
+ struct sock *sk;
- rcu_read_lock();
- result = NULL;
- udp_portaddr_for_each_entry_rcu(sk, node, &hslot2->head) {
+ udp_portaddr_for_each_entry_rcu(sk, &hslot2->head) {
if (INET_MATCH(sk, net, acookie,
rmt_addr, loc_addr, ports, dif))
- result = sk;
- /* Only check first socket in chain */
- break;
- }
-
- if (result) {
- if (unlikely(!atomic_inc_not_zero_hint(&result->sk_refcnt, 2)))
- result = NULL;
- else if (unlikely(!INET_MATCH(sk, net, acookie,
- rmt_addr, loc_addr,
- ports, dif))) {
- sock_put(result);
- result = NULL;
- }
+ return sk;
}
- rcu_read_unlock();
- return result;
+ return NULL;
}
void udp_v4_early_demux(struct sk_buff *skb)
@@ -2051,7 +1928,7 @@ void udp_v4_early_demux(struct sk_buff *skb)
struct net *net = dev_net(skb->dev);
const struct iphdr *iph;
const struct udphdr *uh;
- struct sock *sk;
+ struct sock *sk = NULL;
struct dst_entry *dst;
int dif = skb->dev->ifindex;
int ours;
@@ -2083,11 +1960,9 @@ void udp_v4_early_demux(struct sk_buff *skb)
} else if (skb->pkt_type == PACKET_HOST) {
sk = __udp4_lib_demux_lookup(net, uh->dest, iph->daddr,
uh->source, iph->saddr, dif);
- } else {
- return;
}
- if (!sk)
+ if (!sk || !atomic_inc_not_zero_hint(&sk->sk_refcnt, 2))
return;
skb->sk = sk;
@@ -2387,14 +2262,13 @@ static struct sock *udp_get_first(struct seq_file *seq, int start)
for (state->bucket = start; state->bucket <= state->udp_table->mask;
++state->bucket) {
- struct hlist_nulls_node *node;
struct udp_hslot *hslot = &state->udp_table->hash[state->bucket];
- if (hlist_nulls_empty(&hslot->head))
+ if (hlist_empty(&hslot->head))
continue;
spin_lock_bh(&hslot->lock);
- sk_nulls_for_each(sk, node, &hslot->head) {
+ sk_for_each(sk, &hslot->head) {
if (!net_eq(sock_net(sk), net))
continue;
if (sk->sk_family == state->family)
@@ -2413,7 +2287,7 @@ static struct sock *udp_get_next(struct seq_file *seq, struct sock *sk)
struct net *net = seq_file_net(seq);
do {
- sk = sk_nulls_next(sk);
+ sk = sk_next(sk);
} while (sk && (!net_eq(sock_net(sk), net) || sk->sk_family != state->family));
if (!sk) {
@@ -2622,12 +2496,12 @@ void __init udp_table_init(struct udp_table *table, const char *name)
table->hash2 = table->hash + (table->mask + 1);
for (i = 0; i <= table->mask; i++) {
- INIT_HLIST_NULLS_HEAD(&table->hash[i].head, i);
+ INIT_HLIST_HEAD(&table->hash[i].head);
table->hash[i].count = 0;
spin_lock_init(&table->hash[i].lock);
}
for (i = 0; i <= table->mask; i++) {
- INIT_HLIST_NULLS_HEAD(&table->hash2[i].head, i);
+ INIT_HLIST_HEAD(&table->hash2[i].head);
table->hash2[i].count = 0;
spin_lock_init(&table->hash2[i].lock);
}
diff --git a/net/ipv4/udp_diag.c b/net/ipv4/udp_diag.c
index df1966f3b6ec..3d5ccf4b1412 100644
--- a/net/ipv4/udp_diag.c
+++ b/net/ipv4/udp_diag.c
@@ -36,10 +36,11 @@ static int udp_dump_one(struct udp_table *tbl, struct sk_buff *in_skb,
const struct inet_diag_req_v2 *req)
{
int err = -EINVAL;
- struct sock *sk;
+ struct sock *sk = NULL;
struct sk_buff *rep;
struct net *net = sock_net(in_skb->sk);
+ rcu_read_lock();
if (req->sdiag_family == AF_INET)
sk = __udp4_lib_lookup(net,
req->id.idiag_src[0], req->id.idiag_sport,
@@ -54,9 +55,9 @@ static int udp_dump_one(struct udp_table *tbl, struct sk_buff *in_skb,
req->id.idiag_dport,
req->id.idiag_if, tbl, NULL);
#endif
- else
- goto out_nosk;
-
+ if (sk && !atomic_inc_not_zero(&sk->sk_refcnt))
+ sk = NULL;
+ rcu_read_unlock();
err = -ENOENT;
if (!sk)
goto out_nosk;
@@ -96,24 +97,23 @@ static void udp_dump(struct udp_table *table, struct sk_buff *skb,
struct netlink_callback *cb,
const struct inet_diag_req_v2 *r, struct nlattr *bc)
{
- int num, s_num, slot, s_slot;
struct net *net = sock_net(skb->sk);
+ int num, s_num, slot, s_slot;
s_slot = cb->args[0];
num = s_num = cb->args[1];
for (slot = s_slot; slot <= table->mask; s_num = 0, slot++) {
- struct sock *sk;
- struct hlist_nulls_node *node;
struct udp_hslot *hslot = &table->hash[slot];
+ struct sock *sk;
num = 0;
- if (hlist_nulls_empty(&hslot->head))
+ if (hlist_empty(&hslot->head))
continue;
spin_lock_bh(&hslot->lock);
- sk_nulls_for_each(sk, node, &hslot->head) {
+ sk_for_each(sk, &hslot->head) {
struct inet_sock *inet = inet_sk(sk);
if (!net_eq(sock_net(sk), net))
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index fd25e447a5fa..eca04b25879b 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -213,37 +213,28 @@ static struct sock *udp6_lib_lookup2(struct net *net,
struct sk_buff *skb)
{
struct sock *sk, *result;
- struct hlist_nulls_node *node;
int score, badness, matches = 0, reuseport = 0;
- bool select_ok = true;
u32 hash = 0;
-begin:
result = NULL;
badness = -1;
- udp_portaddr_for_each_entry_rcu(sk, node, &hslot2->head) {
+ udp_portaddr_for_each_entry_rcu(sk, &hslot2->head) {
score = compute_score2(sk, net, saddr, sport,
daddr, hnum, dif);
if (score > badness) {
- result = sk;
- badness = score;
reuseport = sk->sk_reuseport;
if (reuseport) {
hash = udp6_ehashfn(net, daddr, hnum,
saddr, sport);
- if (select_ok) {
- struct sock *sk2;
- sk2 = reuseport_select_sock(sk, hash, skb,
+ result = reuseport_select_sock(sk, hash, skb,
sizeof(struct udphdr));
- if (sk2) {
- result = sk2;
- select_ok = false;
- goto found;
- }
- }
+ if (result)
+ return result;
matches = 1;
}
+ result = sk;
+ badness = score;
} else if (score == badness && reuseport) {
matches++;
if (reciprocal_scale(hash, matches) == 0)
@@ -251,27 +242,10 @@ begin:
hash = next_pseudo_random32(hash);
}
}
- /*
- * if the nulls value we got at the end of this lookup is
- * not the expected one, we must restart lookup.
- * We probably met an item that was moved to another chain.
- */
- if (get_nulls_value(node) != slot2)
- goto begin;
-
- if (result) {
-found:
- if (unlikely(!atomic_inc_not_zero_hint(&result->sk_refcnt, 2)))
- result = NULL;
- else if (unlikely(compute_score2(result, net, saddr, sport,
- daddr, hnum, dif) < badness)) {
- sock_put(result);
- goto begin;
- }
- }
return result;
}
+/* rcu_read_lock() must be held */
struct sock *__udp6_lib_lookup(struct net *net,
const struct in6_addr *saddr, __be16 sport,
const struct in6_addr *daddr, __be16 dport,
@@ -279,15 +253,12 @@ struct sock *__udp6_lib_lookup(struct net *net,
struct sk_buff *skb)
{
struct sock *sk, *result;
- struct hlist_nulls_node *node;
unsigned short hnum = ntohs(dport);
unsigned int hash2, slot2, slot = udp_hashfn(net, hnum, udptable->mask);
struct udp_hslot *hslot2, *hslot = &udptable->hash[slot];
int score, badness, matches = 0, reuseport = 0;
- bool select_ok = true;
u32 hash = 0;
- rcu_read_lock();
if (hslot->count > 10) {
hash2 = udp6_portaddr_hash(net, daddr, hnum);
slot2 = hash2 & udptable->mask;
@@ -309,34 +280,26 @@ struct sock *__udp6_lib_lookup(struct net *net,
&in6addr_any, hnum, dif,
hslot2, slot2, skb);
}
- rcu_read_unlock();
return result;
}
begin:
result = NULL;
badness = -1;
- sk_nulls_for_each_rcu(sk, node, &hslot->head) {
+ sk_for_each_rcu(sk, &hslot->head) {
score = compute_score(sk, net, hnum, saddr, sport, daddr, dport, dif);
if (score > badness) {
- result = sk;
- badness = score;
reuseport = sk->sk_reuseport;
if (reuseport) {
hash = udp6_ehashfn(net, daddr, hnum,
saddr, sport);
- if (select_ok) {
- struct sock *sk2;
-
- sk2 = reuseport_select_sock(sk, hash, skb,
+ result = reuseport_select_sock(sk, hash, skb,
sizeof(struct udphdr));
- if (sk2) {
- result = sk2;
- select_ok = false;
- goto found;
- }
- }
+ if (result)
+ return result;
matches = 1;
}
+ result = sk;
+ badness = score;
} else if (score == badness && reuseport) {
matches++;
if (reciprocal_scale(hash, matches) == 0)
@@ -344,25 +307,6 @@ begin:
hash = next_pseudo_random32(hash);
}
}
- /*
- * if the nulls value we got at the end of this lookup is
- * not the expected one, we must restart lookup.
- * We probably met an item that was moved to another chain.
- */
- if (get_nulls_value(node) != slot)
- goto begin;
-
- if (result) {
-found:
- if (unlikely(!atomic_inc_not_zero_hint(&result->sk_refcnt, 2)))
- result = NULL;
- else if (unlikely(compute_score(result, net, hnum, saddr, sport,
- daddr, dport, dif) < badness)) {
- sock_put(result);
- goto begin;
- }
- }
- rcu_read_unlock();
return result;
}
EXPORT_SYMBOL_GPL(__udp6_lib_lookup);
@@ -382,12 +326,24 @@ static struct sock *__udp6_lib_lookup_skb(struct sk_buff *skb,
udptable, skb);
}
+/* Must be called under rcu_read_lock().
+ * Does increment socket refcount.
+ */
+#if IS_ENABLED(CONFIG_NETFILTER_XT_MATCH_SOCKET) || \
+ IS_ENABLED(CONFIG_NETFILTER_XT_TARGET_TPROXY)
struct sock *udp6_lib_lookup(struct net *net, const struct in6_addr *saddr, __be16 sport,
const struct in6_addr *daddr, __be16 dport, int dif)
{
- return __udp6_lib_lookup(net, saddr, sport, daddr, dport, dif, &udp_table, NULL);
+ struct sock *sk;
+
+ sk = __udp6_lib_lookup(net, saddr, sport, daddr, dport,
+ dif, &udp_table, NULL);
+ if (sk && !atomic_inc_not_zero(&sk->sk_refcnt))
+ sk = NULL;
+ return sk;
}
EXPORT_SYMBOL_GPL(udp6_lib_lookup);
+#endif
/*
* This should be easy, if there is something there we
@@ -585,7 +541,7 @@ void __udp6_lib_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
sk->sk_err = err;
sk->sk_error_report(sk);
out:
- sock_put(sk);
+ return;
}
static int __udpv6_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
@@ -747,33 +703,6 @@ static bool __udp_v6_is_mcast_sock(struct net *net, struct sock *sk,
return true;
}
-static void flush_stack(struct sock **stack, unsigned int count,
- struct sk_buff *skb, unsigned int final)
-{
- struct sk_buff *skb1 = NULL;
- struct sock *sk;
- unsigned int i;
-
- for (i = 0; i < count; i++) {
- sk = stack[i];
- if (likely(!skb1))
- skb1 = (i == final) ? skb : skb_clone(skb, GFP_ATOMIC);
- if (!skb1) {
- atomic_inc(&sk->sk_drops);
- UDP6_INC_STATS_BH(sock_net(sk), UDP_MIB_RCVBUFERRORS,
- IS_UDPLITE(sk));
- UDP6_INC_STATS_BH(sock_net(sk), UDP_MIB_INERRORS,
- IS_UDPLITE(sk));
- }
-
- if (skb1 && udpv6_queue_rcv_skb(sk, skb1) <= 0)
- skb1 = NULL;
- sock_put(sk);
- }
- if (unlikely(skb1))
- kfree_skb(skb1);
-}
-
static void udp6_csum_zero_error(struct sk_buff *skb)
{
/* RFC 2460 section 8.1 says that we SHOULD log
@@ -792,15 +721,15 @@ static int __udp6_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
const struct in6_addr *saddr, const struct in6_addr *daddr,
struct udp_table *udptable, int proto)
{
- struct sock *sk, *stack[256 / sizeof(struct sock *)];
+ struct sock *sk, *first = NULL;
const struct udphdr *uh = udp_hdr(skb);
- struct hlist_nulls_node *node;
unsigned short hnum = ntohs(uh->dest);
struct udp_hslot *hslot = udp_hashslot(udptable, net, hnum);
- int dif = inet6_iif(skb);
- unsigned int count = 0, offset = offsetof(typeof(*sk), sk_nulls_node);
+ unsigned int offset = offsetof(typeof(*sk), sk_node);
unsigned int hash2 = 0, hash2_any = 0, use_hash2 = (hslot->count > 10);
- bool inner_flushed = false;
+ int dif = inet6_iif(skb);
+ struct hlist_node *node;
+ struct sk_buff *nskb;
if (use_hash2) {
hash2_any = udp6_portaddr_hash(net, &in6addr_any, hnum) &
@@ -811,27 +740,32 @@ start_lookup:
offset = offsetof(typeof(*sk), __sk_common.skc_portaddr_node);
}
- spin_lock(&hslot->lock);
- sk_nulls_for_each_entry_offset(sk, node, &hslot->head, offset) {
- if (__udp_v6_is_mcast_sock(net, sk,
- uh->dest, daddr,
- uh->source, saddr,
- dif, hnum) &&
- /* If zero checksum and no_check is not on for
- * the socket then skip it.
- */
- (uh->check || udp_sk(sk)->no_check6_rx)) {
- if (unlikely(count == ARRAY_SIZE(stack))) {
- flush_stack(stack, count, skb, ~0);
- inner_flushed = true;
- count = 0;
- }
- stack[count++] = sk;
- sock_hold(sk);
+ sk_for_each_entry_offset_rcu(sk, node, &hslot->head, offset) {
+ if (!__udp_v6_is_mcast_sock(net, sk, uh->dest, daddr,
+ uh->source, saddr, dif, hnum))
+ continue;
+ /* If zero checksum and no_check is not on for
+ * the socket then skip it.
+ */
+ if (!uh->check && !udp_sk(sk)->no_check6_rx)
+ continue;
+ if (!first) {
+ first = sk;
+ continue;
+ }
+ nskb = skb_clone(skb, GFP_ATOMIC);
+ if (unlikely(!nskb)) {
+ atomic_inc(&sk->sk_drops);
+ UDP6_INC_STATS_BH(sock_net(sk), UDP_MIB_RCVBUFERRORS,
+ IS_UDPLITE(sk));
+ UDP6_INC_STATS_BH(sock_net(sk), UDP_MIB_INERRORS,
+ IS_UDPLITE(sk));
+ continue;
}
- }
- spin_unlock(&hslot->lock);
+ if (udpv6_queue_rcv_skb(sk, nskb) > 0)
+ kfree_skb(nskb);
+ }
/* Also lookup *:port if we are using hash2 and haven't done so yet. */
if (use_hash2 && hash2 != hash2_any) {
@@ -839,24 +773,18 @@ start_lookup:
goto start_lookup;
}
- if (count) {
- flush_stack(stack, count, skb, count - 1);
- } else {
- if (!inner_flushed)
- UDP_INC_STATS_BH(net, UDP_MIB_IGNOREDMULTI,
- proto == IPPROTO_UDPLITE);
- consume_skb(skb);
- }
+ if (!first || udpv6_queue_rcv_skb(first, skb) > 0)
+ kfree_skb(skb);
return 0;
}
int __udp6_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
int proto)
{
+ const struct in6_addr *saddr, *daddr;
struct net *net = dev_net(skb->dev);
- struct sock *sk;
struct udphdr *uh;
- const struct in6_addr *saddr, *daddr;
+ struct sock *sk;
u32 ulen = 0;
if (!pskb_may_pull(skb, sizeof(struct udphdr)))
@@ -910,7 +838,6 @@ int __udp6_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
int ret;
if (!uh->check && !udp_sk(sk)->no_check6_rx) {
- sock_put(sk);
udp6_csum_zero_error(skb);
goto csum_error;
}
@@ -920,7 +847,6 @@ int __udp6_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
ip6_compute_pseudo);
ret = udpv6_queue_rcv_skb(sk, skb);
- sock_put(sk);
/* a return value > 0 means to resubmit the input */
if (ret > 0)
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* Re: [PATCH RFC 3/9] net: Add fast receive encapsulation
From: Joe Perches @ 2016-03-25 22:31 UTC (permalink / raw)
To: David Miller, tom; +Cc: netdev, kernel-team
In-Reply-To: <20160325.164042.1532577255302949895.davem@davemloft.net>
On Fri, 2016-03-25 at 16:40 -0400, David Miller wrote:
> From: Tom Herbert <tom@herbertland.com>
> Date: Wed, 23 Mar 2016 15:36:52 -0700
>
> > +{
> > + struct udp_sock *up = udp_sk(sk);
> > + int is_udplite = IS_UDPLITE(sk);
> > +
> > + int (*encap_rcv)(struct sock *sk, struct sk_buff *skb);
> > +
>
> Small nit, please put this encap_rcv function pointer declaration at
> the top of the local variable list.
It might also be nice to remove the equivalent typedef and
use the same form in udp_tunnel.h
---
include/net/udp_tunnel.h | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/include/net/udp_tunnel.h b/include/net/udp_tunnel.h
index b831140..71885b1 100644
--- a/include/net/udp_tunnel.h
+++ b/include/net/udp_tunnel.h
@@ -62,15 +62,12 @@ static inline int udp_sock_create(struct net *net,
return -EPFNOSUPPORT;
}
-typedef int (*udp_tunnel_encap_rcv_t)(struct sock *sk, struct sk_buff *skb);
-typedef void (*udp_tunnel_encap_destroy_t)(struct sock *sk);
-
struct udp_tunnel_sock_cfg {
void *sk_user_data; /* user data used by encap_rcv call back */
/* Used for setting up udp_sock fields, see udp.h for details */
__u8 encap_type;
- udp_tunnel_encap_rcv_t encap_rcv;
- udp_tunnel_encap_destroy_t encap_destroy;
+ int (*encap_rcv)(struct sock *sk, struct sk_buff *skb);
+ void (*encap_destroy)(struct sock *sk);
};
/* Setup the given (UDP) sock to receive UDP encapsulated packets */
^ permalink raw reply related
* Re: [PATCH net-next v3.16]r8169: Correct value from speed 10 on MII_BMCR
From: Francois Romieu @ 2016-03-25 22:53 UTC (permalink / raw)
To: Phil Sutter; +Cc: Corcodel Marian, netdev
In-Reply-To: <20160325133113.3001B62813@mail.nwl.cc>
Phil Sutter <phil@nwl.cc> :
[...]
> Your patch submissions are getting better, also good to see you're
> finally using git-send-email. A few things need to be corrected though:
>
#define BMCR_RESV 0x003f /* Unused... */
#define BMCR_SPEED1000 0x0040 /* MSB of Speed (1000) */
#define BMCR_CTST 0x0080 /* Collision test */
#define BMCR_FULLDPLX 0x0100 /* Full duplex */
#define BMCR_ANRESTART 0x0200 /* Auto negotiation restart */
#define BMCR_ISOLATE 0x0400 /* Isolate data paths from MII */
#define BMCR_PDOWN 0x0800 /* Enable low power state */
#define BMCR_ANENABLE 0x1000 /* Enable auto negotiation */
#define BMCR_SPEED100 0x2000 /* Select 100Mbps */
#define BMCR_LOOPBACK 0x4000 /* TXD loopback bits */
BMCR_SPEED100 apart, *all* these bits are now set.
It does not make much sense.
> Also detailed instructions on how to trigger the problem you are fixing
> for would be good. In detail: Which specific hardware was used, in which
> situation did the problem occur, how did it behave in that situation and
> what was the expected behaviour?
Been there. Such requests are usually left unanswered. :o(
Btw, this stuff targets 3.16 (...) and net-next is still closed.
--
Ueimor
^ permalink raw reply
* Re: veth regression with "don’t modify ip_summed; doing so treats packets with bad checksums as good."
From: Vijay Pandurangan @ 2016-03-25 23:03 UTC (permalink / raw)
To: Ben Greear; +Cc: Cong Wang, netdev, Evan Jones, Cong Wang
In-Reply-To: <56F5BA45.2030706@candelatech.com>
On Fri, Mar 25, 2016 at 6:23 PM, Ben Greear <greearb@candelatech.com> wrote:
> On 03/25/2016 02:59 PM, Vijay Pandurangan wrote:
>>
>> consider two scenarios, where process a sends raw ethernet frames
>> containing UDP packets to b
>>
>> I) process a --> veth --> process b
>>
>> II) process a -> eth -> wire -> eth -> process b
>>
>> I believe (I) is the simplest setup we can create that will replicate this
>> bug.
>>
>> If process a sends frames that contain UDP packets to process b, what
>> is the behaviour we want if the UDP packet *has an incorrect
>> checksum*?
>>
>> It seems to me that I and II should have identical behaviour, and I
>> would think that (II) would not deliver the packets to the
>> application.
>>
>> In (I) with Cong's patch would we be delivering corrupt UDP packets to
>> process b despite an incorrect checksum in (I)?
>>
>> If so, I would argue that this patch isn't right.
>
>
> Checksums are normally used to deal with flaky transport mechanisms,
> and once a machine receives the frame, we do not keep re-calculating
> checksums
> as we move it through various drivers and subsystems.
>
> In particular, checksums are NOT a security mechanism and can be easily
> faked.
>
> Since packets sent on one veth never actually hit any unreliable transport
> before they are received on the peer veth, then there should be no need to
> checksum packets whose origin is known to be on the local machine.
That's a good argument. I'm trying to figure out how to reconcile
your thoughts with the argument that virtual ethernet devices are an
abstraction that should behave identically to perfectly-functional
physical ethernet devices when connected with a wire.
In my view, the invariant must be identical functionality, and if I
were writing a regression test for this system, that's what I would
test. I think optimizations for eliding checksums should be
implemented only if they don't alter this functionality.
There must be a way to structure / write this code so that we can
optimize veths without causing different behaviour ...
>
> Any frame sent from a socket can be considered to be a local packet in my
> opinion.
I'm not sure that's totally right. Your bridge is adding a delay to
your packets; it could just as easily be simulating corruption by
corrupting 5% of packets going through it. If this change allows
corrupt packets to be delivered to an application when they could not
be delivered if the packets were routed via physical eths, I think
that is a bug.
>
> That is what Cong's patch does as far as I can tell.
>
>
> Thanks,
> Ben
>
>
> --
> Ben Greear <greearb@candelatech.com>
> Candela Technologies Inc http://www.candelatech.com
>
^ permalink raw reply
* Re: [RFC PATCH] tcp: Add SOF_TIMESTAMPING_TX_EOR and allow MSG_EOR in tcp_sendmsg
From: Yuchung Cheng @ 2016-03-25 23:05 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Martin KaFai Lau, Network Development, Kernel Team, Eric Dumazet,
Neal Cardwell, Willem de Bruijn, Soheil Hassas Yeganeh
In-Reply-To: <CAF=yD-JPvjNz=CZH6X=Hj+0vSyTyZcwKJoeTdOZKjx1Ys17DNQ@mail.gmail.com>
On Thu, Mar 24, 2016 at 6:39 PM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> > This patch allows the user process to use MSG_EOR during
> > tcp_sendmsg to tell the kernel that it is the last byte
> > of an application response message.
> >
> > The user process can use the new SOF_TIMESTAMPING_TX_EOR to
> > ask the kernel to only track timestamp of the MSG_EOR byte.
>
> Selective timestamp requests is a useful addition. Soheil (cc-ed) also
> happens to be looking at this. That approach uses cmsg to selectively
> tag send calls, avoiding the need to define a new SOF_ flag.
>
> > The current SOF_TIMESTAMPING_TX_ACK is tracking the last
> > byte appended to a skb during the tcp_sendmsg. It may track
> > multiple bytes if the response spans across multiple skbs.
>
> It only tracks the last byte of the buffer passed in sendmsg. If a
> sendmsg results in multiple skbuffs, only the last skb is tracked. It
> is, however, possible that that skbuff is appended to in a follow-on
> sendmsg call. If multiple calls enable recording on an skbuff, only
> the last record request on an skb is kept.
>
> > it is enough to measure the response latency for application
> > protocol with a single request/response at a time (like HTTP 1.1
> > without pipeline), it does not work well for application protocol
> > with >1 pipeline responses (like HTTP2).
Looks like an interesting and useful patch. Since HTTP2 allows
multiplexing data stream frames from multiple logical streams on a
single socket,
how would you instrument to measure the latency of each stream? e.g.,
sendmsg of data_frame_1_of_stream_a
sendmsg of data_frame_1_of_stream_b
sendmsg of data_frame_2_of_stream_a
sendmsg of data_frame_1_of_stream_c
sendmsg of data_frame_2_of_stream_b
> >
> > Each skb can only track one tskey (which is the seq number of
> > the last byte of the message). To allow tracking the
> > last byte of multiple response messages, this patch takes an approach
> > by not appending to the last skb during tcp_sendmsg if the last skb's
> > tskey will be overwritten. A similar case also happens during retransmit.
> >
> > This approach avoids introducing another list to track the tskey. The
> > downside is that it will have less gso benefit and/or more outgoing
> > packets. Practically, due to the amount of measurement data generated,
> > sampling is usually used in production. (i.e. not every connection is
> > tracked).
>
> Agreed. This is the simplest approach to avoiding timestamp request
> overwrites. A list-based approach quickly becomes complex as skbuffs
> can be split and merged at various points.
>
> Since this use is rare, I would suggest making the code even simpler
> by just jumping to new_segment on a call with this MSG option (or
> cmsg) set, avoiding tcp_tx_ts_noappend_skb() on each new segment.
+1
>
> > One of our use case is at the webserver. The webserver tracks
> > the HTTP2 response latency by measuring when the webserver
> > sends the first byte to the socket till the TCP ACK of the last byte
> > is received. In the cases where we don't have client side
> > measurement, measuring from the server side is the only option.
> > In the cases we have the client side measurement, the server side
> > data can also be used to justify/cross-check-with the client
> > side data (e.g. is there slowness at the layer above the client's
> > TCP stack).
> >
> > The TCP PRR paper [1] also measures a similar metrics:
> > "The TCP latency of a HTTP response when the server sends the first
> > byte until it receives the acknowledgment (ACK) for the last byte."
> >
> > [1] Proportional Rate Reduction for TCP:
> > http://static.googleusercontent.com/media/research.google.com/en//pubs/archive/37486.pdf
> >
> > Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> > Cc: Eric Dumazet <edumazet@google.com>
> > Cc: Neal Cardwell <ncardwell@google.com>
> > Cc: Willem de Bruijn <willemb@google.com>
> > Cc: Yuchung Cheng <ycheng@google.com>
> > ---
> > include/uapi/linux/net_tstamp.h | 3 ++-
> > net/ipv4/tcp.c | 23 ++++++++++++++++++-----
> > net/ipv4/tcp_output.c | 9 +++++++--
> > 3 files changed, 27 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h
> > index 6d1abea..5376569 100644
> > --- a/include/uapi/linux/net_tstamp.h
> > +++ b/include/uapi/linux/net_tstamp.h
> > @@ -25,8 +25,9 @@ enum {
> > SOF_TIMESTAMPING_TX_ACK = (1<<9),
> > SOF_TIMESTAMPING_OPT_CMSG = (1<<10),
> > SOF_TIMESTAMPING_OPT_TSONLY = (1<<11),
> > + SOF_TIMESTAMPING_TX_EOR = (1<<12),
> >
> > - SOF_TIMESTAMPING_LAST = SOF_TIMESTAMPING_OPT_TSONLY,
> > + SOF_TIMESTAMPING_LAST = SOF_TIMESTAMPING_TX_EOR,
> > SOF_TIMESTAMPING_MASK = (SOF_TIMESTAMPING_LAST - 1) |
> > SOF_TIMESTAMPING_LAST
> > };
> > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > index 08b8b96..7de96eb 100644
> > --- a/net/ipv4/tcp.c
> > +++ b/net/ipv4/tcp.c
> > @@ -428,11 +428,16 @@ void tcp_init_sock(struct sock *sk)
> > }
> > EXPORT_SYMBOL(tcp_init_sock);
> >
> > -static void tcp_tx_timestamp(struct sock *sk, struct sk_buff *skb)
> > +static void tcp_tx_timestamp(struct sock *sk, struct sk_buff *skb, int flags)
> > {
> > if (sk->sk_tsflags) {
> > - struct skb_shared_info *shinfo = skb_shinfo(skb);
> > + struct skb_shared_info *shinfo;
> >
> > + if ((sk->sk_tsflags & SOF_TIMESTAMPING_TX_EOR) &&
> > + !(flags & MSG_EOR))
> > + return;
> > +
> > + shinfo = skb_shinfo(skb);
> > sock_tx_timestamp(sk, &shinfo->tx_flags);
> > if (shinfo->tx_flags & SKBTX_ANY_TSTAMP)
> > shinfo->tskey = TCP_SKB_CB(skb)->seq + skb->len - 1;
> > @@ -957,7 +962,7 @@ new_segment:
> > offset += copy;
> > size -= copy;
> > if (!size) {
> > - tcp_tx_timestamp(sk, skb);
> > + tcp_tx_timestamp(sk, skb, flags);
> > goto out;
> > }
> >
> > @@ -1073,6 +1078,14 @@ static int tcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg,
> > return err;
> > }
> >
> > +static bool tcp_tx_ts_noappend_skb(const struct sock *sk,
> > + const struct sk_buff *last_skb, int flags)
> > +{
> > + return unlikely((sk->sk_tsflags & SOF_TIMESTAMPING_TX_EOR) &&
> > + (flags & MSG_EOR) &&
>
> flags seems more likely to be cached than sk->sk_tsflags at this
> point, in which case swap those tests.
>
> > + (skb_shinfo(last_skb)->tx_flags & SKBTX_ANY_TSTAMP));
> > +}
> > +
> > int tcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
>
> for a non-rfc patch, also change do_tcp_sendpages
>
> > {
> > struct tcp_sock *tp = tcp_sk(sk);
> > @@ -1144,7 +1157,7 @@ int tcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
> > copy = max - skb->len;
> > }
> >
> > - if (copy <= 0) {
> > + if (copy <= 0 || tcp_tx_ts_noappend_skb(sk, skb, flags)) {
> > new_segment:
>
> This adds a test to every segment for a niche feature. See my point of
> just jumping here on first entering the loop.
^ permalink raw reply
* Re: veth regression with "don’t modify ip_summed; doing so treats packets with bad checksums as good."
From: Ben Greear @ 2016-03-25 23:46 UTC (permalink / raw)
To: Vijay Pandurangan; +Cc: Cong Wang, netdev, Evan Jones, Cong Wang
In-Reply-To: <CAKUBDd8FGh3mhDmoyH70WvNGyofPVbjdROPKzenm+6DLfALyqg@mail.gmail.com>
On 03/25/2016 04:03 PM, Vijay Pandurangan wrote:
> On Fri, Mar 25, 2016 at 6:23 PM, Ben Greear <greearb@candelatech.com> wrote:
>> On 03/25/2016 02:59 PM, Vijay Pandurangan wrote:
>>>
>>> consider two scenarios, where process a sends raw ethernet frames
>>> containing UDP packets to b
>>>
>>> I) process a --> veth --> process b
>>>
>>> II) process a -> eth -> wire -> eth -> process b
>>>
>>> I believe (I) is the simplest setup we can create that will replicate this
>>> bug.
>>>
>>> If process a sends frames that contain UDP packets to process b, what
>>> is the behaviour we want if the UDP packet *has an incorrect
>>> checksum*?
>>>
>>> It seems to me that I and II should have identical behaviour, and I
>>> would think that (II) would not deliver the packets to the
>>> application.
>>>
>>> In (I) with Cong's patch would we be delivering corrupt UDP packets to
>>> process b despite an incorrect checksum in (I)?
>>>
>>> If so, I would argue that this patch isn't right.
>>
>>
>> Checksums are normally used to deal with flaky transport mechanisms,
>> and once a machine receives the frame, we do not keep re-calculating
>> checksums
>> as we move it through various drivers and subsystems.
>>
>> In particular, checksums are NOT a security mechanism and can be easily
>> faked.
>>
>> Since packets sent on one veth never actually hit any unreliable transport
>> before they are received on the peer veth, then there should be no need to
>> checksum packets whose origin is known to be on the local machine.
>
> That's a good argument. I'm trying to figure out how to reconcile
> your thoughts with the argument that virtual ethernet devices are an
> abstraction that should behave identically to perfectly-functional
> physical ethernet devices when connected with a wire.
>
> In my view, the invariant must be identical functionality, and if I
> were writing a regression test for this system, that's what I would
> test. I think optimizations for eliding checksums should be
> implemented only if they don't alter this functionality.
>
> There must be a way to structure / write this code so that we can
> optimize veths without causing different behaviour ...
A real NIC can either do hardware checksums, or it cannot. If it
cannot, then the host must do it on the CPU for both transmit and
receive.
Veth is not a real NIC, and it cannot do hardware checksum offloading.
So, we either lie and pretend it does, or we eat massive amounts
of CPU usage to calculate and check checksums when sending across
a veth pair.
>> Any frame sent from a socket can be considered to be a local packet in my
>> opinion.
>
> I'm not sure that's totally right. Your bridge is adding a delay to
> your packets; it could just as easily be simulating corruption by
> corrupting 5% of packets going through it. If this change allows
> corrupt packets to be delivered to an application when they could not
> be delivered if the packets were routed via physical eths, I think
> that is a bug.
I actually do support corrupting the frame, but what I normally do is corrupt the contents
of the packet, and then recalculate the IP checksum (and TCP if it applies)
and send it on its way. The receiving NIC and stack will pass the frame up to
the application since the checksums match, and it would be up the application
to deal with it. So, I can easily cause an application to receive corrupted
frames over physical eths.
I can also corrupt without updating the checksums in case you want to
test another systems NIC and/or stack.
But, if I am purposely corrupting a frame destined for veth, then the only reason
I would want the stack to check the checksums is if I were testing my own
stack's checksum logic, and that seems to be a pretty limited use.
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: [PATCH net-next v3.16]r8169: Correct value from speed 10 on MII_BMCR
From: Phil Sutter @ 2016-03-25 23:51 UTC (permalink / raw)
To: Francois Romieu; +Cc: Corcodel Marian, netdev
In-Reply-To: <20160325225325.GA28935@electric-eye.fr.zoreil.com>
On Fri, Mar 25, 2016 at 11:53:25PM +0100, Francois Romieu wrote:
> Phil Sutter <phil@nwl.cc> :
> [...]
> > Your patch submissions are getting better, also good to see you're
> > finally using git-send-email. A few things need to be corrected though:
> >
>
> #define BMCR_RESV 0x003f /* Unused... */
> #define BMCR_SPEED1000 0x0040 /* MSB of Speed (1000) */
> #define BMCR_CTST 0x0080 /* Collision test */
> #define BMCR_FULLDPLX 0x0100 /* Full duplex */
> #define BMCR_ANRESTART 0x0200 /* Auto negotiation restart */
> #define BMCR_ISOLATE 0x0400 /* Isolate data paths from MII */
> #define BMCR_PDOWN 0x0800 /* Enable low power state */
> #define BMCR_ANENABLE 0x1000 /* Enable auto negotiation */
> #define BMCR_SPEED100 0x2000 /* Select 100Mbps */
> #define BMCR_LOOPBACK 0x4000 /* TXD loopback bits */
>
> BMCR_SPEED100 apart, *all* these bits are now set.
>
> It does not make much sense.
I presumed that already, but didn't care to check myself so instead
ignored the actual code change. Thanks for pointing out the futility of
the whole thing. :)
> > Also detailed instructions on how to trigger the problem you are fixing
> > for would be good. In detail: Which specific hardware was used, in which
> > situation did the problem occur, how did it behave in that situation and
> > what was the expected behaviour?
>
> Been there. Such requests are usually left unanswered. :o(
That's my impression from following the (somewhat amusing) former
threads, too. I was merely impressed by the sheer quality of this patch
in comparison to previous ones. Speaking of which, the presented
tenacity certainly earns some respect.
Cheers, Phil
^ permalink raw reply
* Re: [PATCH] ipv6: Fix the pmtu path for connected UDP socket
From: Martin KaFai Lau @ 2016-03-25 23:55 UTC (permalink / raw)
To: Wei Wang
Cc: Eric Dumazet, Cong Wang, Eric Dumazet, Wei Wang, David Miller,
Linux Kernel Network Developers
In-Reply-To: <CAC15z3g-LDOdpcsZEs6XFFOOb92bXAbdOORdnd+MTvF62TgSiA@mail.gmail.com>
On Wed, Mar 23, 2016 at 04:57:22PM -0700, Wei Wang wrote:
> What about something like this:
>
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index ed44663..21b4102 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -1394,6 +1394,19 @@ static void ip6_rt_update_pmtu(struct dst_entry
> *dst, struct sock *sk,
> __ip6_rt_update_pmtu(dst, sk, skb ? ipv6_hdr(skb) : NULL, mtu);
> }
>
> +static void ip6_fill_in_flow(struct flowi6 *fl6, struct net *net,
> + struct sk_buff *skb, int oif, u32 mark)
> +{
> + const struct ipv6hdr *iph = (struct ipv6hdr *) skb->data;
> +
> + memset(fl6, 0, sizeof(fl6));
> + fl6->flowi6_oif = oif;
> + fl6->flowi6_mark = mark ? mark : IP6_REPLY_MARK(net, skb->mark);
> + fl6->daddr = iph->daddr;
> + fl6->saddr = iph->saddr;
> + fl6->flowlabel = ip6_flowinfo(iph);
> +}
> +
> void ip6_update_pmtu(struct sk_buff *skb, struct net *net, __be32 mtu,
> int oif, u32 mark)
> {
> @@ -1401,13 +1414,7 @@ void ip6_update_pmtu(struct sk_buff *skb,
> struct net *net, __be32 mtu,
> struct dst_entry *dst;
> struct flowi6 fl6;
>
> - memset(&fl6, 0, sizeof(fl6));
> - fl6.flowi6_oif = oif;
> - fl6.flowi6_mark = mark ? mark : IP6_REPLY_MARK(net, skb->mark);
> - fl6.daddr = iph->daddr;
> - fl6.saddr = iph->saddr;
> - fl6.flowlabel = ip6_flowinfo(iph);
> -
> + ip6_fill_in_flow(&fl6, net, skb, oif, mark);
> dst = ip6_route_output(net, NULL, &fl6);
> if (!dst->error)
> __ip6_rt_update_pmtu(dst, NULL, iph, ntohl(mtu));
> @@ -1417,8 +1424,22 @@ EXPORT_SYMBOL_GPL(ip6_update_pmtu);
>
> void ip6_sk_update_pmtu(struct sk_buff *skb, struct sock *sk, __be32 mtu)
> {
> - ip6_update_pmtu(skb, sock_net(sk), mtu,
> + struct ipv6_pinfo *np = inet6_sk(sk);
> + struct dst_entry *dst_new;
> + struct flowi6 fl6;
> + struct net *net = sock_net(sk);
> +
> + ip6_update_pmtu(skb, net, mtu,
> + sk->sk_bound_dev_if, sk->sk_mark);
> +
> + if (sk->sk_state == TCP_ESTABLISHED &&
> + !sk_dst_check(sk, np->dst_cookie)) {
> + ip6_fill_in_flow(&fl6, net, skb,
> sk->sk_bound_dev_if, sk->sk_mark);
> + dst_new = ip6_route_output(net, NULL, &fl6);
> + if (!IS_ERR(dst_new))
> + ip6_dst_store(sk, dst_new, NULL, NULL);
> + }
> }
> EXPORT_SYMBOL_GPL(ip6_sk_update_pmtu);
Here is the change I come up with (based on your flow key buildup function
with one fix), take a little different approach in ip6_sk_update_pmtu()
and some explanation on ip6_dst_store() after looking at ip6_rt_check()
I have not included the sk lock yet. There is still a few things I don't fully
understand in another discussion thread.
[PATCH] ipv6: Update sk->sk_dst_cache in ip6_sk_update_pmtu()
There is a case in connected UDP socket such that
getsockopt(IPV6_MTU) will return a stale MTU value. The reproducible
sequence could be the following:
1. Create a connected UDP socket
2. Send some datagrams out
3. Receive a ICMPV6_PKT_TOOBIG
4. No new outgoing datagrams to trigger the sk_dst_check()
logic to update the sk->sk_dst_cache.
5. getsockopt(IPV6_MTU) returns the mtu from the invalid
sk->sk_dst_cache instead of the newly created RTF_CACHE clone which
contains the MTU value instructed by the ICMPV6_PKT_TOOBIG message.
One observation is that, the __udp6_lib_err() code path does not do a
sk_dst_check() and route relookup (if needed) after doing the pmtu
update (while TCP does).
This patch does a ip6_dst_check() in ip6_sk_update_pmtu() which is used
by UDP to do the pmtu update and update sk->sk_dst_cache if it is needed.
ip6_dst_store(sk, ndst, NULL, NULL) is used. NULL(s) are passed to
the daddr and saddr parameters for simplicity reason. The current
use case is only for the UDP lookup. The details is in ip6_rt_check().
At the point where ip6_dst_store(sk, ndst, NULL, NULL) is called in
this patch, ndst should be a RTF_CACHE clone (which implies
((struct rt6i_info *)ndst)->rt6i_dst.plen == 128) and ip6_rt_check()
will pass without even considering np->d/saddr_cache.
Even it has to use np->d/saddr_cache, the worst case is ip6_rt_check()
fails and causes one more route lookup.
Test:
Server (Connected UDP Socket):
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Route Details:
[root@arch-fb-vm1 ~]# ip -6 r show | egrep '2fac'
2fac::/64 dev eth0 proto kernel metric 256 pref medium
2fac:face::/64 via 2fac::face dev eth0 metric 1024 pref medium
A crappy python code to create a connected UDP socket:
import socket
import errno
HOST = '2fac::1'
PORT = 8080
s = socket.socket(socket.AF_INET6, socket.SOCK_DGRAM)
s.bind((HOST, PORT))
s.connect(('2fac:face::face', 53))
print("connected")
while True:
try:
data = s.recv(1024)
except socket.error as se:
if se.errno == errno.EMSGSIZE:
pmtu = s.getsockopt(41, 24)
print("PMTU:%d" % pmtu)
break
s.close()
Python program output after getting a ICMPV6_PKT_TOOBIG:
[root@arch-fb-vm1 ~]# python2 ~/devshare/kernel/tasks/fib6/udp-connect-53-8080.py
connected
PMTU:1300
Cache routes after recieving TOOBIG:
[root@arch-fb-vm1 ~]# ip -6 r show table cache
2fac:face::face via 2fac::face dev eth0 metric 0
cache expires 463sec mtu 1300 pref medium
Client (Send the ICMPV6_PKT_TOOBIG):
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
scapy is used to generate the TOOBIG message. Here is the scrapy script I have
used:
>>> p=Ether(src='da:75:4d:36:ac:32', dst='52:54:00:12:34:66', type=0x86dd)/IPv6(src='2fac::face', dst='2fac::1')/ICMPv6PacketTooBig(mtu=1300)/IPv6(src='2fac::
1',dst='2fac:face::face', nh='UDP')/UDP(sport=8080,dport=53)
>>> sendp(p, iface='qemubr0')
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
Reported-by: Wei Wang <weiwan@google.com>
---
net/ipv6/route.c | 40 +++++++++++++++++++++++++++++++++-------
1 file changed, 33 insertions(+), 7 deletions(-)
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index ed44663..06934ff 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -1394,6 +1394,20 @@ static void ip6_rt_update_pmtu(struct dst_entry *dst, struct sock *sk,
__ip6_rt_update_pmtu(dst, sk, skb ? ipv6_hdr(skb) : NULL, mtu);
}
+static void build_skb_flow_key(struct flowi6 *fl6,
+ const struct sk_buff *skb,
+ struct net *net, int oif, u32 mark)
+{
+ const struct ipv6hdr *iph = (struct ipv6hdr *)skb->data;
+
+ memset(fl6, 0, sizeof(*fl6));
+ fl6->flowi6_oif = oif;
+ fl6->flowi6_mark = mark ? mark : IP6_REPLY_MARK(net, skb->mark);
+ fl6->daddr = iph->daddr;
+ fl6->saddr = iph->saddr;
+ fl6->flowlabel = ip6_flowinfo(iph);
+}
+
void ip6_update_pmtu(struct sk_buff *skb, struct net *net, __be32 mtu,
int oif, u32 mark)
{
@@ -1401,13 +1415,7 @@ void ip6_update_pmtu(struct sk_buff *skb, struct net *net, __be32 mtu,
struct dst_entry *dst;
struct flowi6 fl6;
- memset(&fl6, 0, sizeof(fl6));
- fl6.flowi6_oif = oif;
- fl6.flowi6_mark = mark ? mark : IP6_REPLY_MARK(net, skb->mark);
- fl6.daddr = iph->daddr;
- fl6.saddr = iph->saddr;
- fl6.flowlabel = ip6_flowinfo(iph);
-
+ build_skb_flow_key(&fl6, skb, net, oif, mark);
dst = ip6_route_output(net, NULL, &fl6);
if (!dst->error)
__ip6_rt_update_pmtu(dst, NULL, iph, ntohl(mtu));
@@ -1417,8 +1425,26 @@ EXPORT_SYMBOL_GPL(ip6_update_pmtu);
void ip6_sk_update_pmtu(struct sk_buff *skb, struct sock *sk, __be32 mtu)
{
+ struct dst_entry *odst;
+
+ odst = sk_dst_get(sk);
+
ip6_update_pmtu(skb, sock_net(sk), mtu,
sk->sk_bound_dev_if, sk->sk_mark);
+
+ if (odst && !odst->error &&
+ !ip6_dst_check(odst, inet6_sk(sk)->dst_cookie)) {
+ struct dst_entry *ndst;
+ struct flowi6 fl6;
+
+ build_skb_flow_key(&fl6, skb, sock_net(sk),
+ sk->sk_bound_dev_if, sk->sk_mark);
+ ndst = ip6_route_output(sock_net(sk), NULL, &fl6);
+ if (!ndst->error)
+ ip6_dst_store(sk, ndst, NULL, NULL);
+ }
+
+ dst_release(odst);
}
EXPORT_SYMBOL_GPL(ip6_sk_update_pmtu);
^ permalink raw reply related
* Re: [RFC net-next 2/2] udp: No longer use SLAB_DESTROY_BY_RCU
From: Tom Herbert @ 2016-03-26 0:08 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S . Miller, netdev, Eric Dumazet
In-Reply-To: <1458944964-12890-3-git-send-email-edumazet@google.com>
On Fri, Mar 25, 2016 at 3:29 PM, Eric Dumazet <edumazet@google.com> wrote:
> Tom Herbert would like to avoid touching UDP socket refcnt for encapsulated
> traffic. For this to happen, we need to use normal RCU rules, with a grace
> period before freeing a socket. UDP sockets are not short lived in the
> high usage case, so the added cost of call_rcu() should not be a concern.
>
> This actually removes a lot of complexity in UDP stack
>
> Multicast receives no longer need to hold a bucket lock.
>
> Note that ip early demux still needs to take a reference on the socket.
>
> Same remark for functions used by xt_socket and xt_PROXY netfilter modules,
> but this might be changed later.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Tom Herbert <tom@herbertland.com>
> ---
> include/linux/udp.h | 8 +-
> include/net/sock.h | 12 +--
> include/net/udp.h | 2 +-
> net/ipv4/udp.c | 290 +++++++++++++++-------------------------------------
> net/ipv4/udp_diag.c | 18 ++--
> net/ipv6/udp.c | 194 +++++++++++------------------------
> 6 files changed, 162 insertions(+), 362 deletions(-)
>
> diff --git a/include/linux/udp.h b/include/linux/udp.h
> index 87c094961bd5..32342754643a 100644
> --- a/include/linux/udp.h
> +++ b/include/linux/udp.h
> @@ -98,11 +98,11 @@ static inline bool udp_get_no_check6_rx(struct sock *sk)
> return udp_sk(sk)->no_check6_rx;
> }
>
> -#define udp_portaddr_for_each_entry(__sk, node, list) \
> - hlist_nulls_for_each_entry(__sk, node, list, __sk_common.skc_portaddr_node)
> +#define udp_portaddr_for_each_entry(__sk, list) \
> + hlist_for_each_entry(__sk, list, __sk_common.skc_portaddr_node)
>
> -#define udp_portaddr_for_each_entry_rcu(__sk, node, list) \
> - hlist_nulls_for_each_entry_rcu(__sk, node, list, __sk_common.skc_portaddr_node)
> +#define udp_portaddr_for_each_entry_rcu(__sk, list) \
> + hlist_for_each_entry_rcu(__sk, list, __sk_common.skc_portaddr_node)
>
> #define IS_UDPLITE(__sk) (udp_sk(__sk)->pcflag)
>
> diff --git a/include/net/sock.h b/include/net/sock.h
> index c88785a3e76c..5b9562bc478e 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -178,7 +178,7 @@ struct sock_common {
> int skc_bound_dev_if;
> union {
> struct hlist_node skc_bind_node;
> - struct hlist_nulls_node skc_portaddr_node;
> + struct hlist_node skc_portaddr_node;
> };
> struct proto *skc_prot;
> possible_net_t skc_net;
> @@ -670,18 +670,18 @@ static inline void sk_add_bind_node(struct sock *sk,
> hlist_for_each_entry(__sk, list, sk_bind_node)
>
> /**
> - * sk_nulls_for_each_entry_offset - iterate over a list at a given struct offset
> + * sk_for_each_entry_offset - iterate over a list at a given struct offset
> * @tpos: the type * to use as a loop cursor.
> * @pos: the &struct hlist_node to use as a loop cursor.
> * @head: the head for your list.
> * @offset: offset of hlist_node within the struct.
> *
> */
> -#define sk_nulls_for_each_entry_offset(tpos, pos, head, offset) \
> - for (pos = (head)->first; \
> - (!is_a_nulls(pos)) && \
> +#define sk_for_each_entry_offset_rcu(tpos, pos, head, offset) \
> + for (pos = rcu_dereference((head)->first); \
> + pos != NULL && \
> ({ tpos = (typeof(*tpos) *)((void *)pos - offset); 1;}); \
> - pos = pos->next)
> + pos = rcu_dereference(pos->next))
>
> static inline struct user_namespace *sk_user_ns(struct sock *sk)
> {
> diff --git a/include/net/udp.h b/include/net/udp.h
> index 92927f729ac8..d870ec1611c4 100644
> --- a/include/net/udp.h
> +++ b/include/net/udp.h
> @@ -59,7 +59,7 @@ struct udp_skb_cb {
> * @lock: spinlock protecting changes to head/count
> */
> struct udp_hslot {
> - struct hlist_nulls_head head;
> + struct hlist_head head;
> int count;
> spinlock_t lock;
> } __attribute__((aligned(2 * sizeof(long))));
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index 08eed5e16df0..3ebca8445d35 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -143,10 +143,9 @@ static int udp_lib_lport_inuse(struct net *net, __u16 num,
> unsigned int log)
> {
> struct sock *sk2;
> - struct hlist_nulls_node *node;
> kuid_t uid = sock_i_uid(sk);
>
> - sk_nulls_for_each(sk2, node, &hslot->head) {
> + sk_for_each(sk2, &hslot->head) {
> if (net_eq(sock_net(sk2), net) &&
> sk2 != sk &&
> (bitmap || udp_sk(sk2)->udp_port_hash == num) &&
> @@ -177,12 +176,11 @@ static int udp_lib_lport_inuse2(struct net *net, __u16 num,
> bool match_wildcard))
> {
> struct sock *sk2;
> - struct hlist_nulls_node *node;
> kuid_t uid = sock_i_uid(sk);
> int res = 0;
>
> spin_lock(&hslot2->lock);
> - udp_portaddr_for_each_entry(sk2, node, &hslot2->head) {
> + udp_portaddr_for_each_entry(sk2, &hslot2->head) {
> if (net_eq(sock_net(sk2), net) &&
> sk2 != sk &&
> (udp_sk(sk2)->udp_port_hash == num) &&
> @@ -207,11 +205,10 @@ static int udp_reuseport_add_sock(struct sock *sk, struct udp_hslot *hslot,
> bool match_wildcard))
> {
> struct net *net = sock_net(sk);
> - struct hlist_nulls_node *node;
> kuid_t uid = sock_i_uid(sk);
> struct sock *sk2;
>
> - sk_nulls_for_each(sk2, node, &hslot->head) {
> + sk_for_each(sk2, &hslot->head) {
> if (net_eq(sock_net(sk2), net) &&
> sk2 != sk &&
> sk2->sk_family == sk->sk_family &&
> @@ -333,17 +330,18 @@ found:
> goto fail_unlock;
> }
>
> - sk_nulls_add_node_rcu(sk, &hslot->head);
> + sk_add_node_rcu(sk, &hslot->head);
> hslot->count++;
> sock_prot_inuse_add(sock_net(sk), sk->sk_prot, 1);
>
> hslot2 = udp_hashslot2(udptable, udp_sk(sk)->udp_portaddr_hash);
> spin_lock(&hslot2->lock);
> - hlist_nulls_add_head_rcu(&udp_sk(sk)->udp_portaddr_node,
> + hlist_add_head_rcu(&udp_sk(sk)->udp_portaddr_node,
> &hslot2->head);
> hslot2->count++;
> spin_unlock(&hslot2->lock);
> }
> + sock_set_flag(sk, SOCK_RCU_FREE);
> error = 0;
> fail_unlock:
> spin_unlock_bh(&hslot->lock);
> @@ -497,37 +495,27 @@ static struct sock *udp4_lib_lookup2(struct net *net,
> struct sk_buff *skb)
> {
> struct sock *sk, *result;
> - struct hlist_nulls_node *node;
> int score, badness, matches = 0, reuseport = 0;
> - bool select_ok = true;
> u32 hash = 0;
>
> -begin:
> result = NULL;
> badness = 0;
> - udp_portaddr_for_each_entry_rcu(sk, node, &hslot2->head) {
> + udp_portaddr_for_each_entry_rcu(sk, &hslot2->head) {
> score = compute_score2(sk, net, saddr, sport,
> daddr, hnum, dif);
> if (score > badness) {
> - result = sk;
> - badness = score;
> reuseport = sk->sk_reuseport;
> if (reuseport) {
> hash = udp_ehashfn(net, daddr, hnum,
> saddr, sport);
> - if (select_ok) {
> - struct sock *sk2;
> -
> - sk2 = reuseport_select_sock(sk, hash, skb,
> + result = reuseport_select_sock(sk, hash, skb,
> sizeof(struct udphdr));
> - if (sk2) {
> - result = sk2;
> - select_ok = false;
> - goto found;
> - }
> - }
> + if (result)
> + return result;
> matches = 1;
> }
> + badness = score;
> + result = sk;
> } else if (score == badness && reuseport) {
> matches++;
> if (reciprocal_scale(hash, matches) == 0)
> @@ -535,23 +523,6 @@ begin:
> hash = next_pseudo_random32(hash);
> }
> }
> - /*
> - * if the nulls value we got at the end of this lookup is
> - * not the expected one, we must restart lookup.
> - * We probably met an item that was moved to another chain.
> - */
> - if (get_nulls_value(node) != slot2)
> - goto begin;
> - if (result) {
> -found:
> - if (unlikely(!atomic_inc_not_zero_hint(&result->sk_refcnt, 2)))
> - result = NULL;
> - else if (unlikely(compute_score2(result, net, saddr, sport,
> - daddr, hnum, dif) < badness)) {
> - sock_put(result);
> - goto begin;
> - }
> - }
> return result;
> }
>
> @@ -563,15 +534,12 @@ struct sock *__udp4_lib_lookup(struct net *net, __be32 saddr,
> int dif, struct udp_table *udptable, struct sk_buff *skb)
> {
> struct sock *sk, *result;
> - struct hlist_nulls_node *node;
> unsigned short hnum = ntohs(dport);
> unsigned int hash2, slot2, slot = udp_hashfn(net, hnum, udptable->mask);
> struct udp_hslot *hslot2, *hslot = &udptable->hash[slot];
> int score, badness, matches = 0, reuseport = 0;
> - bool select_ok = true;
> u32 hash = 0;
>
> - rcu_read_lock();
> if (hslot->count > 10) {
> hash2 = udp4_portaddr_hash(net, daddr, hnum);
> slot2 = hash2 & udptable->mask;
> @@ -593,35 +561,27 @@ struct sock *__udp4_lib_lookup(struct net *net, __be32 saddr,
> htonl(INADDR_ANY), hnum, dif,
> hslot2, slot2, skb);
> }
> - rcu_read_unlock();
> return result;
> }
> begin:
> result = NULL;
> badness = 0;
> - sk_nulls_for_each_rcu(sk, node, &hslot->head) {
> + sk_for_each_rcu(sk, &hslot->head) {
> score = compute_score(sk, net, saddr, hnum, sport,
> daddr, dport, dif);
> if (score > badness) {
> - result = sk;
> - badness = score;
> reuseport = sk->sk_reuseport;
> if (reuseport) {
> hash = udp_ehashfn(net, daddr, hnum,
> saddr, sport);
> - if (select_ok) {
> - struct sock *sk2;
> -
> - sk2 = reuseport_select_sock(sk, hash, skb,
> + result = reuseport_select_sock(sk, hash, skb,
> sizeof(struct udphdr));
> - if (sk2) {
> - result = sk2;
> - select_ok = false;
> - goto found;
> - }
> - }
> + if (result)
> + return result;
> matches = 1;
> }
> + result = sk;
> + badness = score;
> } else if (score == badness && reuseport) {
> matches++;
> if (reciprocal_scale(hash, matches) == 0)
> @@ -629,25 +589,6 @@ begin:
> hash = next_pseudo_random32(hash);
> }
> }
> - /*
> - * if the nulls value we got at the end of this lookup is
> - * not the expected one, we must restart lookup.
> - * We probably met an item that was moved to another chain.
> - */
> - if (get_nulls_value(node) != slot)
> - goto begin;
> -
> - if (result) {
> -found:
> - if (unlikely(!atomic_inc_not_zero_hint(&result->sk_refcnt, 2)))
> - result = NULL;
> - else if (unlikely(compute_score(result, net, saddr, hnum, sport,
> - daddr, dport, dif) < badness)) {
> - sock_put(result);
> - goto begin;
> - }
> - }
> - rcu_read_unlock();
> return result;
> }
> EXPORT_SYMBOL_GPL(__udp4_lib_lookup);
> @@ -663,13 +604,24 @@ static inline struct sock *__udp4_lib_lookup_skb(struct sk_buff *skb,
> udptable, skb);
> }
>
> +/* Must be called under rcu_read_lock().
It might be just as easy to do the rcu_read_lock() within the
function. That way we don't need to require callers to do it now.
> + * Does increment socket refcount.
> + */
> +#if IS_ENABLED(CONFIG_NETFILTER_XT_MATCH_SOCKET) || \
> + IS_ENABLED(CONFIG_NETFILTER_XT_TARGET_TPROXY)
> struct sock *udp4_lib_lookup(struct net *net, __be32 saddr, __be16 sport,
> __be32 daddr, __be16 dport, int dif)
> {
> - return __udp4_lib_lookup(net, saddr, sport, daddr, dport, dif,
> - &udp_table, NULL);
> + struct sock *sk;
> +
> + sk = __udp4_lib_lookup(net, saddr, sport, daddr, dport,
> + dif, &udp_table, NULL);
> + if (sk && !atomic_inc_not_zero(&sk->sk_refcnt))
> + sk = NULL;
> + return sk;
> }
> EXPORT_SYMBOL_GPL(udp4_lib_lookup);
> +#endif
>
> static inline bool __udp_is_mcast_sock(struct net *net, struct sock *sk,
> __be16 loc_port, __be32 loc_addr,
> @@ -771,7 +723,7 @@ void __udp4_lib_err(struct sk_buff *skb, u32 info, struct udp_table *udptable)
> sk->sk_err = err;
> sk->sk_error_report(sk);
> out:
> - sock_put(sk);
> + return;
> }
>
> void udp_err(struct sk_buff *skb, u32 info)
> @@ -1474,13 +1426,13 @@ void udp_lib_unhash(struct sock *sk)
> spin_lock_bh(&hslot->lock);
> if (rcu_access_pointer(sk->sk_reuseport_cb))
> reuseport_detach_sock(sk);
> - if (sk_nulls_del_node_init_rcu(sk)) {
> + if (sk_del_node_init_rcu(sk)) {
> hslot->count--;
> inet_sk(sk)->inet_num = 0;
> sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
>
> spin_lock(&hslot2->lock);
> - hlist_nulls_del_init_rcu(&udp_sk(sk)->udp_portaddr_node);
> + hlist_del_init_rcu(&udp_sk(sk)->udp_portaddr_node);
> hslot2->count--;
> spin_unlock(&hslot2->lock);
> }
> @@ -1513,12 +1465,12 @@ void udp_lib_rehash(struct sock *sk, u16 newhash)
>
> if (hslot2 != nhslot2) {
> spin_lock(&hslot2->lock);
> - hlist_nulls_del_init_rcu(&udp_sk(sk)->udp_portaddr_node);
> + hlist_del_init_rcu(&udp_sk(sk)->udp_portaddr_node);
> hslot2->count--;
> spin_unlock(&hslot2->lock);
>
> spin_lock(&nhslot2->lock);
> - hlist_nulls_add_head_rcu(&udp_sk(sk)->udp_portaddr_node,
> + hlist_add_head_rcu(&udp_sk(sk)->udp_portaddr_node,
> &nhslot2->head);
> nhslot2->count++;
> spin_unlock(&nhslot2->lock);
> @@ -1697,35 +1649,6 @@ drop:
> return -1;
> }
>
> -static void flush_stack(struct sock **stack, unsigned int count,
> - struct sk_buff *skb, unsigned int final)
> -{
> - unsigned int i;
> - struct sk_buff *skb1 = NULL;
> - struct sock *sk;
> -
> - for (i = 0; i < count; i++) {
> - sk = stack[i];
> - if (likely(!skb1))
> - skb1 = (i == final) ? skb : skb_clone(skb, GFP_ATOMIC);
> -
> - if (!skb1) {
> - atomic_inc(&sk->sk_drops);
> - UDP_INC_STATS_BH(sock_net(sk), UDP_MIB_RCVBUFERRORS,
> - IS_UDPLITE(sk));
> - UDP_INC_STATS_BH(sock_net(sk), UDP_MIB_INERRORS,
> - IS_UDPLITE(sk));
> - }
> -
> - if (skb1 && udp_queue_rcv_skb(sk, skb1) <= 0)
> - skb1 = NULL;
> -
> - sock_put(sk);
> - }
> - if (unlikely(skb1))
> - kfree_skb(skb1);
> -}
> -
> /* For TCP sockets, sk_rx_dst is protected by socket lock
> * For UDP, we use xchg() to guard against concurrent changes.
> */
> @@ -1749,14 +1672,14 @@ static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
> struct udp_table *udptable,
> int proto)
> {
> - struct sock *sk, *stack[256 / sizeof(struct sock *)];
> - struct hlist_nulls_node *node;
> + struct sock *sk, *first = NULL;
> unsigned short hnum = ntohs(uh->dest);
> struct udp_hslot *hslot = udp_hashslot(udptable, net, hnum);
> - int dif = skb->dev->ifindex;
> - unsigned int count = 0, offset = offsetof(typeof(*sk), sk_nulls_node);
> unsigned int hash2 = 0, hash2_any = 0, use_hash2 = (hslot->count > 10);
> - bool inner_flushed = false;
> + unsigned int offset = offsetof(typeof(*sk), sk_node);
> + int dif = skb->dev->ifindex;
> + struct hlist_node *node;
> + struct sk_buff *nskb;
>
> if (use_hash2) {
> hash2_any = udp4_portaddr_hash(net, htonl(INADDR_ANY), hnum) &
> @@ -1767,23 +1690,27 @@ start_lookup:
> offset = offsetof(typeof(*sk), __sk_common.skc_portaddr_node);
> }
>
> - spin_lock(&hslot->lock);
> - sk_nulls_for_each_entry_offset(sk, node, &hslot->head, offset) {
> - if (__udp_is_mcast_sock(net, sk,
> - uh->dest, daddr,
> - uh->source, saddr,
> - dif, hnum)) {
> - if (unlikely(count == ARRAY_SIZE(stack))) {
> - flush_stack(stack, count, skb, ~0);
> - inner_flushed = true;
> - count = 0;
> - }
> - stack[count++] = sk;
> - sock_hold(sk);
> + sk_for_each_entry_offset_rcu(sk, node, &hslot->head, offset) {
> + if (!__udp_is_mcast_sock(net, sk, uh->dest, daddr,
> + uh->source, saddr, dif, hnum))
> + continue;
> +
> + if (!first) {
> + first = sk;
> + continue;
> }
> - }
> + nskb = skb_clone(skb, GFP_ATOMIC);
>
> - spin_unlock(&hslot->lock);
> + if (unlikely(!nskb)) {
> + atomic_inc(&sk->sk_drops);
> + UDP_INC_STATS_BH(sock_net(sk), UDP_MIB_RCVBUFERRORS,
> + IS_UDPLITE(sk));
> + UDP_INC_STATS_BH(sock_net(sk), UDP_MIB_INERRORS,
> + IS_UDPLITE(sk));
> + continue;
> + }
> + udp_queue_rcv_skb(sk, nskb);
> + }
>
> /* Also lookup *:port if we are using hash2 and haven't done so yet. */
> if (use_hash2 && hash2 != hash2_any) {
> @@ -1791,17 +1718,10 @@ start_lookup:
> goto start_lookup;
> }
>
> - /*
> - * do the slow work with no lock held
> - */
> - if (count) {
> - flush_stack(stack, count, skb, count - 1);
> - } else {
> - if (!inner_flushed)
> - UDP_INC_STATS_BH(net, UDP_MIB_IGNOREDMULTI,
> - proto == IPPROTO_UDPLITE);
> - consume_skb(skb);
> - }
> + if (first)
> + udp_queue_rcv_skb(first, skb);
> + else
> + kfree_skb(skb);
> return 0;
> }
>
> @@ -1897,7 +1817,6 @@ int __udp4_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
> inet_compute_pseudo);
>
> ret = udp_queue_rcv_skb(sk, skb);
> - sock_put(sk);
>
> /* a return value > 0 means to resubmit the input, but
> * it wants the return to be -protocol, or 0
> @@ -1958,49 +1877,24 @@ static struct sock *__udp4_lib_mcast_demux_lookup(struct net *net,
> int dif)
> {
> struct sock *sk, *result;
> - struct hlist_nulls_node *node;
> unsigned short hnum = ntohs(loc_port);
> - unsigned int count, slot = udp_hashfn(net, hnum, udp_table.mask);
> + unsigned int slot = udp_hashfn(net, hnum, udp_table.mask);
> struct udp_hslot *hslot = &udp_table.hash[slot];
>
> /* Do not bother scanning a too big list */
> if (hslot->count > 10)
> return NULL;
>
> - rcu_read_lock();
> -begin:
> - count = 0;
> result = NULL;
> - sk_nulls_for_each_rcu(sk, node, &hslot->head) {
> - if (__udp_is_mcast_sock(net, sk,
> - loc_port, loc_addr,
> - rmt_port, rmt_addr,
> - dif, hnum)) {
> + sk_for_each_rcu(sk, &hslot->head) {
> + if (__udp_is_mcast_sock(net, sk, loc_port, loc_addr,
> + rmt_port, rmt_addr, dif, hnum)) {
> + if (result)
> + return NULL;
> result = sk;
> - ++count;
> }
> }
> - /*
> - * if the nulls value we got at the end of this lookup is
> - * not the expected one, we must restart lookup.
> - * We probably met an item that was moved to another chain.
> - */
> - if (get_nulls_value(node) != slot)
> - goto begin;
> -
> - if (result) {
> - if (count != 1 ||
> - unlikely(!atomic_inc_not_zero_hint(&result->sk_refcnt, 2)))
> - result = NULL;
> - else if (unlikely(!__udp_is_mcast_sock(net, result,
> - loc_port, loc_addr,
> - rmt_port, rmt_addr,
> - dif, hnum))) {
> - sock_put(result);
> - result = NULL;
> - }
> - }
> - rcu_read_unlock();
> +
> return result;
> }
>
> @@ -2013,37 +1907,20 @@ static struct sock *__udp4_lib_demux_lookup(struct net *net,
> __be16 rmt_port, __be32 rmt_addr,
> int dif)
> {
> - struct sock *sk, *result;
> - struct hlist_nulls_node *node;
> unsigned short hnum = ntohs(loc_port);
> unsigned int hash2 = udp4_portaddr_hash(net, loc_addr, hnum);
> unsigned int slot2 = hash2 & udp_table.mask;
> struct udp_hslot *hslot2 = &udp_table.hash2[slot2];
> INET_ADDR_COOKIE(acookie, rmt_addr, loc_addr);
> const __portpair ports = INET_COMBINED_PORTS(rmt_port, hnum);
> + struct sock *sk;
>
> - rcu_read_lock();
> - result = NULL;
> - udp_portaddr_for_each_entry_rcu(sk, node, &hslot2->head) {
> + udp_portaddr_for_each_entry_rcu(sk, &hslot2->head) {
> if (INET_MATCH(sk, net, acookie,
> rmt_addr, loc_addr, ports, dif))
> - result = sk;
> - /* Only check first socket in chain */
> - break;
> - }
> -
> - if (result) {
> - if (unlikely(!atomic_inc_not_zero_hint(&result->sk_refcnt, 2)))
> - result = NULL;
> - else if (unlikely(!INET_MATCH(sk, net, acookie,
> - rmt_addr, loc_addr,
> - ports, dif))) {
> - sock_put(result);
> - result = NULL;
> - }
> + return sk;
> }
> - rcu_read_unlock();
> - return result;
> + return NULL;
> }
>
> void udp_v4_early_demux(struct sk_buff *skb)
> @@ -2051,7 +1928,7 @@ void udp_v4_early_demux(struct sk_buff *skb)
> struct net *net = dev_net(skb->dev);
> const struct iphdr *iph;
> const struct udphdr *uh;
> - struct sock *sk;
> + struct sock *sk = NULL;
> struct dst_entry *dst;
> int dif = skb->dev->ifindex;
> int ours;
> @@ -2083,11 +1960,9 @@ void udp_v4_early_demux(struct sk_buff *skb)
> } else if (skb->pkt_type == PACKET_HOST) {
> sk = __udp4_lib_demux_lookup(net, uh->dest, iph->daddr,
> uh->source, iph->saddr, dif);
> - } else {
> - return;
> }
>
> - if (!sk)
> + if (!sk || !atomic_inc_not_zero_hint(&sk->sk_refcnt, 2))
> return;
>
> skb->sk = sk;
> @@ -2387,14 +2262,13 @@ static struct sock *udp_get_first(struct seq_file *seq, int start)
>
> for (state->bucket = start; state->bucket <= state->udp_table->mask;
> ++state->bucket) {
> - struct hlist_nulls_node *node;
> struct udp_hslot *hslot = &state->udp_table->hash[state->bucket];
>
> - if (hlist_nulls_empty(&hslot->head))
> + if (hlist_empty(&hslot->head))
> continue;
>
> spin_lock_bh(&hslot->lock);
> - sk_nulls_for_each(sk, node, &hslot->head) {
> + sk_for_each(sk, &hslot->head) {
> if (!net_eq(sock_net(sk), net))
> continue;
> if (sk->sk_family == state->family)
> @@ -2413,7 +2287,7 @@ static struct sock *udp_get_next(struct seq_file *seq, struct sock *sk)
> struct net *net = seq_file_net(seq);
>
> do {
> - sk = sk_nulls_next(sk);
> + sk = sk_next(sk);
> } while (sk && (!net_eq(sock_net(sk), net) || sk->sk_family != state->family));
>
> if (!sk) {
> @@ -2622,12 +2496,12 @@ void __init udp_table_init(struct udp_table *table, const char *name)
>
> table->hash2 = table->hash + (table->mask + 1);
> for (i = 0; i <= table->mask; i++) {
> - INIT_HLIST_NULLS_HEAD(&table->hash[i].head, i);
> + INIT_HLIST_HEAD(&table->hash[i].head);
> table->hash[i].count = 0;
> spin_lock_init(&table->hash[i].lock);
> }
> for (i = 0; i <= table->mask; i++) {
> - INIT_HLIST_NULLS_HEAD(&table->hash2[i].head, i);
> + INIT_HLIST_HEAD(&table->hash2[i].head);
> table->hash2[i].count = 0;
> spin_lock_init(&table->hash2[i].lock);
> }
> diff --git a/net/ipv4/udp_diag.c b/net/ipv4/udp_diag.c
> index df1966f3b6ec..3d5ccf4b1412 100644
> --- a/net/ipv4/udp_diag.c
> +++ b/net/ipv4/udp_diag.c
> @@ -36,10 +36,11 @@ static int udp_dump_one(struct udp_table *tbl, struct sk_buff *in_skb,
> const struct inet_diag_req_v2 *req)
> {
> int err = -EINVAL;
> - struct sock *sk;
> + struct sock *sk = NULL;
> struct sk_buff *rep;
> struct net *net = sock_net(in_skb->sk);
>
> + rcu_read_lock();
> if (req->sdiag_family == AF_INET)
> sk = __udp4_lib_lookup(net,
> req->id.idiag_src[0], req->id.idiag_sport,
> @@ -54,9 +55,9 @@ static int udp_dump_one(struct udp_table *tbl, struct sk_buff *in_skb,
> req->id.idiag_dport,
> req->id.idiag_if, tbl, NULL);
> #endif
> - else
> - goto out_nosk;
> -
> + if (sk && !atomic_inc_not_zero(&sk->sk_refcnt))
> + sk = NULL;
> + rcu_read_unlock();
> err = -ENOENT;
> if (!sk)
> goto out_nosk;
> @@ -96,24 +97,23 @@ static void udp_dump(struct udp_table *table, struct sk_buff *skb,
> struct netlink_callback *cb,
> const struct inet_diag_req_v2 *r, struct nlattr *bc)
> {
> - int num, s_num, slot, s_slot;
> struct net *net = sock_net(skb->sk);
> + int num, s_num, slot, s_slot;
>
> s_slot = cb->args[0];
> num = s_num = cb->args[1];
>
> for (slot = s_slot; slot <= table->mask; s_num = 0, slot++) {
> - struct sock *sk;
> - struct hlist_nulls_node *node;
> struct udp_hslot *hslot = &table->hash[slot];
> + struct sock *sk;
>
> num = 0;
>
> - if (hlist_nulls_empty(&hslot->head))
> + if (hlist_empty(&hslot->head))
> continue;
>
> spin_lock_bh(&hslot->lock);
> - sk_nulls_for_each(sk, node, &hslot->head) {
> + sk_for_each(sk, &hslot->head) {
> struct inet_sock *inet = inet_sk(sk);
>
> if (!net_eq(sock_net(sk), net))
> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index fd25e447a5fa..eca04b25879b 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -213,37 +213,28 @@ static struct sock *udp6_lib_lookup2(struct net *net,
> struct sk_buff *skb)
> {
> struct sock *sk, *result;
> - struct hlist_nulls_node *node;
> int score, badness, matches = 0, reuseport = 0;
> - bool select_ok = true;
> u32 hash = 0;
>
> -begin:
> result = NULL;
> badness = -1;
> - udp_portaddr_for_each_entry_rcu(sk, node, &hslot2->head) {
> + udp_portaddr_for_each_entry_rcu(sk, &hslot2->head) {
> score = compute_score2(sk, net, saddr, sport,
> daddr, hnum, dif);
> if (score > badness) {
> - result = sk;
> - badness = score;
> reuseport = sk->sk_reuseport;
> if (reuseport) {
> hash = udp6_ehashfn(net, daddr, hnum,
> saddr, sport);
> - if (select_ok) {
> - struct sock *sk2;
>
> - sk2 = reuseport_select_sock(sk, hash, skb,
> + result = reuseport_select_sock(sk, hash, skb,
> sizeof(struct udphdr));
> - if (sk2) {
> - result = sk2;
> - select_ok = false;
> - goto found;
> - }
> - }
> + if (result)
> + return result;
> matches = 1;
> }
> + result = sk;
> + badness = score;
> } else if (score == badness && reuseport) {
> matches++;
> if (reciprocal_scale(hash, matches) == 0)
> @@ -251,27 +242,10 @@ begin:
> hash = next_pseudo_random32(hash);
> }
> }
> - /*
> - * if the nulls value we got at the end of this lookup is
> - * not the expected one, we must restart lookup.
> - * We probably met an item that was moved to another chain.
> - */
> - if (get_nulls_value(node) != slot2)
> - goto begin;
> -
> - if (result) {
> -found:
> - if (unlikely(!atomic_inc_not_zero_hint(&result->sk_refcnt, 2)))
> - result = NULL;
> - else if (unlikely(compute_score2(result, net, saddr, sport,
> - daddr, hnum, dif) < badness)) {
> - sock_put(result);
> - goto begin;
> - }
> - }
> return result;
> }
>
> +/* rcu_read_lock() must be held */
> struct sock *__udp6_lib_lookup(struct net *net,
> const struct in6_addr *saddr, __be16 sport,
> const struct in6_addr *daddr, __be16 dport,
> @@ -279,15 +253,12 @@ struct sock *__udp6_lib_lookup(struct net *net,
> struct sk_buff *skb)
> {
> struct sock *sk, *result;
> - struct hlist_nulls_node *node;
> unsigned short hnum = ntohs(dport);
> unsigned int hash2, slot2, slot = udp_hashfn(net, hnum, udptable->mask);
> struct udp_hslot *hslot2, *hslot = &udptable->hash[slot];
> int score, badness, matches = 0, reuseport = 0;
> - bool select_ok = true;
> u32 hash = 0;
>
> - rcu_read_lock();
> if (hslot->count > 10) {
> hash2 = udp6_portaddr_hash(net, daddr, hnum);
> slot2 = hash2 & udptable->mask;
> @@ -309,34 +280,26 @@ struct sock *__udp6_lib_lookup(struct net *net,
> &in6addr_any, hnum, dif,
> hslot2, slot2, skb);
> }
> - rcu_read_unlock();
> return result;
> }
> begin:
> result = NULL;
> badness = -1;
> - sk_nulls_for_each_rcu(sk, node, &hslot->head) {
> + sk_for_each_rcu(sk, &hslot->head) {
> score = compute_score(sk, net, hnum, saddr, sport, daddr, dport, dif);
> if (score > badness) {
> - result = sk;
> - badness = score;
> reuseport = sk->sk_reuseport;
> if (reuseport) {
> hash = udp6_ehashfn(net, daddr, hnum,
> saddr, sport);
> - if (select_ok) {
> - struct sock *sk2;
> -
> - sk2 = reuseport_select_sock(sk, hash, skb,
> + result = reuseport_select_sock(sk, hash, skb,
> sizeof(struct udphdr));
> - if (sk2) {
> - result = sk2;
> - select_ok = false;
> - goto found;
> - }
> - }
> + if (result)
> + return result;
> matches = 1;
> }
> + result = sk;
> + badness = score;
> } else if (score == badness && reuseport) {
> matches++;
> if (reciprocal_scale(hash, matches) == 0)
> @@ -344,25 +307,6 @@ begin:
> hash = next_pseudo_random32(hash);
> }
> }
> - /*
> - * if the nulls value we got at the end of this lookup is
> - * not the expected one, we must restart lookup.
> - * We probably met an item that was moved to another chain.
> - */
> - if (get_nulls_value(node) != slot)
> - goto begin;
> -
> - if (result) {
> -found:
> - if (unlikely(!atomic_inc_not_zero_hint(&result->sk_refcnt, 2)))
> - result = NULL;
> - else if (unlikely(compute_score(result, net, hnum, saddr, sport,
> - daddr, dport, dif) < badness)) {
> - sock_put(result);
> - goto begin;
> - }
> - }
> - rcu_read_unlock();
> return result;
> }
> EXPORT_SYMBOL_GPL(__udp6_lib_lookup);
> @@ -382,12 +326,24 @@ static struct sock *__udp6_lib_lookup_skb(struct sk_buff *skb,
> udptable, skb);
> }
>
> +/* Must be called under rcu_read_lock().
Same here.
> + * Does increment socket refcount.
> + */
> +#if IS_ENABLED(CONFIG_NETFILTER_XT_MATCH_SOCKET) || \
> + IS_ENABLED(CONFIG_NETFILTER_XT_TARGET_TPROXY)
> struct sock *udp6_lib_lookup(struct net *net, const struct in6_addr *saddr, __be16 sport,
> const struct in6_addr *daddr, __be16 dport, int dif)
> {
> - return __udp6_lib_lookup(net, saddr, sport, daddr, dport, dif, &udp_table, NULL);
> + struct sock *sk;
> +
> + sk = __udp6_lib_lookup(net, saddr, sport, daddr, dport,
> + dif, &udp_table, NULL);
> + if (sk && !atomic_inc_not_zero(&sk->sk_refcnt))
> + sk = NULL;
> + return sk;
> }
> EXPORT_SYMBOL_GPL(udp6_lib_lookup);
> +#endif
>
> /*
> * This should be easy, if there is something there we
> @@ -585,7 +541,7 @@ void __udp6_lib_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
> sk->sk_err = err;
> sk->sk_error_report(sk);
> out:
> - sock_put(sk);
> + return;
> }
>
> static int __udpv6_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
> @@ -747,33 +703,6 @@ static bool __udp_v6_is_mcast_sock(struct net *net, struct sock *sk,
> return true;
> }
>
> -static void flush_stack(struct sock **stack, unsigned int count,
> - struct sk_buff *skb, unsigned int final)
> -{
> - struct sk_buff *skb1 = NULL;
> - struct sock *sk;
> - unsigned int i;
> -
> - for (i = 0; i < count; i++) {
> - sk = stack[i];
> - if (likely(!skb1))
> - skb1 = (i == final) ? skb : skb_clone(skb, GFP_ATOMIC);
> - if (!skb1) {
> - atomic_inc(&sk->sk_drops);
> - UDP6_INC_STATS_BH(sock_net(sk), UDP_MIB_RCVBUFERRORS,
> - IS_UDPLITE(sk));
> - UDP6_INC_STATS_BH(sock_net(sk), UDP_MIB_INERRORS,
> - IS_UDPLITE(sk));
> - }
> -
> - if (skb1 && udpv6_queue_rcv_skb(sk, skb1) <= 0)
> - skb1 = NULL;
> - sock_put(sk);
> - }
> - if (unlikely(skb1))
> - kfree_skb(skb1);
> -}
> -
> static void udp6_csum_zero_error(struct sk_buff *skb)
> {
> /* RFC 2460 section 8.1 says that we SHOULD log
> @@ -792,15 +721,15 @@ static int __udp6_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
> const struct in6_addr *saddr, const struct in6_addr *daddr,
> struct udp_table *udptable, int proto)
> {
> - struct sock *sk, *stack[256 / sizeof(struct sock *)];
> + struct sock *sk, *first = NULL;
> const struct udphdr *uh = udp_hdr(skb);
> - struct hlist_nulls_node *node;
> unsigned short hnum = ntohs(uh->dest);
> struct udp_hslot *hslot = udp_hashslot(udptable, net, hnum);
> - int dif = inet6_iif(skb);
> - unsigned int count = 0, offset = offsetof(typeof(*sk), sk_nulls_node);
> + unsigned int offset = offsetof(typeof(*sk), sk_node);
> unsigned int hash2 = 0, hash2_any = 0, use_hash2 = (hslot->count > 10);
> - bool inner_flushed = false;
> + int dif = inet6_iif(skb);
> + struct hlist_node *node;
> + struct sk_buff *nskb;
>
> if (use_hash2) {
> hash2_any = udp6_portaddr_hash(net, &in6addr_any, hnum) &
> @@ -811,27 +740,32 @@ start_lookup:
> offset = offsetof(typeof(*sk), __sk_common.skc_portaddr_node);
> }
>
> - spin_lock(&hslot->lock);
> - sk_nulls_for_each_entry_offset(sk, node, &hslot->head, offset) {
> - if (__udp_v6_is_mcast_sock(net, sk,
> - uh->dest, daddr,
> - uh->source, saddr,
> - dif, hnum) &&
> - /* If zero checksum and no_check is not on for
> - * the socket then skip it.
> - */
> - (uh->check || udp_sk(sk)->no_check6_rx)) {
> - if (unlikely(count == ARRAY_SIZE(stack))) {
> - flush_stack(stack, count, skb, ~0);
> - inner_flushed = true;
> - count = 0;
> - }
> - stack[count++] = sk;
> - sock_hold(sk);
> + sk_for_each_entry_offset_rcu(sk, node, &hslot->head, offset) {
> + if (!__udp_v6_is_mcast_sock(net, sk, uh->dest, daddr,
> + uh->source, saddr, dif, hnum))
> + continue;
> + /* If zero checksum and no_check is not on for
> + * the socket then skip it.
> + */
> + if (!uh->check && !udp_sk(sk)->no_check6_rx)
> + continue;
> + if (!first) {
> + first = sk;
> + continue;
> + }
> + nskb = skb_clone(skb, GFP_ATOMIC);
> + if (unlikely(!nskb)) {
> + atomic_inc(&sk->sk_drops);
> + UDP6_INC_STATS_BH(sock_net(sk), UDP_MIB_RCVBUFERRORS,
> + IS_UDPLITE(sk));
> + UDP6_INC_STATS_BH(sock_net(sk), UDP_MIB_INERRORS,
> + IS_UDPLITE(sk));
> + continue;
> }
> - }
>
> - spin_unlock(&hslot->lock);
> + if (udpv6_queue_rcv_skb(sk, nskb) > 0)
> + kfree_skb(nskb);
> + }
>
> /* Also lookup *:port if we are using hash2 and haven't done so yet. */
> if (use_hash2 && hash2 != hash2_any) {
> @@ -839,24 +773,18 @@ start_lookup:
> goto start_lookup;
> }
>
> - if (count) {
> - flush_stack(stack, count, skb, count - 1);
> - } else {
> - if (!inner_flushed)
> - UDP_INC_STATS_BH(net, UDP_MIB_IGNOREDMULTI,
> - proto == IPPROTO_UDPLITE);
> - consume_skb(skb);
> - }
> + if (!first || udpv6_queue_rcv_skb(first, skb) > 0)
> + kfree_skb(skb);
> return 0;
> }
>
> int __udp6_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
> int proto)
> {
> + const struct in6_addr *saddr, *daddr;
> struct net *net = dev_net(skb->dev);
> - struct sock *sk;
> struct udphdr *uh;
> - const struct in6_addr *saddr, *daddr;
> + struct sock *sk;
> u32 ulen = 0;
>
> if (!pskb_may_pull(skb, sizeof(struct udphdr)))
> @@ -910,7 +838,6 @@ int __udp6_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
> int ret;
>
> if (!uh->check && !udp_sk(sk)->no_check6_rx) {
> - sock_put(sk);
> udp6_csum_zero_error(skb);
> goto csum_error;
> }
> @@ -920,7 +847,6 @@ int __udp6_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
> ip6_compute_pseudo);
>
> ret = udpv6_queue_rcv_skb(sk, skb);
> - sock_put(sk);
>
> /* a return value > 0 means to resubmit the input */
> if (ret > 0)
> --
> 2.8.0.rc3.226.g39d4020
>
^ permalink raw reply
* Re: [RFC net-next 1/2] net: add SOCK_RCU_FREE socket flag
From: Tom Herbert @ 2016-03-26 0:08 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S . Miller, netdev, Eric Dumazet
In-Reply-To: <1458944964-12890-2-git-send-email-edumazet@google.com>
On Fri, Mar 25, 2016 at 3:29 PM, Eric Dumazet <edumazet@google.com> wrote:
> We want a generic way to insert an RCU grace period before socket
> freeing for cases where RCU_SLAB_DESTROY_BY_RCU is adding too
> much overhead.
>
> SLAB_DESTROY_BY_RCU strict rules force us to take a reference
> on the socket sk_refcnt, and it is a performance problem for UDP
> encapsulation, or TCP synflood behavior, as many CPUs might
> attempt the atomic operations on a shared sk_refcnt
>
> UDP sockets and TCP listeners can set SOCK_RCU_FREE so that their
> lookup can use traditional RCU rules, without refcount changes.
> They can set the flag only once hashed and visible by other cpus.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Tom Herbert <tom@herbertland.com>
> ---
> include/net/sock.h | 2 ++
> net/core/sock.c | 14 +++++++++++++-
> 2 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 255d3e03727b..c88785a3e76c 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -438,6 +438,7 @@ struct sock {
> struct sk_buff *skb);
> void (*sk_destruct)(struct sock *sk);
> struct sock_reuseport __rcu *sk_reuseport_cb;
> + struct rcu_head sk_rcu;
> };
>
> #define __sk_user_data(sk) ((*((void __rcu **)&(sk)->sk_user_data)))
> @@ -720,6 +721,7 @@ enum sock_flags {
> */
> SOCK_FILTER_LOCKED, /* Filter cannot be changed anymore */
> SOCK_SELECT_ERR_QUEUE, /* Wake select on error queue */
> + SOCK_RCU_FREE, /* wait rcu grace period in sk_destruct() */
> };
>
> #define SK_FLAGS_TIMESTAMP ((1UL << SOCK_TIMESTAMP) | (1UL << SOCK_TIMESTAMPING_RX_SOFTWARE))
> diff --git a/net/core/sock.c b/net/core/sock.c
> index b67b9aedb230..238a94f879ca 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -1418,8 +1418,12 @@ struct sock *sk_alloc(struct net *net, int family, gfp_t priority,
> }
> EXPORT_SYMBOL(sk_alloc);
>
> -void sk_destruct(struct sock *sk)
> +/* Sockets having SOCK_RCU_FREE will call this function after one RCU
> + * grace period. This is the case for UDP sockets and TCP listeners.
> + */
> +static void __sk_destruct(struct rcu_head *head)
> {
> + struct sock *sk = container_of(head, struct sock, sk_rcu);
> struct sk_filter *filter;
>
> if (sk->sk_destruct)
> @@ -1448,6 +1452,14 @@ void sk_destruct(struct sock *sk)
> sk_prot_free(sk->sk_prot_creator, sk);
> }
>
> +void sk_destruct(struct sock *sk)
> +{
> + if (sock_flag(sk, SOCK_RCU_FREE))
> + call_rcu(&sk->sk_rcu, __sk_destruct);
> + else
> + __sk_destruct(&sk->sk_rcu);
> +}
> +
Very nice!
> static void __sk_free(struct sock *sk)
> {
> if (unlikely(sock_diag_has_destroy_listeners(sk) && sk->sk_net_refcnt))
> --
> 2.8.0.rc3.226.g39d4020
>
^ permalink raw reply
* [PATCH 1/2] net: dsa: mv88e6xxx: Introduce _mv88e6xxx_phy_page_{read,write}
From: Patrick Uiterwijk @ 2016-03-26 0:10 UTC (permalink / raw)
To: linux, davem
Cc: linux, vivien.didelot, andrew, netdev, dennis, pbrobinson,
Patrick Uiterwijk
Add versions of the phy_page_read and _write functions to
be used in a context where the SMI mutex is held.
Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org>
---
drivers/net/dsa/mv88e6xxx.c | 42 ++++++++++++++++++++++++++++++++----------
1 file changed, 32 insertions(+), 10 deletions(-)
diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index fa086e0..13db5d8 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -2708,37 +2708,59 @@ int mv88e6xxx_switch_reset(struct dsa_switch *ds, bool ppu_active)
return 0;
}
-int mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page, int reg)
+static int _mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page,
+ int reg)
{
- struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
int ret;
- mutex_lock(&ps->smi_mutex);
ret = _mv88e6xxx_phy_write_indirect(ds, port, 0x16, page);
if (ret < 0)
- goto error;
+ goto clear;
ret = _mv88e6xxx_phy_read_indirect(ds, port, reg);
-error:
+clear:
_mv88e6xxx_phy_write_indirect(ds, port, 0x16, 0x0);
- mutex_unlock(&ps->smi_mutex);
+
return ret;
}
-int mv88e6xxx_phy_page_write(struct dsa_switch *ds, int port, int page,
- int reg, int val)
+int mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page, int reg)
{
struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
int ret;
mutex_lock(&ps->smi_mutex);
+ ret = _mv88e6xxx_phy_page_read(ds, port, page, reg);
+ mutex_unlock(&ps->smi_mutex);
+
+ return ret;
+}
+
+static int _mv88e6xxx_phy_page_write(struct dsa_switch *ds, int port, int page,
+ int reg, int val)
+{
+ int ret;
+
ret = _mv88e6xxx_phy_write_indirect(ds, port, 0x16, page);
if (ret < 0)
- goto error;
+ goto clear;
ret = _mv88e6xxx_phy_write_indirect(ds, port, reg, val);
-error:
+clear:
_mv88e6xxx_phy_write_indirect(ds, port, 0x16, 0x0);
+
+ return ret;
+}
+
+int mv88e6xxx_phy_page_write(struct dsa_switch *ds, int port, int page,
+ int reg, int val)
+{
+ struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
+ int ret;
+
+ mutex_lock(&ps->smi_mutex);
+ ret = _mv88e6xxx_phy_page_write(ds, port, page, reg, val);
mutex_unlock(&ps->smi_mutex);
+
return ret;
}
--
2.7.4
^ permalink raw reply related
* [PATCH 2/2] net: dsa: mv88e6xxx: Clear the PDOWN bit on setup
From: Patrick Uiterwijk @ 2016-03-26 0:10 UTC (permalink / raw)
To: linux, davem
Cc: linux, vivien.didelot, andrew, netdev, dennis, pbrobinson,
Patrick Uiterwijk
In-Reply-To: <1458951004-27424-1-git-send-email-patrick@puiterwijk.org>
Some of the vendor-specific bootloaders set up this part
of the initialization for us, so this was never added.
However, since upstream bootloaders don't initialize the
chip specifically, they leave the fiber MII's PDOWN flag
set, which means that the CPU port doesn't connect.
This patch checks whether this flag has been clear prior
by something else, and if not make us clear it.
Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org>
---
drivers/net/dsa/mv88e6xxx.c | 99 +++++++++++++++++++++++++++++++--------------
drivers/net/dsa/mv88e6xxx.h | 8 ++++
2 files changed, 76 insertions(+), 31 deletions(-)
diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 13db5d8..05b2efb 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -2264,6 +2264,57 @@ static void mv88e6xxx_bridge_work(struct work_struct *work)
mutex_unlock(&ps->smi_mutex);
}
+static int _mv88e6xxx_phy_page_write(struct dsa_switch *ds, int port, int page,
+ int reg, int val)
+{
+ int ret;
+
+ ret = _mv88e6xxx_phy_write_indirect(ds, port, 0x16, page);
+ if (ret < 0)
+ goto clear;
+
+ ret = _mv88e6xxx_phy_write_indirect(ds, port, reg, val);
+clear:
+ _mv88e6xxx_phy_write_indirect(ds, port, 0x16, 0x0);
+
+ return ret;
+}
+
+static int _mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page,
+ int reg)
+{
+ int ret;
+
+ ret = _mv88e6xxx_phy_write_indirect(ds, port, 0x16, page);
+ if (ret < 0)
+ goto clear;
+
+ ret = _mv88e6xxx_phy_read_indirect(ds, port, reg);
+clear:
+ _mv88e6xxx_phy_write_indirect(ds, port, 0x16, 0x0);
+
+ return ret;
+}
+
+static int mv88e6xxx_power_on_serdes(struct dsa_switch *ds)
+{
+ int ret;
+
+ ret = _mv88e6xxx_phy_page_read(ds, REG_FIBER_SERDES, PAGE_FIBER_SERDES,
+ MII_BMCR);
+ if (ret < 0)
+ return ret;
+
+ if (ret & BMCR_PDOWN) {
+ ret = ret & ~BMCR_PDOWN;
+ ret = _mv88e6xxx_phy_page_write(ds, REG_FIBER_SERDES,
+ PAGE_FIBER_SERDES, MII_BMCR,
+ ret);
+ }
+
+ return ret;
+}
+
static int mv88e6xxx_setup_port(struct dsa_switch *ds, int port)
{
struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
@@ -2367,6 +2418,23 @@ static int mv88e6xxx_setup_port(struct dsa_switch *ds, int port)
goto abort;
}
+ /* If this port is connected to a SerDes, make sure the SerDes is not
+ * powered down.
+ */
+ if (mv88e6xxx_6352_family(ds)) {
+ ret = _mv88e6xxx_reg_read(ds, REG_PORT(port), PORT_STATUS);
+ if (ret < 0)
+ goto abort;
+ ret &= PORT_STATUS_CMODE_MASK;
+ if ((ret == PORT_STATUS_CMODE_100BASE_X) ||
+ (ret == PORT_STATUS_CMODE_1000BASE_X) ||
+ (ret == PORT_STATUS_CMODE_SGMII)) {
+ ret = mv88e6xxx_power_on_serdes(ds);
+ if (ret < 0)
+ goto abort;
+ }
+ }
+
/* Port Control 2: don't force a good FCS, set the maximum frame size to
* 10240 bytes, disable 802.1q tags checking, don't discard tagged or
* untagged frames on this port, do a destination address lookup on all
@@ -2708,21 +2776,6 @@ int mv88e6xxx_switch_reset(struct dsa_switch *ds, bool ppu_active)
return 0;
}
-static int _mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page,
- int reg)
-{
- int ret;
-
- ret = _mv88e6xxx_phy_write_indirect(ds, port, 0x16, page);
- if (ret < 0)
- goto clear;
- ret = _mv88e6xxx_phy_read_indirect(ds, port, reg);
-clear:
- _mv88e6xxx_phy_write_indirect(ds, port, 0x16, 0x0);
-
- return ret;
-}
-
int mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page, int reg)
{
struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
@@ -2735,22 +2788,6 @@ int mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page, int reg)
return ret;
}
-static int _mv88e6xxx_phy_page_write(struct dsa_switch *ds, int port, int page,
- int reg, int val)
-{
- int ret;
-
- ret = _mv88e6xxx_phy_write_indirect(ds, port, 0x16, page);
- if (ret < 0)
- goto clear;
-
- ret = _mv88e6xxx_phy_write_indirect(ds, port, reg, val);
-clear:
- _mv88e6xxx_phy_write_indirect(ds, port, 0x16, 0x0);
-
- return ret;
-}
-
int mv88e6xxx_phy_page_write(struct dsa_switch *ds, int port, int page,
int reg, int val)
{
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index 9a038ab..26a424a 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -28,6 +28,10 @@
#define SMI_CMD_OP_45_READ_DATA_INC ((3 << 10) | SMI_CMD_BUSY)
#define SMI_DATA 0x01
+/* Fiber/SERDES Registers are located at SMI address F, page 1 */
+#define REG_FIBER_SERDES 0x0f
+#define PAGE_FIBER_SERDES 0x01
+
#define REG_PORT(p) (0x10 + (p))
#define PORT_STATUS 0x00
#define PORT_STATUS_PAUSE_EN BIT(15)
@@ -45,6 +49,10 @@
#define PORT_STATUS_MGMII BIT(6) /* 6185 */
#define PORT_STATUS_TX_PAUSED BIT(5)
#define PORT_STATUS_FLOW_CTRL BIT(4)
+#define PORT_STATUS_CMODE_MASK 0x0f
+#define PORT_STATUS_CMODE_100BASE_X 0x8
+#define PORT_STATUS_CMODE_1000BASE_X 0x9
+#define PORT_STATUS_CMODE_SGMII 0xa
#define PORT_PCS_CTRL 0x01
#define PORT_PCS_CTRL_RGMII_DELAY_RXCLK BIT(15)
#define PORT_PCS_CTRL_RGMII_DELAY_TXCLK BIT(14)
--
2.7.4
^ permalink raw reply related
* Re: [PATCH] ipv6: Fix the pmtu path for connected UDP socket
From: Martin KaFai Lau @ 2016-03-26 0:16 UTC (permalink / raw)
To: Wei Wang
Cc: Eric Dumazet, Cong Wang, Eric Dumazet, Wei Wang, David Miller,
Linux Kernel Network Developers
In-Reply-To: <20160325235527.GA29327@kafai-mba.local>
On Fri, Mar 25, 2016 at 04:55:27PM -0700, Martin KaFai Lau wrote:
> void ip6_sk_update_pmtu(struct sk_buff *skb, struct sock *sk, __be32 mtu)
> {
> + struct dst_entry *odst;
> +
> + odst = sk_dst_get(sk);
> +
> ip6_update_pmtu(skb, sock_net(sk), mtu,
> sk->sk_bound_dev_if, sk->sk_mark);
> +
> + if (odst && !odst->error &&
> + !ip6_dst_check(odst, inet6_sk(sk)->dst_cookie)) {
> + struct dst_entry *ndst;
> + struct flowi6 fl6;
> +
> + build_skb_flow_key(&fl6, skb, sock_net(sk),
> + sk->sk_bound_dev_if, sk->sk_mark);
> + ndst = ip6_route_output(sock_net(sk), NULL, &fl6);
> + if (!ndst->error)
> + ip6_dst_store(sk, ndst, NULL, NULL);
oops...missed:
else
dst_release(ndst);
> + }
> +
> + dst_release(odst);
> }
> EXPORT_SYMBOL_GPL(ip6_sk_update_pmtu);
>
> --
> 2.5.1
^ permalink raw reply
* [PATCH] Drivers: isdn: hisax: isac.c: Fix assignment and check into one expression.
From: Cosmin-Gabriel Samoila @ 2016-03-26 0:49 UTC (permalink / raw)
To: isdn; +Cc: netdev, linux-kernel, Cosmin-Gabriel Samoila
Fix variable assignment inside if statement. It is error-prone and hard to read.
Signed-off-by: Cosmin-Gabriel Samoila <gabrielcsmo@gmail.com>
---
drivers/isdn/hisax/isac.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/isdn/hisax/isac.c b/drivers/isdn/hisax/isac.c
index 7fdf78f..df7e05c 100644
--- a/drivers/isdn/hisax/isac.c
+++ b/drivers/isdn/hisax/isac.c
@@ -215,9 +215,11 @@ isac_interrupt(struct IsdnCardState *cs, u_char val)
if (count == 0)
count = 32;
isac_empty_fifo(cs, count);
- if ((count = cs->rcvidx) > 0) {
+ count = cs->rcvidx;
+ if (count > 0) {
cs->rcvidx = 0;
- if (!(skb = alloc_skb(count, GFP_ATOMIC)))
+ skb = alloc_skb(count, GFP_ATOMIC);
+ if (!skb)
printk(KERN_WARNING "HiSax: D receive out of memory\n");
else {
memcpy(skb_put(skb, count), cs->rcvbuf, count);
@@ -251,7 +253,8 @@ isac_interrupt(struct IsdnCardState *cs, u_char val)
cs->tx_skb = NULL;
}
}
- if ((cs->tx_skb = skb_dequeue(&cs->sq))) {
+ cs->tx_skb = skb_dequeue(&cs->sq);
+ if (cs->tx_skb) {
cs->tx_cnt = 0;
isac_fill_fifo(cs);
} else
@@ -313,7 +316,8 @@ afterXPR:
#if ARCOFI_USE
if (v1 & 0x08) {
if (!cs->dc.isac.mon_rx) {
- if (!(cs->dc.isac.mon_rx = kmalloc(MAX_MON_FRAME, GFP_ATOMIC))) {
+ cs->dc.isac.mon_rx = kmalloc(MAX_MON_FRAME, GFP_ATOMIC);
+ if (!cs->dc.isac.mon_rx) {
if (cs->debug & L1_DEB_WARN)
debugl1(cs, "ISAC MON RX out of memory!");
cs->dc.isac.mocr &= 0xf0;
@@ -343,7 +347,8 @@ afterXPR:
afterMONR0:
if (v1 & 0x80) {
if (!cs->dc.isac.mon_rx) {
- if (!(cs->dc.isac.mon_rx = kmalloc(MAX_MON_FRAME, GFP_ATOMIC))) {
+ cs->dc.isac.mon_rx = kmalloc(MAX_MON_FRAME, GFP_ATOMIC);
+ if (!cs->dc.isac.mon_rx) {
if (cs->debug & L1_DEB_WARN)
debugl1(cs, "ISAC MON RX out of memory!");
cs->dc.isac.mocr &= 0x0f;
--
1.9.1
^ permalink raw reply related
* Re: [PATCH 1/2] net: dsa: mv88e6xxx: Introduce _mv88e6xxx_phy_page_{read,write}
From: Guenter Roeck @ 2016-03-26 1:45 UTC (permalink / raw)
To: Patrick Uiterwijk, davem
Cc: linux, vivien.didelot, andrew, netdev, dennis, pbrobinson
In-Reply-To: <1458951004-27424-1-git-send-email-patrick@puiterwijk.org>
On 03/25/2016 05:10 PM, Patrick Uiterwijk wrote:
> Add versions of the phy_page_read and _write functions to
> be used in a context where the SMI mutex is held.
>
> Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org>
> ---
> drivers/net/dsa/mv88e6xxx.c | 42 ++++++++++++++++++++++++++++++++----------
> 1 file changed, 32 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
> index fa086e0..13db5d8 100644
> --- a/drivers/net/dsa/mv88e6xxx.c
> +++ b/drivers/net/dsa/mv88e6xxx.c
> @@ -2708,37 +2708,59 @@ int mv88e6xxx_switch_reset(struct dsa_switch *ds, bool ppu_active)
> return 0;
> }
>
> -int mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page, int reg)
> +static int _mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page,
> + int reg)
> {
> - struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
> int ret;
>
> - mutex_lock(&ps->smi_mutex);
> ret = _mv88e6xxx_phy_write_indirect(ds, port, 0x16, page);
> if (ret < 0)
> - goto error;
> + goto clear;
> ret = _mv88e6xxx_phy_read_indirect(ds, port, reg);
> -error:
> +clear:
Is there some good reason for changing the name of those labels ?
Guenter
> _mv88e6xxx_phy_write_indirect(ds, port, 0x16, 0x0);
> - mutex_unlock(&ps->smi_mutex);
> +
> return ret;
> }
>
> -int mv88e6xxx_phy_page_write(struct dsa_switch *ds, int port, int page,
> - int reg, int val)
> +int mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page, int reg)
> {
> struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
> int ret;
>
> mutex_lock(&ps->smi_mutex);
> + ret = _mv88e6xxx_phy_page_read(ds, port, page, reg);
> + mutex_unlock(&ps->smi_mutex);
> +
> + return ret;
> +}
> +
> +static int _mv88e6xxx_phy_page_write(struct dsa_switch *ds, int port, int page,
> + int reg, int val)
> +{
> + int ret;
> +
> ret = _mv88e6xxx_phy_write_indirect(ds, port, 0x16, page);
> if (ret < 0)
> - goto error;
> + goto clear;
>
> ret = _mv88e6xxx_phy_write_indirect(ds, port, reg, val);
> -error:
> +clear:
> _mv88e6xxx_phy_write_indirect(ds, port, 0x16, 0x0);
> +
> + return ret;
> +}
> +
> +int mv88e6xxx_phy_page_write(struct dsa_switch *ds, int port, int page,
> + int reg, int val)
> +{
> + struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
> + int ret;
> +
> + mutex_lock(&ps->smi_mutex);
> + ret = _mv88e6xxx_phy_page_write(ds, port, page, reg, val);
> mutex_unlock(&ps->smi_mutex);
> +
> return ret;
> }
>
>
^ permalink raw reply
* Re: [RFC net-next 2/2] udp: No longer use SLAB_DESTROY_BY_RCU
From: Alexei Starovoitov @ 2016-03-26 1:55 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S . Miller, netdev, Eric Dumazet, Tom Herbert
In-Reply-To: <1458944964-12890-3-git-send-email-edumazet@google.com>
On Fri, Mar 25, 2016 at 03:29:24PM -0700, Eric Dumazet wrote:
> Tom Herbert would like to avoid touching UDP socket refcnt for encapsulated
> traffic. For this to happen, we need to use normal RCU rules, with a grace
> period before freeing a socket. UDP sockets are not short lived in the
> high usage case, so the added cost of call_rcu() should not be a concern.
>
> This actually removes a lot of complexity in UDP stack
>
> Multicast receives no longer need to hold a bucket lock.
>
> Note that ip early demux still needs to take a reference on the socket.
>
> Same remark for functions used by xt_socket and xt_PROXY netfilter modules,
> but this might be changed later.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Tom Herbert <tom@herbertland.com>
> ---
> include/linux/udp.h | 8 +-
> include/net/sock.h | 12 +--
> include/net/udp.h | 2 +-
> net/ipv4/udp.c | 290 +++++++++++++++-------------------------------------
> net/ipv4/udp_diag.c | 18 ++--
> net/ipv6/udp.c | 194 +++++++++++------------------------
> 6 files changed, 162 insertions(+), 362 deletions(-)
great stuff!
^ permalink raw reply
* Re: [PATCH 1/2] net: dsa: mv88e6xxx: Introduce _mv88e6xxx_phy_page_{read,write}
From: Patrick Uiterwijk @ 2016-03-26 1:58 UTC (permalink / raw)
To: Guenter Roeck
Cc: davem, linux, vivien.didelot, andrew, netdev, Dennis Gilmore,
Peter Robinson
In-Reply-To: <56F5E9D1.9030909@roeck-us.net>
On Sat, Mar 26, 2016 at 2:45 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 03/25/2016 05:10 PM, Patrick Uiterwijk wrote:
>>
>> Add versions of the phy_page_read and _write functions to
>> be used in a context where the SMI mutex is held.
>>
>> Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org>
>> ---
>> drivers/net/dsa/mv88e6xxx.c | 42
>> ++++++++++++++++++++++++++++++++----------
>> 1 file changed, 32 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
>> index fa086e0..13db5d8 100644
>> --- a/drivers/net/dsa/mv88e6xxx.c
>> +++ b/drivers/net/dsa/mv88e6xxx.c
>> @@ -2708,37 +2708,59 @@ int mv88e6xxx_switch_reset(struct dsa_switch *ds,
>> bool ppu_active)
>> return 0;
>> }
>>
>> -int mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page,
>> int reg)
>> +static int _mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int
>> page,
>> + int reg)
>> {
>> - struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
>> int ret;
>>
>> - mutex_lock(&ps->smi_mutex);
>> ret = _mv88e6xxx_phy_write_indirect(ds, port, 0x16, page);
>> if (ret < 0)
>> - goto error;
>> + goto clear;
>> ret = _mv88e6xxx_phy_read_indirect(ds, port, reg);
>> -error:
>> +clear:
>
>
> Is there some good reason for changing the name of those labels ?
Vivien suggested to rename this since it makes more clear that this write is
meant to return to page 0 to make sure that phylib doesn't get confused
about the currently active page.
Patrick
>
> Guenter
>
>
>> _mv88e6xxx_phy_write_indirect(ds, port, 0x16, 0x0);
>> - mutex_unlock(&ps->smi_mutex);
>> +
>> return ret;
>> }
>>
>> -int mv88e6xxx_phy_page_write(struct dsa_switch *ds, int port, int page,
>> - int reg, int val)
>> +int mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page,
>> int reg)
>> {
>> struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
>> int ret;
>>
>> mutex_lock(&ps->smi_mutex);
>> + ret = _mv88e6xxx_phy_page_read(ds, port, page, reg);
>> + mutex_unlock(&ps->smi_mutex);
>> +
>> + return ret;
>> +}
>> +
>> +static int _mv88e6xxx_phy_page_write(struct dsa_switch *ds, int port, int
>> page,
>> + int reg, int val)
>> +{
>> + int ret;
>> +
>> ret = _mv88e6xxx_phy_write_indirect(ds, port, 0x16, page);
>> if (ret < 0)
>> - goto error;
>> + goto clear;
>>
>> ret = _mv88e6xxx_phy_write_indirect(ds, port, reg, val);
>> -error:
>> +clear:
>> _mv88e6xxx_phy_write_indirect(ds, port, 0x16, 0x0);
>> +
>> + return ret;
>> +}
>> +
>> +int mv88e6xxx_phy_page_write(struct dsa_switch *ds, int port, int page,
>> + int reg, int val)
>> +{
>> + struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
>> + int ret;
>> +
>> + mutex_lock(&ps->smi_mutex);
>> + ret = _mv88e6xxx_phy_page_write(ds, port, page, reg, val);
>> mutex_unlock(&ps->smi_mutex);
>> +
>> return ret;
>> }
>>
>>
>
^ permalink raw reply
* Re: [PATCH 1/2] net: dsa: mv88e6xxx: Introduce _mv88e6xxx_phy_page_{read,write}
From: Guenter Roeck @ 2016-03-26 2:42 UTC (permalink / raw)
To: Patrick Uiterwijk
Cc: davem, linux, vivien.didelot, andrew, netdev, Dennis Gilmore,
Peter Robinson
In-Reply-To: <CAJweMdYoiA5mwHW3kSQOY0y46=GuLhnBor7mS3kXo9K3FGqW4g@mail.gmail.com>
On 03/25/2016 06:58 PM, Patrick Uiterwijk wrote:
> On Sat, Mar 26, 2016 at 2:45 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 03/25/2016 05:10 PM, Patrick Uiterwijk wrote:
>>>
>>> Add versions of the phy_page_read and _write functions to
>>> be used in a context where the SMI mutex is held.
>>>
>>> Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org>
>>> ---
>>> drivers/net/dsa/mv88e6xxx.c | 42
>>> ++++++++++++++++++++++++++++++++----------
>>> 1 file changed, 32 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
>>> index fa086e0..13db5d8 100644
>>> --- a/drivers/net/dsa/mv88e6xxx.c
>>> +++ b/drivers/net/dsa/mv88e6xxx.c
>>> @@ -2708,37 +2708,59 @@ int mv88e6xxx_switch_reset(struct dsa_switch *ds,
>>> bool ppu_active)
>>> return 0;
>>> }
>>>
>>> -int mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page,
>>> int reg)
>>> +static int _mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int
>>> page,
>>> + int reg)
>>> {
>>> - struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
>>> int ret;
>>>
>>> - mutex_lock(&ps->smi_mutex);
>>> ret = _mv88e6xxx_phy_write_indirect(ds, port, 0x16, page);
>>> if (ret < 0)
>>> - goto error;
>>> + goto clear;
>>> ret = _mv88e6xxx_phy_read_indirect(ds, port, reg);
>>> -error:
>>> +clear:
>>
>>
>> Is there some good reason for changing the name of those labels ?
>
> Vivien suggested to rename this since it makes more clear that this write is
> meant to return to page 0 to make sure that phylib doesn't get confused
> about the currently active page.
>
And "clear:" accomplishes that ? I would not have guessed.
Wonder if anyone else does. I would have used a comment.
/* Try to return to page 0 even after an error */
or something like that.
Guenter
> Patrick
>
>>
>> Guenter
>>
>>
>>> _mv88e6xxx_phy_write_indirect(ds, port, 0x16, 0x0);
>>> - mutex_unlock(&ps->smi_mutex);
>>> +
>>> return ret;
>>> }
>>>
>>> -int mv88e6xxx_phy_page_write(struct dsa_switch *ds, int port, int page,
>>> - int reg, int val)
>>> +int mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page,
>>> int reg)
>>> {
>>> struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
>>> int ret;
>>>
>>> mutex_lock(&ps->smi_mutex);
>>> + ret = _mv88e6xxx_phy_page_read(ds, port, page, reg);
>>> + mutex_unlock(&ps->smi_mutex);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int _mv88e6xxx_phy_page_write(struct dsa_switch *ds, int port, int
>>> page,
>>> + int reg, int val)
>>> +{
>>> + int ret;
>>> +
>>> ret = _mv88e6xxx_phy_write_indirect(ds, port, 0x16, page);
>>> if (ret < 0)
>>> - goto error;
>>> + goto clear;
>>>
>>> ret = _mv88e6xxx_phy_write_indirect(ds, port, reg, val);
>>> -error:
>>> +clear:
>>> _mv88e6xxx_phy_write_indirect(ds, port, 0x16, 0x0);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +int mv88e6xxx_phy_page_write(struct dsa_switch *ds, int port, int page,
>>> + int reg, int val)
>>> +{
>>> + struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
>>> + int ret;
>>> +
>>> + mutex_lock(&ps->smi_mutex);
>>> + ret = _mv88e6xxx_phy_page_write(ds, port, page, reg, val);
>>> mutex_unlock(&ps->smi_mutex);
>>> +
>>> return ret;
>>> }
>>>
>>>
>>
>
^ permalink raw reply
* Re: [PATCH net-next v3.16]r8169: Correct value from speed 10 on MII_BMCR
From: David Miller @ 2016-03-26 4:34 UTC (permalink / raw)
To: romieu; +Cc: phil, asd, netdev
In-Reply-To: <20160325225325.GA28935@electric-eye.fr.zoreil.com>
From: Francois Romieu <romieu@fr.zoreil.com>
Date: Fri, 25 Mar 2016 23:53:25 +0100
> Been there. Such requests are usually left unanswered. :o(
Due to this patch submitters continued anti-social and anti-community
behavior, I have been completely ignoring their patches.
I will continue to mark all of their patch submissions as "REJECTED"
in patchwork until they start to behave like proper community members
and actually respond properly and act upon to the feedback they are
given.
You should also feel free to ignore their work as well, your time is
valuable, don't waste it on someone who ignores everyone's feedback.
^ permalink raw reply
* [ask for help and advice] fib6_del triggered BUG_ON, because rt->rt6i_ref counter is 2
From: wangyufen @ 2016-03-26 7:10 UTC (permalink / raw)
To: Hannes Frederic Sowa, Hideaki YOSHIFUJI, Patrick McHardy,
Alexey Kuznetsov, James Morris, netdev
Hi, all
I used kernel-3.4 and applied patch
"6e9e16e6143b72 ipv6: replacing a rt6_info needs to purge possible propagated rt6_infos too",
I'm not sure which opertion triggered the BUG_ON, from vmcore, I got that
rt6i_ref's counter is 0x2 and rt6i_dst is ff02::02
Until now ,the BUG_ON triggered 3 times, rt6i_dst always multicast address (ff02::XXX),
So, I guess maybe there are some issues on fib6_add/fib6_del multicast address routes.
The latest BUG_ON logs:
<2>[ 407.197464] kernel BUG at /usr/src/packages/BUILD/kernel-default-3.4.24.19/linux-3.4/net/ipv6/ip6_fib.c:655!
<4>[ 407.220891] Pid: 0, comm: swapper/8 Tainted: P W O 3.4.24.19-0.11-default
<4>[ 407.220896] RIP: 0010:[<ffffffff813f3b79>] [<ffffffff813f3b79>] fib6_purge_rt+0xe9/0xf0
<4>[ 407.222627] RSP: 0018:ffff8801a1f05c30 EFLAGS: 00010202
<4>[ 407.222629] RAX: 0000000000000002 RBX: ffff880172d70c80 RCX: 000000018040001c
<4>[ 407.222631] RDX: ffffffff81856d00 RSI: 0000000000000000 RDI: ffff880172d70c80
<4>[ 407.222633] RBP: ffff8801a1f05c50 R08: ffff880121f26d00 R09: 000000018040001c
<4>[ 407.222635] R10: 0000000021f26601 R11: 0000000000000004 R12: ffff88013b9eeac0
<4>[ 407.222636] R13: ffff8801a1f05cf8 R14: ffffffff81856d00 R15: ffff8801596f4200
<4>[ 407.222639] FS: 0000000000000000(0000) GS:ffff8801a1f00000(0000) knlGS:0000000000000000
<4>[ 407.222641] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
<4>[ 407.222643] CR2: 00007f0b4c0136b0 CR3: 0000000103558000 CR4: 00000000001407e0
<4>[ 407.222645] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
<4>[ 407.222647] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
<4>[ 407.222650] Process swapper/8 (pid: 0, threadinfo ffff88017f746000, task ffff880173389720)
<4>[ 407.223060] Stack:
<4>[ 407.223144] ffff880121f26640 ffff8801a1f05cf8 ffffffff81856d00 ffff880172d70c80
<4>[ 407.223474] ffff8801a1f05ce0 ffffffff813f4b25 0000000000000092 ffff8801a1f05c78
<4>[ 407.223804] ffff8801546fdc80 ffff8801a1f05cd8 ffffffff8106a2dd ffff880159610048
<4>[ 407.224133] ffff8801a1f14788 000000000000000d 000000000000000d ffff8801a1f14700
<4>[ 407.224472] ffff88017fe17ee8 ffffffffa2effad0 ffff880172d70c80 ffff8801a1f05d90
<4>[ 407.224809] 0000000000000000 ffffffff813f2dd0 ffff8801a1f05d20 ffffffff813f4c59
<4>[ 407.225139] ffffffff810585f1 0000000000000000 ffffffff81856d00 0000000000000000
<4>[ 407.225484] ffff8801a1f05d90 ffff8801596f4218 ffff8801a1f05d40 ffffffff813f2c66
<4>[ 407.225822] ffff8801a1f05d40 ffff8801a1f05d90 ffff8801a1f05d70 ffffffff813f2cf5
<4>[ 407.226152] ffff8801a1f05d70 ffffffff81451489 ffff8801a1f05d70 ffffffff81856d00
<4>[ 407.226481] ffff8801a1f05e20 ffffffff813f4e3c 00000000000007f8 00ffffff81857100
<4>[ 407.226825] ffffffff818610a0 ffffffff818610a0 ffff8801596f4230 ffff88013b026f80
<4>[ 407.227154] 0000000000000000 000000003a310004 0000000000000006 ffffffff813f4bf0
<4>[ 407.227484] ffff8801a1f05e00 ffffffff81856d00 ffffffff813f2dd0 0000000000000000
<4>[ 407.227828] ffff8801a1f05e00 ffffffff81856d00 0000000000001d4c 0000000000000100
<4>[ 407.228159] ffffffff813f4f60 143dd57c7c88b3b2
<4>[ 407.228243] Call Trace:
<4>[ 407.228245] <IRQ>
<4>[ 407.228249] [<ffffffff813f4b25>] fib6_del+0x1e5/0x2b0
<4>[ 407.228255] [<ffffffff8106a2dd>] ? try_to_wake_up+0x1dd/0x2e0
<4>[ 407.228265] [<ffffffff813f2dd0>] ? fib6_dump_node+0x80/0x80
<4>[ 407.228268] [<ffffffff813f4c59>] fib6_clean_node+0x69/0xd0
<4>[ 407.228272] [<ffffffff810585f1>] ? autoremove_wake_function+0x11/0x40
<4>[ 407.228276] [<ffffffff813f2c66>] fib6_walk_continue+0x176/0x1b0
<4>[ 407.228279] [<ffffffff813f2cf5>] fib6_walk+0x55/0xb0
<4>[ 407.228284] [<ffffffff81451489>] ? _raw_write_lock_bh+0x19/0x20
<4>[ 407.228287] [<ffffffff813f4e3c>] fib6_clean_all+0x9c/0xe0
<4>[ 407.228290] [<ffffffff813f4bf0>] ? fib6_del+0x2b0/0x2b0
<4>[ 407.228293] [<ffffffff813f2dd0>] ? fib6_dump_node+0x80/0x80
<4>[ 407.228297] [<ffffffff813f4f60>] ? fib6_run_gc+0xe0/0xe0
<4>[ 407.228300] [<ffffffff813f4ecb>] fib6_run_gc+0x4b/0xe0
<4>[ 407.228303] [<ffffffff813f4f73>] fib6_gc_timer_cb+0x13/0x20
<4>[ 407.228308] [<ffffffff8104606f>] run_timer_softirq+0x14f/0x340
<4>[ 407.228312] [<ffffffff8108386f>] ? ktime_get+0x5f/0xe0
<4>[ 407.228316] [<ffffffff8103e5a1>] __do_softirq+0xd1/0x200
<4>[ 407.228320] [<ffffffff8105c7cc>] ? hrtimer_interrupt+0x12c/0x230
<4>[ 407.228324] [<ffffffff8145a5cc>] call_softirq+0x1c/0x30
<4>[ 407.228330] [<ffffffff8100414d>] do_softirq+0x6d/0xa0
<4>[ 407.228333] [<ffffffff8103e965>] irq_exit+0xa5/0xb0
<4>[ 407.228337] [<ffffffff810212f9>] smp_apic_timer_interrupt+0x69/0xa0
<4>[ 407.228341] [<ffffffff81459c0a>] apic_timer_interrupt+0x6a/0x70
<4>[ 407.228342] <EOI>
<4>[ 407.228366] [<ffffffffa0e08972>] ? arch_local_irq_enable+0xb/0xd [processor]
<4>[ 407.228379] [<ffffffffa0e09308>] acpi_idle_enter_c1+0x90/0xba [processor]
<4>[ 407.228390] [<ffffffff81326b69>] cpuidle_enter+0x19/0x20
<4>[ 407.228393] [<ffffffff813271a3>] cpuidle_idle_call+0xb3/0x260
<4>[ 407.228397] [<ffffffff8100b255>] cpu_idle+0x65/0xd0
<4>[ 407.228401] [<ffffffff814414a2>] start_secondary+0x200/0x202
<4>[ 407.228403] Code: 48 89 55 e0 48 89 75 e8 e8 75 58 f6 ff 48 8b 75 e8 48 8b 55 e0 e9 56 ff ff ff 0f 1f 84 00 00 00 00 00 48 8b 82 90 03 00 00 eb 87 <0f> 0b 0f 1f 44 00 00 55 41 89 d2 48 89 e5 41 57 41 56 41 55 49
<1>[ 407.228432] RIP [<ffffffff813f3b79>] fib6_purge_rt+0xe9/0xf0
<4>[ 407.228435] RSP <ffff8801a1f05c30>
^ permalink raw reply
* [PATCH net-next v3.16]r8169: Add delay on set_rx_mode
From: Corcodel Marian @ 2016-03-26 11:21 UTC (permalink / raw)
To: netdev; +Cc: Francois Romieu, Corcodel Marian
Set one line space extra delay on set_rx_mode for starting
on full duplex and full speed.
Signed-off-by: Corcodel Marian <asd@marian1000.go.ro>
---
drivers/net/ethernet/realtek/r8169.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 0ae8969..457b12a 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -4646,6 +4646,7 @@ static void rtl_set_rx_mode(struct net_device *dev)
mc_filter[1] = mc_filter[0] = 0xffffffff;
RTL_W32(MAR0 + 4, mc_filter[1]);
+
RTL_W32(MAR0 + 0, mc_filter[0]);
RTL_W32(RxConfig, tmp);
--
2.1.4
^ permalink raw reply related
* [PATCH 1/2 net-next v3.16]r8169: Disable set bit multicast enable per multicast address.
From: Corcodel Marian @ 2016-03-26 10:57 UTC (permalink / raw)
To: netdev; +Cc: Francois Romieu, Corcodel Marian
This patch correct set bit multicast enable only once per set_rx_mode
invocation.
Signed-off-by: Corcodel Marian <asd@marian1000.go.ro>
---
drivers/net/ethernet/realtek/r8169.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 7f6fb1f..f7b0dfb 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -4619,12 +4619,11 @@ static void rtl_set_rx_mode(struct net_device *dev)
} else {
struct netdev_hw_addr *ha;
- rx_mode = AcceptBroadcast | AcceptMyPhys;
+ rx_mode = AcceptBroadcast | AcceptMyPhys | AcceptMulticast;
mc_filter[1] = mc_filter[0] = 0;
netdev_for_each_mc_addr(ha, dev) {
int bit_nr = ether_crc(ETH_ALEN, ha->addr) >> 26;
mc_filter[bit_nr >> 5] |= 1 << (bit_nr & 31);
- rx_mode |= AcceptMulticast;
}
}
--
2.1.4
^ permalink raw reply related
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