* [PATCH 09/16] batman-adv: Only increase refcounter once for alternate router
From: Antonio Quartulli @ 2012-10-29 8:58 UTC (permalink / raw)
To: davem; +Cc: netdev, b.a.t.m.a.n, Sven Eckelmann, Antonio Quartulli
In-Reply-To: <1351501097-1289-1-git-send-email-ordex@autistici.org>
From: Sven Eckelmann <sven@narfation.org>
The test whether we can use a router for alternating bonding should only be
done once because it is already known that it is still usable and will not be
deleted from the list soon.
This patch addresses Coverity #712285: Unchecked return value
Signed-off-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Antonio Quartulli <ordex@autistici.org>
---
net/batman-adv/routing.c | 23 ++++++++---------------
1 file changed, 8 insertions(+), 15 deletions(-)
diff --git a/net/batman-adv/routing.c b/net/batman-adv/routing.c
index 8bdafc8..1ac072d 100644
--- a/net/batman-adv/routing.c
+++ b/net/batman-adv/routing.c
@@ -549,25 +549,18 @@ batadv_find_ifalter_router(struct batadv_orig_node *primary_orig,
if (tmp_neigh_node->if_incoming == recv_if)
continue;
+ if (router && tmp_neigh_node->tq_avg <= router->tq_avg)
+ continue;
+
if (!atomic_inc_not_zero(&tmp_neigh_node->refcount))
continue;
- /* if we don't have a router yet
- * or this one is better, choose it.
- */
- if ((!router) ||
- (tmp_neigh_node->tq_avg > router->tq_avg)) {
- /* decrement refcount of
- * previously selected router
- */
- if (router)
- batadv_neigh_node_free_ref(router);
+ /* decrement refcount of previously selected router */
+ if (router)
+ batadv_neigh_node_free_ref(router);
- router = tmp_neigh_node;
- atomic_inc_not_zero(&router->refcount);
- }
-
- batadv_neigh_node_free_ref(tmp_neigh_node);
+ /* we found a better router (or at least one valid router) */
+ router = tmp_neigh_node;
}
/* use the first candidate if nothing was found. */
--
1.7.12.4
^ permalink raw reply related
* [PATCH 10/16] batman-adv: don't allow ECTP traffic on batman-adv
From: Antonio Quartulli @ 2012-10-29 8:58 UTC (permalink / raw)
To: davem
Cc: netdev, b.a.t.m.a.n, Simon Wunderlich, Simon Wunderlich,
Antonio Quartulli
In-Reply-To: <1351501097-1289-1-git-send-email-ordex@autistici.org>
From: Simon Wunderlich <simon.wunderlich@s2003.tu-chemnitz.de>
We have seen this to break networks when used with bridge loop
avoidance. As we can't see any benefit from sending these ancient frames
via our mesh, we just drop them.
Signed-off-by: Simon Wunderlich <siwu@hrz.tu-chemnitz.de>
Signed-off-by: Antonio Quartulli <ordex@autistici.org>
---
net/batman-adv/soft-interface.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/net/batman-adv/soft-interface.c b/net/batman-adv/soft-interface.c
index 948860a..22bc651 100644
--- a/net/batman-adv/soft-interface.c
+++ b/net/batman-adv/soft-interface.c
@@ -146,8 +146,10 @@ static int batadv_interface_tx(struct sk_buff *skb,
struct batadv_bcast_packet *bcast_packet;
struct vlan_ethhdr *vhdr;
__be16 ethertype = __constant_htons(BATADV_ETH_P_BATMAN);
- static const uint8_t stp_addr[ETH_ALEN] = {0x01, 0x80, 0xC2, 0x00, 0x00,
- 0x00};
+ static const uint8_t stp_addr[ETH_ALEN] = {0x01, 0x80, 0xC2, 0x00,
+ 0x00, 0x00};
+ static const uint8_t ectp_addr[ETH_ALEN] = {0xCF, 0x00, 0x00, 0x00,
+ 0x00, 0x00};
unsigned int header_len = 0;
int data_len = skb->len, ret;
short vid __maybe_unused = -1;
@@ -180,10 +182,16 @@ static int batadv_interface_tx(struct sk_buff *skb,
/* don't accept stp packets. STP does not help in meshes.
* better use the bridge loop avoidance ...
+ *
+ * The same goes for ECTP sent at least by some Cisco Switches,
+ * it might confuse the mesh when used with bridge loop avoidance.
*/
if (batadv_compare_eth(ethhdr->h_dest, stp_addr))
goto dropped;
+ if (batadv_compare_eth(ethhdr->h_dest, ectp_addr))
+ goto dropped;
+
if (is_multicast_ether_addr(ethhdr->h_dest)) {
do_bcast = true;
--
1.7.12.4
^ permalink raw reply related
* [PATCH 11/16] batman-adv: properly store the roaming time
From: Antonio Quartulli @ 2012-10-29 8:58 UTC (permalink / raw)
To: davem; +Cc: netdev, b.a.t.m.a.n, Antonio Quartulli
In-Reply-To: <1351501097-1289-1-git-send-email-ordex@autistici.org>
in case of a new global entry added because of roaming, the roam_at field must
be properly initiated with the current time. This value will be later use to
purge this entry out on time out (if nobody claims it). Instead roam_at field
is now set to zero in this situation leading to an immediate purging of the
related entry.
Signed-off-by: Antonio Quartulli <ordex@autistici.org>
---
net/batman-adv/translation-table.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c
index 42a74e3..0ac39d5 100644
--- a/net/batman-adv/translation-table.c
+++ b/net/batman-adv/translation-table.c
@@ -726,6 +726,12 @@ int batadv_tt_global_add(struct batadv_priv *bat_priv,
common->flags = flags;
tt_global_entry->roam_at = 0;
+ /* node must store current time in case of roaming. This is
+ * needed to purge this entry out on timeout (if nobody claims
+ * it)
+ */
+ if (flags & BATADV_TT_CLIENT_ROAM)
+ tt_global_entry->roam_at = jiffies;
atomic_set(&common->refcount, 2);
common->added_at = jiffies;
--
1.7.12.4
^ permalink raw reply related
* [PATCH 13/16] batman-adv: check for more space before accessing the skb
From: Antonio Quartulli @ 2012-10-29 8:58 UTC (permalink / raw)
To: davem; +Cc: netdev, b.a.t.m.a.n, Antonio Quartulli
In-Reply-To: <1351501097-1289-1-git-send-email-ordex@autistici.org>
In batadv_check_unicast_ttvn() the code accesses both the unicast header and the
Ethernet header in the payload. For this reason pskb_may_pull() must be invoked
to check for the required space.
Signed-off-by: Antonio Quartulli <ordex@autistici.org>
---
net/batman-adv/routing.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/net/batman-adv/routing.c b/net/batman-adv/routing.c
index 456a0a9..46dd5b4 100644
--- a/net/batman-adv/routing.c
+++ b/net/batman-adv/routing.c
@@ -908,8 +908,12 @@ static int batadv_check_unicast_ttvn(struct batadv_priv *bat_priv,
bool tt_poss_change;
int is_old_ttvn;
- /* I could need to modify it */
- if (skb_cow(skb, sizeof(struct batadv_unicast_packet)) < 0)
+ /* check if there is enough data before accessing it */
+ if (pskb_may_pull(skb, sizeof(*unicast_packet) + ETH_HLEN) < 0)
+ return 0;
+
+ /* create a copy of the skb (in case of for re-routing) to modify it. */
+ if (skb_cow(skb, sizeof(*unicast_packet)) < 0)
return 0;
unicast_packet = (struct batadv_unicast_packet *)skb->data;
--
1.7.12.4
^ permalink raw reply related
* [PATCH 12/16] batman-adv: print packets re-routing on DBG_TT and ratelimit it
From: Antonio Quartulli @ 2012-10-29 8:58 UTC (permalink / raw)
To: davem; +Cc: netdev, b.a.t.m.a.n, Antonio Quartulli
In-Reply-To: <1351501097-1289-1-git-send-email-ordex@autistici.org>
To simplify TranslationTable debugging it is better to print the packet
rerouting message on the DBG_TT log level. In this way a developer interested in
packets rerouting doesn't need to filter it out of the whole ROUTES log.
Moreover, since this message will appear for each rerouted message, it is now
"ratelimited".
Signed-off-by: Antonio Quartulli <ordex@autistici.org>
---
net/batman-adv/routing.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/net/batman-adv/routing.c b/net/batman-adv/routing.c
index 1ac072d..456a0a9 100644
--- a/net/batman-adv/routing.c
+++ b/net/batman-adv/routing.c
@@ -965,10 +965,10 @@ static int batadv_check_unicast_ttvn(struct batadv_priv *bat_priv,
batadv_orig_node_free_ref(orig_node);
}
- batadv_dbg(BATADV_DBG_ROUTES, bat_priv,
- "TTVN mismatch (old_ttvn %u new_ttvn %u)! Rerouting unicast packet (for %pM) to %pM\n",
- unicast_packet->ttvn, curr_ttvn, ethhdr->h_dest,
- unicast_packet->dest);
+ net_ratelimited_function(batadv_dbg, BATADV_DBG_TT, bat_priv,
+ "TTVN mismatch (old_ttvn %u new_ttvn %u)! Rerouting unicast packet (for %pM) to %pM\n",
+ unicast_packet->ttvn, curr_ttvn,
+ ethhdr->h_dest, unicast_packet->dest);
unicast_packet->ttvn = curr_ttvn;
}
--
1.7.12.4
^ permalink raw reply related
* [PATCH 15/16] batman-adv: pass the WIFI flag from the local to global entry
From: Antonio Quartulli @ 2012-10-29 8:58 UTC (permalink / raw)
To: davem; +Cc: netdev, b.a.t.m.a.n, Antonio Quartulli
In-Reply-To: <1351501097-1289-1-git-send-email-ordex@autistici.org>
in case of client roaming a new global entry is added while a corresponding
local one is still present. In this case the node can safely pass the WIFI flag
from the local to the global entry.
This change is required to let the AP-isolation correctly working in case of
roaming: if a generic WIFI client C roams from node A to B, A adds a global
entry for C without adding any WIFI flag. The latter will be set only later,
once A has received C's advertisement from B. In this time period the
AP-Isolation (if enabled) would not correctly work since C is not marked as
WIFI, so allowing it to communicate with other WIFI clients.
Signed-off-by: Antonio Quartulli <ordex@autistici.org>
---
net/batman-adv/translation-table.c | 31 +++++++++++++++++++++++++------
net/batman-adv/translation-table.h | 6 +++---
2 files changed, 28 insertions(+), 9 deletions(-)
diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c
index c61209f..a570d95 100644
--- a/net/batman-adv/translation-table.c
+++ b/net/batman-adv/translation-table.c
@@ -489,24 +489,39 @@ batadv_tt_local_set_pending(struct batadv_priv *bat_priv,
tt_local_entry->common.addr, message);
}
-void batadv_tt_local_remove(struct batadv_priv *bat_priv, const uint8_t *addr,
- const char *message, bool roaming)
+/**
+ * batadv_tt_local_remove - logically remove an entry from the local table
+ * @bat_priv: the bat priv with all the soft interface information
+ * @addr: the MAC address of the client to remove
+ * @message: message to append to the log on deletion
+ * @roaming: true if the deletion is due to a roaming event
+ *
+ * Returns the flags assigned to the local entry before being deleted
+ */
+uint16_t batadv_tt_local_remove(struct batadv_priv *bat_priv,
+ const uint8_t *addr, const char *message,
+ bool roaming)
{
struct batadv_tt_local_entry *tt_local_entry = NULL;
- uint16_t flags;
+ uint16_t flags, curr_flags = BATADV_NO_FLAGS;
tt_local_entry = batadv_tt_local_hash_find(bat_priv, addr);
if (!tt_local_entry)
goto out;
+ curr_flags = tt_local_entry->common.flags;
+
flags = BATADV_TT_CLIENT_DEL;
if (roaming)
flags |= BATADV_TT_CLIENT_ROAM;
batadv_tt_local_set_pending(bat_priv, tt_local_entry, flags, message);
+
out:
if (tt_local_entry)
batadv_tt_local_entry_free_ref(tt_local_entry);
+
+ return curr_flags;
}
static void batadv_tt_local_purge_list(struct batadv_priv *bat_priv,
@@ -713,6 +728,7 @@ int batadv_tt_global_add(struct batadv_priv *bat_priv,
int ret = 0;
int hash_added;
struct batadv_tt_common_entry *common;
+ uint16_t local_flags;
tt_global_entry = batadv_tt_global_hash_find(bat_priv, tt_addr);
@@ -785,10 +801,13 @@ int batadv_tt_global_add(struct batadv_priv *bat_priv,
ret = 1;
out_remove:
+
/* remove address from local hash if present */
- batadv_tt_local_remove(bat_priv, tt_global_entry->common.addr,
- "global tt received",
- flags & BATADV_TT_CLIENT_ROAM);
+ local_flags = batadv_tt_local_remove(bat_priv, tt_addr,
+ "global tt received",
+ flags & BATADV_TT_CLIENT_ROAM);
+ tt_global_entry->common.flags |= local_flags & BATADV_TT_CLIENT_WIFI;
+
out:
if (tt_global_entry)
batadv_tt_global_entry_free_ref(tt_global_entry);
diff --git a/net/batman-adv/translation-table.h b/net/batman-adv/translation-table.h
index 811fffd..9fa4fe4 100644
--- a/net/batman-adv/translation-table.h
+++ b/net/batman-adv/translation-table.h
@@ -24,9 +24,9 @@ int batadv_tt_len(int changes_num);
int batadv_tt_init(struct batadv_priv *bat_priv);
void batadv_tt_local_add(struct net_device *soft_iface, const uint8_t *addr,
int ifindex);
-void batadv_tt_local_remove(struct batadv_priv *bat_priv,
- const uint8_t *addr, const char *message,
- bool roaming);
+uint16_t batadv_tt_local_remove(struct batadv_priv *bat_priv,
+ const uint8_t *addr, const char *message,
+ bool roaming);
int batadv_tt_local_seq_print_text(struct seq_file *seq, void *offset);
void batadv_tt_global_add_orig(struct batadv_priv *bat_priv,
struct batadv_orig_node *orig_node,
--
1.7.12.4
^ permalink raw reply related
* [PATCH 14/16] batman-adv: properly convert flag into a boolean value
From: Antonio Quartulli @ 2012-10-29 8:58 UTC (permalink / raw)
To: davem; +Cc: netdev, b.a.t.m.a.n, Antonio Quartulli
In-Reply-To: <1351501097-1289-1-git-send-email-ordex@autistici.org>
In order to properly convert a bitwise AND to a boolean value, the whole
expression must be prepended by "!!".
Signed-off-by: Antonio Quartulli <ordex@autistici.org>
---
net/batman-adv/translation-table.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c
index 0ac39d5..c61209f 100644
--- a/net/batman-adv/translation-table.c
+++ b/net/batman-adv/translation-table.c
@@ -2420,7 +2420,7 @@ bool batadv_tt_global_client_is_roaming(struct batadv_priv *bat_priv,
if (!tt_global_entry)
goto out;
- ret = tt_global_entry->common.flags & BATADV_TT_CLIENT_ROAM;
+ ret = !!(tt_global_entry->common.flags & BATADV_TT_CLIENT_ROAM);
batadv_tt_global_entry_free_ref(tt_global_entry);
out:
return ret;
--
1.7.12.4
^ permalink raw reply related
* [PATCH 16/16] batman-adv: add kernel-doc for enum batadv_dbg_level
From: Antonio Quartulli @ 2012-10-29 8:58 UTC (permalink / raw)
To: davem; +Cc: netdev, b.a.t.m.a.n, Antonio Quartulli
In-Reply-To: <1351501097-1289-1-git-send-email-ordex@autistici.org>
Signed-off-by: Antonio Quartulli <ordex@autistici.org>
---
net/batman-adv/main.h | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/net/batman-adv/main.h b/net/batman-adv/main.h
index 9b94f05..897ba6a 100644
--- a/net/batman-adv/main.h
+++ b/net/batman-adv/main.h
@@ -165,12 +165,19 @@ int batadv_algo_register(struct batadv_algo_ops *bat_algo_ops);
int batadv_algo_select(struct batadv_priv *bat_priv, char *name);
int batadv_algo_seq_print_text(struct seq_file *seq, void *offset);
-/* all messages related to routing / flooding / broadcasting / etc */
+/**
+ * enum batadv_dbg_level - available log levels
+ * @BATADV_DBG_BATMAN: OGM and TQ computations related messages
+ * @BATADV_DBG_ROUTES: route added / changed / deleted
+ * @BATADV_DBG_TT: translation table messages
+ * @BATADV_DBG_BLA: bridge loop avoidance messages
+ * @BATADV_DBG_ALL: the union of all the above log levels
+ */
enum batadv_dbg_level {
BATADV_DBG_BATMAN = BIT(0),
- BATADV_DBG_ROUTES = BIT(1), /* route added / changed / deleted */
- BATADV_DBG_TT = BIT(2), /* translation table operations */
- BATADV_DBG_BLA = BIT(3), /* bridge loop avoidance */
+ BATADV_DBG_ROUTES = BIT(1),
+ BATADV_DBG_TT = BIT(2),
+ BATADV_DBG_BLA = BIT(3),
BATADV_DBG_ALL = 15,
};
--
1.7.12.4
^ permalink raw reply related
* Re: [Patch net-next] ipv6: remove another useless NULL check
From: Cong Wang @ 2012-10-29 9:08 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, David S. Miller
In-Reply-To: <1351499588.7394.198.camel@edumazet-glaptop>
On Mon, 2012-10-29 at 09:33 +0100, Eric Dumazet wrote:
> On Mon, 2012-10-29 at 16:16 +0800, Cong Wang wrote:
> > When 'rt' is NULL, '&rt->dst' is NULL too because >dst
> > is always the first field of 'rt'. And dst_release accepts
> > NULL.
> >
> > Cc: Eric Dumazet <eric.dumazet@gmail.com>
> > Cc: David S. Miller <davem@davemloft.net>
> > Signed-off-by: Cong Wang <amwang@redhat.com>
> >
> > ---
> > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> > index 8f0b12a..adeb479 100644
> > --- a/net/ipv6/addrconf.c
> > +++ b/net/ipv6/addrconf.c
> > @@ -2027,8 +2027,7 @@ void addrconf_prefix_rcv(struct net_device *dev, u8 *opt, int len, bool sllao)
> > addrconf_prefix_route(&pinfo->prefix, pinfo->prefix_len,
> > dev, expires, flags);
> > }
> > - if (rt)
> > - dst_release(&rt->dst);
> > + dst_release(&rt->dst);
> > }
> >
> > /* Try to figure out our local address for this prefix */
>
>
>
> Could you instead introduce a helper for that, like we have in IPv4.
Sure, that is better.
^ permalink raw reply
* Re: [PATCH net-next] ipv4: avoid a test in ip_rt_put()
From: Cong Wang @ 2012-10-29 9:09 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev
In-Reply-To: <1351499603.7394.199.camel@edumazet-glaptop>
On Mon, 2012-10-29 at 09:33 +0100, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> We can save a test in ip_rt_put(), considering dst_release() accepts
> a NULL parameter, and dst is first element in rtable.
>
> Add a BUILD_BUG_ON() to catch any change that could break this
> assertion.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Cong Wang <amwang@redhat.com>
Acked-by: Cong Wang <amwang@redhat.com>
Thanks!
^ permalink raw reply
* [PATCH] bonding: fix bond-6-mode change MAC of arp reply from vif to cause Domu's network unreachable intermittently
From: zheng.li @ 2012-10-29 9:11 UTC (permalink / raw)
To: netdev, Jay Vosburgh, Andy Gospodarek
Cc: linux-kernel, David S. Miller, Joe Jin, zheng.x.li
This is a fix for a bug in bond_alb.c
Rate of reproduced:100%
Scenario: set Dom0 to bond 6 mode, Domu communicate with Dom0 through
vif which is in bridge mode. The Dom0's bridge of xenbr0 contains vif
and bond0, bond0 contains eth0 and eth1. You can just need to ping a
host which is in same LAN on
Domu, some of packets will be lost intermittently.
Analyse: When Dom0 set bond mode to 6(alb), the bond_alb will change MAC
of every arp reply in rlb_arp_xmit function to affect receive packets,
it is ok for normal NIC, but it's wrong to Domu, when Domu send arp
reply through vif of Dom0, bond of alb replace Domu's MAC in arp reply
with NIC's MAC address, that will cause remote host send packets to Domu
using real NIC's MAC instead of Domu's MAC. Domu
can't receive the packets whose dst MAC is not Domu's MAC.
Signed-off-by: Zheng Li <zheng.x.li@oracle.com>
Cc: Jay Vosburgh <fubar@us.ibm.com>
Cc: Andy Gospodarek <andy@greyhouse.net>
Cc: "David S. Miller" <davem@davemloft.net>
---
drivers/net/bonding/bond_alb.c | 13 ++++++++++++-
1 files changed, 12 insertions(+), 1 deletions(-)
diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index e15cc11..d6b134a 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -700,7 +700,18 @@ static struct slave *rlb_arp_xmit(struct sk_buff
*skb, struct bonding *bond)
*/
tx_slave = rlb_choose_channel(skb, bond);
if (tx_slave) {
- memcpy(arp->mac_src,tx_slave->dev->dev_addr, ETH_ALEN);
+ struct slave *tmp_slave = NULL;
+ int i = 0, found_mac = 0;
+ bond_for_each_slave(bond, tmp_slave, i) {
+ if (ether_addr_equal_64bits(arp->mac_src,
+ tmp_slave->dev->dev_addr)) {
+ found_mac = 1;
+ break;
+ }
+ }
+ if (found_mac)
+ memcpy(arp->mac_src, tx_slave->dev->dev_addr,
+ ETH_ALEN);
}
pr_debug("Server sent ARP Reply packet\n");
} else if (arp->op_code == htons(ARPOP_REQUEST)) {
--
1.7.6.5
^ permalink raw reply related
* [PATCH net-next] ipv6: remove useless test in dev_forward_change()
From: Nicolas Dichtel @ 2012-10-29 9:24 UTC (permalink / raw)
To: fengguang.wu; +Cc: netdev, davem, Nicolas Dichtel
In-Reply-To: <20121029015709.GA18567@localhost>
idev->dev cannot be NULL.
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
net/ipv6/addrconf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 8f0b12a..c9b1cf3 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -607,7 +607,7 @@ static void dev_forward_change(struct inet6_dev *idev)
dev = idev->dev;
if (idev->cnf.forwarding)
dev_disable_lro(dev);
- if (dev && (dev->flags & IFF_MULTICAST)) {
+ if (dev->flags & IFF_MULTICAST) {
if (idev->cnf.forwarding)
ipv6_dev_mc_inc(dev, &in6addr_linklocal_allrouters);
else
--
1.7.12
^ permalink raw reply related
* Re: [PATCH net-next] tcp: better retrans tracking for defer-accept
From: Julian Anastasov @ 2012-10-29 9:21 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Miller, Vijay Subramanian, netdev, ncardwell,
Venkat Venkatsubra, Elliott Hughes, Yuchung Cheng
In-Reply-To: <1351454568.30380.630.camel@edumazet-glaptop>
Hello,
On Sun, 28 Oct 2012, Eric Dumazet wrote:
> On Sun, 2012-10-28 at 18:51 +0200, Julian Anastasov wrote:
>
> > The idea is:
> >
> > - expire request_sock as before, based on num_timeout with
> > the idea to catch many SYN retransmissions and to reduce
> > SYN-ACK rate from 2*SYN_rate to 1*SYN_rate, up to
> > tcp_synack_retries SYN-ACKs
> >
> > - num_retrans accounts sent SYN-ACKs, they can be sent in
> > response to SYN retr or from timer. If num_retrans increases
> > faster than num_timeout it means client uses lower
> > TCP_TIMEOUT_INIT value and sending SYN-ACKs from
> > tcp_check_req is enough because we apply tcp_synack_retries
> > once as a SYN-ACK limit and second time as expiration
> > period.
> >
> > - If we get 10 SYNs in 1 second, we will send 5 SYN-ACKs
> > immediately (will be restricted in tcp_check_req), from
> > second +1 to +31 we will not send SYN-ACKs if
> > tcp_synack_retries is reached, we will wait for ACK and
> > for more SYNs to drop, silently. Finally, at +63 we expire
> > the request_sock. inet_csk_reqsk_queue_prune still
> > can reduce the expiration period (thresh value) under load.
> >
> > Of course, this is material for separate patch,
> > if idea is liked at all.
> >
> > Regards
>
> On a SYNFLOOD attack, we end up sending one SYNACK per SYN message
> anyway ?
I assume you are referring to want_cookie set by
tcp_syn_flood_action
> If we want to address a non SYNFLOOD attack, why not resetting
> req->expire when we send a SYNACK to a retransmitted SYN ?
>
> tcp_check_req()
> ...
> if (!inet_rtx_syn_ack(sk, req)) {
> req->expire = jiffies +
> min(TCP_TIMEOUT_INIT << req->num_timeout,
> TCP_RTO_MAX);
> }
This is a good trick, only that it affects the
TCP_DEFER_ACCEPT timing, num_timeout will be increased
in different time.
But your example reveals another place where
we can optimize: time_after_eq(now, req->expires)
check in inet_csk_reqsk_queue_prune() is immune to
the thresh reduction on load, we do not touch for long
periods of time entries with num_timeout above the desired
thresh. May be we have to use:
time_after_eq(now, req->expires) ||
(req->num_timeout > thresh && !inet_rsk(req)->acked)
for immediate expire of requests before end of
current period because they already exceed the threshold.
We avoid it only for defer_accept entries with received ACK.
But such checks are not cheap if most of the time we are
not under load.
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply
* Re: [PATCH net-next] ipv6: remove useless test in dev_forward_change()
From: Cong Wang @ 2012-10-29 9:22 UTC (permalink / raw)
To: netdev
In-Reply-To: <1351502649-3876-1-git-send-email-nicolas.dichtel@6wind.com>
On Mon, 29 Oct 2012 at 09:24 GMT, Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote:
> idev->dev cannot be NULL.
>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
I sent a same patch before you:
http://marc.info/?l=linux-netdev&m=135148225303417&w=2
:)
^ permalink raw reply
* Re: # (c) 2007, Joe Perches <joe@perches.com>
From: Vaibhav Hiremath @ 2012-10-29 9:25 UTC (permalink / raw)
To: netdev; +Cc: paul, linux-omap, linux-arm-kernel
In-Reply-To: <1351498881-32482-1-git-send-email-hvaibhav@ti.com>
On 10/29/2012 1:51 PM, Vaibhav Hiremath wrote:
Ignore this patch/mail, I screwed up with my command while sending patches.
Sorry for noise.
Thanks,
Vaibhav
^ permalink raw reply
* Re: [PATCH net-next] ipv6: remove useless test in dev_forward_change()
From: Eric Dumazet @ 2012-10-29 9:31 UTC (permalink / raw)
To: Cong Wang; +Cc: netdev
In-Reply-To: <k6lhsd$jui$1@ger.gmane.org>
On Mon, 2012-10-29 at 09:22 +0000, Cong Wang wrote:
> On Mon, 29 Oct 2012 at 09:24 GMT, Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote:
> > idev->dev cannot be NULL.
> >
> > Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>
> I sent a same patch before you:
>
> http://marc.info/?l=linux-netdev&m=135148225303417&w=2
>
> :)
Yes, and it got proper attribution to Fengguang Wu
Thanks
^ permalink raw reply
* [Patch net-next] ipv6: introduce ip6_rt_put()
From: Cong Wang @ 2012-10-29 9:55 UTC (permalink / raw)
To: netdev; +Cc: Eric Dumazet, David S. Miller, Cong Wang
As suggested by Eric, we could introduce a helper function
for ipv6 too, to avoid checking if rt is NULL before
dst_release().
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <amwang@redhat.com>
---
diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index 20210d7..d1327e4 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -213,6 +213,15 @@ static inline void rt6_set_from(struct rt6_info *rt, struct rt6_info *from)
dst_hold(new);
}
+static inline void ip6_rt_put(struct rt6_info *rt)
+{
+ /* dst_release() accepts a NULL parameter.
+ * We rely on dst being first structure in struct rt6_info
+ */
+ BUILD_BUG_ON(offsetof(struct rt6_info, dst) != 0);
+ dst_release(&rt->dst);
+}
+
struct fib6_walker_t {
struct list_head lh;
struct fib6_node *root, *node;
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 8f0b12a..690a6ab 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -699,7 +699,7 @@ void inet6_ifa_finish_destroy(struct inet6_ifaddr *ifp)
pr_warn("Freeing alive inet6 address %p\n", ifp);
return;
}
- dst_release(&ifp->rt->dst);
+ ip6_rt_put(&ifp->rt);
kfree_rcu(ifp, rcu);
}
@@ -951,7 +951,7 @@ static void ipv6_del_addr(struct inet6_ifaddr *ifp)
rt6_set_expires(rt, expires);
}
}
- dst_release(&rt->dst);
+ ip6_rt_put(rt);
}
/* clean up prefsrc entries */
@@ -2027,8 +2027,7 @@ void addrconf_prefix_rcv(struct net_device *dev, u8 *opt, int len, bool sllao)
addrconf_prefix_route(&pinfo->prefix, pinfo->prefix_len,
dev, expires, flags);
}
- if (rt)
- dst_release(&rt->dst);
+ ip6_rt_put(rt);
}
/* Try to figure out our local address for this prefix */
diff --git a/net/ipv6/fib6_rules.c b/net/ipv6/fib6_rules.c
index d9fb911..2e1a432 100644
--- a/net/ipv6/fib6_rules.c
+++ b/net/ipv6/fib6_rules.c
@@ -100,7 +100,7 @@ static int fib6_rule_action(struct fib_rule *rule, struct flowi *flp,
goto out;
}
again:
- dst_release(&rt->dst);
+ ip6_rt_put(rt);
rt = NULL;
goto out;
diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
index 92f8e48..b19ed51 100644
--- a/net/ipv6/mcast.c
+++ b/net/ipv6/mcast.c
@@ -163,7 +163,7 @@ int ipv6_sock_mc_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
rt = rt6_lookup(net, addr, NULL, 0, 0);
if (rt) {
dev = rt->dst.dev;
- dst_release(&rt->dst);
+ ip6_rt_put(rt);
}
} else
dev = dev_get_by_index_rcu(net, ifindex);
@@ -260,7 +260,7 @@ static struct inet6_dev *ip6_mc_find_dev_rcu(struct net *net,
if (rt) {
dev = rt->dst.dev;
- dst_release(&rt->dst);
+ ip6_rt_put(rt);
}
} else
dev = dev_get_by_index_rcu(net, ifindex);
diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index ff36194..ae0cf81 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -1145,7 +1145,7 @@ static void ndisc_router_discovery(struct sk_buff *skb)
ND_PRINTK(0, err,
"RA: %s got default router without neighbour\n",
__func__);
- dst_release(&rt->dst);
+ ip6_rt_put(rt);
return;
}
}
@@ -1170,7 +1170,7 @@ static void ndisc_router_discovery(struct sk_buff *skb)
ND_PRINTK(0, err,
"RA: %s got default router without neighbour\n",
__func__);
- dst_release(&rt->dst);
+ ip6_rt_put(rt);
return;
}
neigh->flags |= NTF_ROUTER;
@@ -1326,8 +1326,7 @@ skip_routeinfo:
ND_PRINTK(2, warn, "RA: invalid RA options\n");
}
out:
- if (rt)
- dst_release(&rt->dst);
+ ip6_rt_put(rt);
if (neigh)
neigh_release(neigh);
}
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index c42650c..d9badb2 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -732,7 +732,7 @@ int rt6_route_rcv(struct net_device *dev, u8 *opt, int len,
else
rt6_set_expires(rt, jiffies + HZ * lifetime);
- dst_release(&rt->dst);
+ ip6_rt_put(rt);
}
return 0;
}
@@ -948,7 +948,7 @@ restart:
else
goto out2;
- dst_release(&rt->dst);
+ ip6_rt_put(rt);
rt = nrt ? : net->ipv6.ip6_null_entry;
dst_hold(&rt->dst);
@@ -965,7 +965,7 @@ restart:
* Race condition! In the gap, when table->tb6_lock was
* released someone could insert this route. Relookup.
*/
- dst_release(&rt->dst);
+ ip6_rt_put(rt);
goto relookup;
out:
@@ -1576,7 +1576,7 @@ int ip6_route_add(struct fib6_config *cfg)
goto out;
if (dev) {
if (dev != grt->dst.dev) {
- dst_release(&grt->dst);
+ ip6_rt_put(grt);
goto out;
}
} else {
@@ -1587,7 +1587,7 @@ int ip6_route_add(struct fib6_config *cfg)
}
if (!(grt->rt6i_flags & RTF_GATEWAY))
err = 0;
- dst_release(&grt->dst);
+ ip6_rt_put(grt);
if (err)
goto out;
@@ -1673,7 +1673,7 @@ static int __ip6_del_rt(struct rt6_info *rt, struct nl_info *info)
write_unlock_bh(&table->tb6_lock);
out:
- dst_release(&rt->dst);
+ ip6_rt_put(rt);
return err;
}
@@ -2726,7 +2726,7 @@ static int inet6_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr* nlh, void
skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
if (!skb) {
- dst_release(&rt->dst);
+ ip6_rt_put(rt);
err = -ENOBUFS;
goto errout;
}
^ permalink raw reply related
* Re: [Patch net-next] ipv6: introduce ip6_rt_put()
From: Cong Wang @ 2012-10-29 9:59 UTC (permalink / raw)
To: netdev; +Cc: Eric Dumazet, David S. Miller
In-Reply-To: <1351504521-15003-1-git-send-email-amwang@redhat.com>
On Mon, 2012-10-29 at 17:55 +0800, Cong Wang wrote:
> - dst_release(&ifp->rt->dst);
> + ip6_rt_put(&ifp->rt);
Oops, should be ip6_rt_put(ifp->rt), don't know why gcc doesn't warn
me...
^ permalink raw reply
* Re: [PATCH net-next] ipv6: remove useless test in dev_forward_change()
From: Nicolas Dichtel @ 2012-10-29 10:07 UTC (permalink / raw)
To: davem; +Cc: fengguang.wu, netdev
In-Reply-To: <1351502649-3876-1-git-send-email-nicolas.dichtel@6wind.com>
Please ignore, the patch has already be sent by Cong Wang <amwang@redhat.com>.
Le 29/10/2012 10:24, Nicolas Dichtel a écrit :
> idev->dev cannot be NULL.
>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
> net/ipv6/addrconf.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 8f0b12a..c9b1cf3 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -607,7 +607,7 @@ static void dev_forward_change(struct inet6_dev *idev)
> dev = idev->dev;
> if (idev->cnf.forwarding)
> dev_disable_lro(dev);
> - if (dev && (dev->flags & IFF_MULTICAST)) {
> + if (dev->flags & IFF_MULTICAST) {
> if (idev->cnf.forwarding)
> ipv6_dev_mc_inc(dev, &in6addr_linklocal_allrouters);
> else
>
^ permalink raw reply
* [Patch net-next v2] ipv6: introduce ip6_rt_put()
From: Cong Wang @ 2012-10-29 10:13 UTC (permalink / raw)
To: netdev; +Cc: Eric Dumazet, David S. Miller, Cong Wang
V2:
fix a typo
more dst_release() -> ip6_rt_put()
As suggested by Eric, we could introduce a helper function
for ipv6 too, to avoid checking if rt is NULL before
dst_release().
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <amwang@redhat.com>
---
diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index 20210d7..d1327e4 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -213,6 +213,15 @@ static inline void rt6_set_from(struct rt6_info *rt, struct rt6_info *from)
dst_hold(new);
}
+static inline void ip6_rt_put(struct rt6_info *rt)
+{
+ /* dst_release() accepts a NULL parameter.
+ * We rely on dst being first structure in struct rt6_info
+ */
+ BUILD_BUG_ON(offsetof(struct rt6_info, dst) != 0);
+ dst_release(&rt->dst);
+}
+
struct fib6_walker_t {
struct list_head lh;
struct fib6_node *root, *node;
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 8f0b12a..5f46ace 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -699,7 +699,7 @@ void inet6_ifa_finish_destroy(struct inet6_ifaddr *ifp)
pr_warn("Freeing alive inet6 address %p\n", ifp);
return;
}
- dst_release(&ifp->rt->dst);
+ ip6_rt_put(ifp->rt);
kfree_rcu(ifp, rcu);
}
@@ -951,7 +951,7 @@ static void ipv6_del_addr(struct inet6_ifaddr *ifp)
rt6_set_expires(rt, expires);
}
}
- dst_release(&rt->dst);
+ ip6_rt_put(rt);
}
/* clean up prefsrc entries */
@@ -2027,8 +2027,7 @@ void addrconf_prefix_rcv(struct net_device *dev, u8 *opt, int len, bool sllao)
addrconf_prefix_route(&pinfo->prefix, pinfo->prefix_len,
dev, expires, flags);
}
- if (rt)
- dst_release(&rt->dst);
+ ip6_rt_put(rt);
}
/* Try to figure out our local address for this prefix */
diff --git a/net/ipv6/anycast.c b/net/ipv6/anycast.c
index cdf02be..4963c76 100644
--- a/net/ipv6/anycast.c
+++ b/net/ipv6/anycast.c
@@ -84,7 +84,7 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
rt = rt6_lookup(net, addr, NULL, 0, 0);
if (rt) {
dev = rt->dst.dev;
- dst_release(&rt->dst);
+ ip6_rt_put(rt);
} else if (ishost) {
err = -EADDRNOTAVAIL;
goto error;
diff --git a/net/ipv6/fib6_rules.c b/net/ipv6/fib6_rules.c
index d9fb911..2e1a432 100644
--- a/net/ipv6/fib6_rules.c
+++ b/net/ipv6/fib6_rules.c
@@ -100,7 +100,7 @@ static int fib6_rule_action(struct fib_rule *rule, struct flowi *flp,
goto out;
}
again:
- dst_release(&rt->dst);
+ ip6_rt_put(rt);
rt = NULL;
goto out;
diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index 0185679..bbe2e7b 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -1069,7 +1069,7 @@ static void ip6gre_tnl_link_config(struct ip6_tnl *t, int set_mtu)
dev->mtu = IPV6_MIN_MTU;
}
}
- dst_release(&rt->dst);
+ ip6_rt_put(rt);
}
t->hlen = addend;
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index aece3e7..9d9853c 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -756,7 +756,7 @@ int ip6_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *))
if (err == 0) {
IP6_INC_STATS(net, ip6_dst_idev(&rt->dst),
IPSTATS_MIB_FRAGOKS);
- dst_release(&rt->dst);
+ ip6_rt_put(rt);
return 0;
}
@@ -768,7 +768,7 @@ int ip6_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *))
IP6_INC_STATS(net, ip6_dst_idev(&rt->dst),
IPSTATS_MIB_FRAGFAILS);
- dst_release(&rt->dst);
+ ip6_rt_put(rt);
return err;
slow_path_clean:
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index cb7e2de..09482f7 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -663,8 +663,7 @@ ip6ip6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
icmpv6_send(skb2, rel_type, rel_code, rel_info);
- if (rt)
- dst_release(&rt->dst);
+ ip6_rt_put(rt);
kfree_skb(skb2);
}
@@ -1208,7 +1207,7 @@ static void ip6_tnl_link_config(struct ip6_tnl *t)
if (dev->mtu < IPV6_MIN_MTU)
dev->mtu = IPV6_MIN_MTU;
}
- dst_release(&rt->dst);
+ ip6_rt_put(rt);
}
}
diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
index 92f8e48..b19ed51 100644
--- a/net/ipv6/mcast.c
+++ b/net/ipv6/mcast.c
@@ -163,7 +163,7 @@ int ipv6_sock_mc_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
rt = rt6_lookup(net, addr, NULL, 0, 0);
if (rt) {
dev = rt->dst.dev;
- dst_release(&rt->dst);
+ ip6_rt_put(rt);
}
} else
dev = dev_get_by_index_rcu(net, ifindex);
@@ -260,7 +260,7 @@ static struct inet6_dev *ip6_mc_find_dev_rcu(struct net *net,
if (rt) {
dev = rt->dst.dev;
- dst_release(&rt->dst);
+ ip6_rt_put(rt);
}
} else
dev = dev_get_by_index_rcu(net, ifindex);
diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index ff36194..ae0cf81 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -1145,7 +1145,7 @@ static void ndisc_router_discovery(struct sk_buff *skb)
ND_PRINTK(0, err,
"RA: %s got default router without neighbour\n",
__func__);
- dst_release(&rt->dst);
+ ip6_rt_put(rt);
return;
}
}
@@ -1170,7 +1170,7 @@ static void ndisc_router_discovery(struct sk_buff *skb)
ND_PRINTK(0, err,
"RA: %s got default router without neighbour\n",
__func__);
- dst_release(&rt->dst);
+ ip6_rt_put(rt);
return;
}
neigh->flags |= NTF_ROUTER;
@@ -1326,8 +1326,7 @@ skip_routeinfo:
ND_PRINTK(2, warn, "RA: invalid RA options\n");
}
out:
- if (rt)
- dst_release(&rt->dst);
+ ip6_rt_put(rt);
if (neigh)
neigh_release(neigh);
}
diff --git a/net/ipv6/netfilter/ip6t_rpfilter.c b/net/ipv6/netfilter/ip6t_rpfilter.c
index 5d1d8b0..5060d54 100644
--- a/net/ipv6/netfilter/ip6t_rpfilter.c
+++ b/net/ipv6/netfilter/ip6t_rpfilter.c
@@ -67,7 +67,7 @@ static bool rpfilter_lookup_reverse6(const struct sk_buff *skb,
if (rt->rt6i_idev->dev == dev || (flags & XT_RPFILTER_LOOSE))
ret = true;
out:
- dst_release(&rt->dst);
+ ip6_rt_put(rt);
return ret;
}
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index c42650c..d9badb2 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -732,7 +732,7 @@ int rt6_route_rcv(struct net_device *dev, u8 *opt, int len,
else
rt6_set_expires(rt, jiffies + HZ * lifetime);
- dst_release(&rt->dst);
+ ip6_rt_put(rt);
}
return 0;
}
@@ -948,7 +948,7 @@ restart:
else
goto out2;
- dst_release(&rt->dst);
+ ip6_rt_put(rt);
rt = nrt ? : net->ipv6.ip6_null_entry;
dst_hold(&rt->dst);
@@ -965,7 +965,7 @@ restart:
* Race condition! In the gap, when table->tb6_lock was
* released someone could insert this route. Relookup.
*/
- dst_release(&rt->dst);
+ ip6_rt_put(rt);
goto relookup;
out:
@@ -1576,7 +1576,7 @@ int ip6_route_add(struct fib6_config *cfg)
goto out;
if (dev) {
if (dev != grt->dst.dev) {
- dst_release(&grt->dst);
+ ip6_rt_put(grt);
goto out;
}
} else {
@@ -1587,7 +1587,7 @@ int ip6_route_add(struct fib6_config *cfg)
}
if (!(grt->rt6i_flags & RTF_GATEWAY))
err = 0;
- dst_release(&grt->dst);
+ ip6_rt_put(grt);
if (err)
goto out;
@@ -1673,7 +1673,7 @@ static int __ip6_del_rt(struct rt6_info *rt, struct nl_info *info)
write_unlock_bh(&table->tb6_lock);
out:
- dst_release(&rt->dst);
+ ip6_rt_put(rt);
return err;
}
@@ -2726,7 +2726,7 @@ static int inet6_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr* nlh, void
skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
if (!skb) {
- dst_release(&rt->dst);
+ ip6_rt_put(rt);
err = -ENOBUFS;
goto errout;
}
^ permalink raw reply related
* Re: [PATCH RFC] pkt_sched: enable QFQ to support TSO/GSO
From: Cong Wang @ 2012-10-29 11:08 UTC (permalink / raw)
To: Paolo Valente
Cc: jhs, davem, shemminger, linux-kernel, netdev, rizzo, fchecconi
In-Reply-To: <20121029085106.GA3733@paolo-ThinkPad-W520>
On 10/29/2012 04:51 PM, Paolo Valente wrote:
> Hi,
> if the max packet size for some class (configured through tc) is
> violated by the actual size of the packets of that class, then QFQ
> would not schedule classes correctly, and the data structures
> implementing the bucket lists may get corrupted. This problem occurs
> with TSO/GSO even if the max packet size is set to the MTU, and is,
> e.g., the cause of the failure reported in [1]. Two patches have been
> proposed to solve this problem in [2], one of them is a preliminary
> version of this patch.
>
> This patch addresses the above issues by: 1) setting QFQ parameters to
> proper values for supporting TSO/GSO (in particular, setting the
> maximum possible packet size to 64KB), 2) automatically increasing the
> max packet size for a class, lmax, when a packet with a larger size
> than the current value of lmax arrives (a notification is also appended
> to the log).
>
> The drawback of the first point is that the maximum weight for a class
> is now limited to 4096, which is equal to 1/16 of the maximum weight
> sum.
>
> Finally, this patch also forcibly caps the timestamps of a class if
> they are too high to be stored in the bucket list. This capping, taken
> from QFQ+ [3], handles the unfrequent case described in the comment to
> the function slot_insert.
>
> [1] http://marc.info/?l=linux-netdev&m=134968777902077&w=2
> [2] http://marc.info/?l=linux-netdev&m=135096573507936&w=2
> [3] http://marc.info/?l=linux-netdev&m=134902691421670&w=2
>
> Signed-off-by: Paolo Valente <paolo.valente@unimore.it>
Please respect people who helps you to test it:
Tested-by: Cong Wang <amwang@redhat.com>
I tested it again just in case...
By the way, one nit below:
> + if (unlikely(cl->lmax < qdisc_pkt_len(skb))) {
> + pr_notice("qfq: increasing maxpkt from %u to %u for class %u",
> + cl->lmax, qdisc_pkt_len(skb), cl->common.classid);
> + qfq_update_reactivate_class(q, cl, cl->inv_w,
> + qdisc_pkt_len(skb), 0);
> + }
Seems the log level KERN_NOTICE is overkill, normal users don't care
about these information, so s/pr_notice/pr_debug/.
Thanks!
^ permalink raw reply
* Re: [PATCH RFC] pkt_sched: enable QFQ to support TSO/GSO
From: Paolo Valente @ 2012-10-29 11:24 UTC (permalink / raw)
To: Cong Wang; +Cc: jhs, davem, shemminger, linux-kernel, netdev, rizzo, fchecconi
In-Reply-To: <508E63AD.20803@gmail.com>
Il 29/10/2012 12:08, Cong Wang ha scritto:
> On 10/29/2012 04:51 PM, Paolo Valente wrote:
>> Hi,
>> if the max packet size for some class (configured through tc) is
>> violated by the actual size of the packets of that class, then QFQ
>> would not schedule classes correctly, and the data structures
>> implementing the bucket lists may get corrupted. This problem occurs
>> with TSO/GSO even if the max packet size is set to the MTU, and is,
>> e.g., the cause of the failure reported in [1]. Two patches have been
>> proposed to solve this problem in [2], one of them is a preliminary
>> version of this patch.
>>
>> This patch addresses the above issues by: 1) setting QFQ parameters to
>> proper values for supporting TSO/GSO (in particular, setting the
>> maximum possible packet size to 64KB), 2) automatically increasing the
>> max packet size for a class, lmax, when a packet with a larger size
>> than the current value of lmax arrives (a notification is also appended
>> to the log).
>>
>> The drawback of the first point is that the maximum weight for a class
>> is now limited to 4096, which is equal to 1/16 of the maximum weight
>> sum.
>>
>> Finally, this patch also forcibly caps the timestamps of a class if
>> they are too high to be stored in the bucket list. This capping, taken
>> from QFQ+ [3], handles the unfrequent case described in the comment to
>> the function slot_insert.
>>
>> [1] http://marc.info/?l=linux-netdev&m=134968777902077&w=2
>> [2] http://marc.info/?l=linux-netdev&m=135096573507936&w=2
>> [3] http://marc.info/?l=linux-netdev&m=134902691421670&w=2
>>
>> Signed-off-by: Paolo Valente <paolo.valente@unimore.it>
>
>
> Please respect people who helps you to test it:
>
> Tested-by: Cong Wang <amwang@redhat.com>
Oops, really sorry about that. I did not mean to hide your contribution.
I am just not familiar with the process.
>
> I tested it again just in case...
Thanks again
>
> By the way, one nit below:
>
>
>> + if (unlikely(cl->lmax < qdisc_pkt_len(skb))) {
>> + pr_notice("qfq: increasing maxpkt from %u to %u for class %u",
>> + cl->lmax, qdisc_pkt_len(skb), cl->common.classid);
>> + qfq_update_reactivate_class(q, cl, cl->inv_w,
>> + qdisc_pkt_len(skb), 0);
>> + }
>
>
> Seems the log level KERN_NOTICE is overkill, normal users don't care
> about these information, so s/pr_notice/pr_debug/.
>
Thanks, I will integrate this and possible other suggestions in a new
version of the patch.
>
> Thanks!
>
>
--
-----------------------------------------------------------
| Paolo Valente | |
| Algogroup | |
| Dip. Ing. Informazione | tel: +39 059 2056318 |
| Via Vignolese 905/b | fax: +39 059 2056129 |
| 41125 Modena - Italy | |
| home: http://algo.ing.unimo.it/people/paolo/ |
-----------------------------------------------------------
^ permalink raw reply
* Re: [PATCH v7 01/16] hashtable: introduce a small and naive hashtable
From: Mathieu Desnoyers @ 2012-10-29 11:29 UTC (permalink / raw)
To: Sasha Levin
Cc: torvalds, tj, akpm, linux-kernel, linux-mm, paul.gortmaker, davem,
rostedt, mingo, ebiederm, aarcange, ericvh, netdev, josh,
eric.dumazet, axboe, agk, dm-devel, neilb, ccaulfie, teigland,
Trond.Myklebust, bfields, fweisbec, jesse, venkat.x.venkatsubra,
ejt, snitzer, edumazet, linux-nfs, dev, rds-devel, lw
In-Reply-To: <1351450948-15618-1-git-send-email-levinsasha928@gmail.com>
* Sasha Levin (levinsasha928@gmail.com) wrote:
> This hashtable implementation is using hlist buckets to provide a simple
> hashtable to prevent it from getting reimplemented all over the kernel.
>
> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> ---
>
> Sorry for the long delay, I was busy with a bunch of personal things.
>
> Changes since v6:
>
> - Use macros that point to internal static inline functions instead of
> implementing everything as a macro.
> - Rebase on latest -next.
> - Resending the enter patch series on request.
> - Break early from hash_empty() if found to be non-empty.
> - DECLARE_HASHTABLE/DEFINE_HASHTABLE.
>
>
> include/linux/hashtable.h | 193 ++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 193 insertions(+)
> create mode 100644 include/linux/hashtable.h
>
> diff --git a/include/linux/hashtable.h b/include/linux/hashtable.h
> new file mode 100644
> index 0000000..1fb8c97
> --- /dev/null
> +++ b/include/linux/hashtable.h
> @@ -0,0 +1,193 @@
> +/*
> + * Statically sized hash table implementation
> + * (C) 2012 Sasha Levin <levinsasha928@gmail.com>
> + */
> +
> +#ifndef _LINUX_HASHTABLE_H
> +#define _LINUX_HASHTABLE_H
> +
> +#include <linux/list.h>
> +#include <linux/types.h>
> +#include <linux/kernel.h>
> +#include <linux/hash.h>
> +#include <linux/rculist.h>
> +
> +#define DEFINE_HASHTABLE(name, bits) \
> + struct hlist_head name[1 << bits] = \
> + { [0 ... ((1 << bits) - 1)] = HLIST_HEAD_INIT }
Although it's unlikely that someone would use this with a binary
operator with lower precedence than "<<" (see e.g.
http://www.swansontec.com/sopc.html) as "bits", lack of parenthesis
around "bits" would be unexpected by the caller, and could introduce
bugs. Please review all macros with the precedence table in mind, and
ask yourself if lack of parenthesis could introduce a subtle bug.
> +
> +#define DECLARE_HASHTABLE(name, bits) \
> + struct hlist_head name[1 << (bits)]
Here, you have parenthesis around "bits", but not above (inconsistency).
> +
> +#define HASH_SIZE(name) (ARRAY_SIZE(name))
> +#define HASH_BITS(name) ilog2(HASH_SIZE(name))
> +
> +/* Use hash_32 when possible to allow for fast 32bit hashing in 64bit kernels. */
> +#define hash_min(val, bits) \
> +({ \
> + sizeof(val) <= 4 ? \
> + hash_32(val, bits) : \
> + hash_long(val, bits); \
> +})
> +
> +static inline void __hash_init(struct hlist_head *ht, int sz)
int -> unsigned int.
> +{
> + int i;
int -> unsigned int.
> +
> + for (i = 0; i < sz; i++)
> + INIT_HLIST_HEAD(&ht[sz]);
ouch. How did this work ? Has it been tested at all ?
sz -> i
> +}
> +
> +/**
> + * hash_init - initialize a hash table
> + * @hashtable: hashtable to be initialized
> + *
> + * Calculates the size of the hashtable from the given parameter, otherwise
> + * same as hash_init_size.
> + *
> + * This has to be a macro since HASH_BITS() will not work on pointers since
> + * it calculates the size during preprocessing.
> + */
> +#define hash_init(hashtable) __hash_init(hashtable, HASH_SIZE(hashtable))
> +
> +/**
> + * hash_add - add an object to a hashtable
> + * @hashtable: hashtable to add to
> + * @node: the &struct hlist_node of the object to be added
> + * @key: the key of the object to be added
> + */
> +#define hash_add(hashtable, node, key) \
> + hlist_add_head(node, &hashtable[hash_min(key, HASH_BITS(hashtable))]);
extra ";" at the end to remove.
> +
> +/**
> + * hash_add_rcu - add an object to a rcu enabled hashtable
> + * @hashtable: hashtable to add to
> + * @node: the &struct hlist_node of the object to be added
> + * @key: the key of the object to be added
> + */
> +#define hash_add_rcu(hashtable, node, key) \
> + hlist_add_head_rcu(node, &hashtable[hash_min(key, HASH_BITS(hashtable))]);
extra ";" at the end to remove.
> +
> +/**
> + * hash_hashed - check whether an object is in any hashtable
> + * @node: the &struct hlist_node of the object to be checked
> + */
> +#define hash_hashed(node) (!hlist_unhashed(node))
Please use a static inline for this instead of a macro.
> +
> +static inline bool __hash_empty(struct hlist_head *ht, int sz)
int -> unsigned int.
> +{
> + int i;
int -> unsigned int.
> +
> + for (i = 0; i < sz; i++)
> + if (!hlist_empty(&ht[i]))
> + return false;
> +
> + return true;
> +}
> +
> +/**
> + * hash_empty - check whether a hashtable is empty
> + * @hashtable: hashtable to check
> + *
> + * This has to be a macro since HASH_BITS() will not work on pointers since
> + * it calculates the size during preprocessing.
> + */
> +#define hash_empty(hashtable) __hash_empty(hashtable, HASH_SIZE(hashtable))
> +
> +/**
> + * hash_del - remove an object from a hashtable
> + * @node: &struct hlist_node of the object to remove
> + */
> +static inline void hash_del(struct hlist_node *node)
> +{
> + hlist_del_init(node);
> +}
> +
> +/**
> + * hash_del_rcu - remove an object from a rcu enabled hashtable
> + * @node: &struct hlist_node of the object to remove
> + */
> +static inline void hash_del_rcu(struct hlist_node *node)
> +{
> + hlist_del_init_rcu(node);
> +}
> +
> +/**
> + * hash_for_each - iterate over a hashtable
> + * @name: hashtable to iterate
> + * @bkt: integer to use as bucket loop cursor
> + * @node: the &struct list_head to use as a loop cursor for each entry
> + * @obj: the type * to use as a loop cursor for each entry
> + * @member: the name of the hlist_node within the struct
> + */
> +#define hash_for_each(name, bkt, node, obj, member) \
> + for (bkt = 0, node = NULL; node == NULL && bkt < HASH_SIZE(name); bkt++)\
if "bkt" happens to be a dereferenced pointer (unary operator '*'), we
get into a situation where "*blah" has higher precedence than "=",
higher than "<", but lower than "++". Any thoughts on fixing this ?
> + hlist_for_each_entry(obj, node, &name[bkt], member)
> +
> +/**
> + * hash_for_each_rcu - iterate over a rcu enabled hashtable
> + * @name: hashtable to iterate
> + * @bkt: integer to use as bucket loop cursor
> + * @node: the &struct list_head to use as a loop cursor for each entry
> + * @obj: the type * to use as a loop cursor for each entry
> + * @member: the name of the hlist_node within the struct
> + */
> +#define hash_for_each_rcu(name, bkt, node, obj, member) \
> + for (bkt = 0, node = NULL; node == NULL && bkt < HASH_SIZE(name); bkt++)\
Same comment as above about "bkt".
> + hlist_for_each_entry_rcu(obj, node, &name[bkt], member)
> +
> +/**
> + * hash_for_each_safe - iterate over a hashtable safe against removal of
> + * hash entry
> + * @name: hashtable to iterate
> + * @bkt: integer to use as bucket loop cursor
> + * @node: the &struct list_head to use as a loop cursor for each entry
> + * @tmp: a &struct used for temporary storage
> + * @obj: the type * to use as a loop cursor for each entry
> + * @member: the name of the hlist_node within the struct
> + */
> +#define hash_for_each_safe(name, bkt, node, tmp, obj, member) \
> + for (bkt = 0, node = NULL; node == NULL && bkt < HASH_SIZE(name); bkt++)\
Same comment as above about "bkt".
Thanks,
Mathieu
> + hlist_for_each_entry_safe(obj, node, tmp, &name[bkt], member)
> +
> +/**
> + * hash_for_each_possible - iterate over all possible objects hashing to the
> + * same bucket
> + * @name: hashtable to iterate
> + * @obj: the type * to use as a loop cursor for each entry
> + * @node: the &struct list_head to use as a loop cursor for each entry
> + * @member: the name of the hlist_node within the struct
> + * @key: the key of the objects to iterate over
> + */
> +#define hash_for_each_possible(name, obj, node, member, key) \
> + hlist_for_each_entry(obj, node, &name[hash_min(key, HASH_BITS(name))], member)
> +
> +/**
> + * hash_for_each_possible_rcu - iterate over all possible objects hashing to the
> + * same bucket in an rcu enabled hashtable
> + * in a rcu enabled hashtable
> + * @name: hashtable to iterate
> + * @obj: the type * to use as a loop cursor for each entry
> + * @node: the &struct list_head to use as a loop cursor for each entry
> + * @member: the name of the hlist_node within the struct
> + * @key: the key of the objects to iterate over
> + */
> +#define hash_for_each_possible_rcu(name, obj, node, member, key) \
> + hlist_for_each_entry_rcu(obj, node, &name[hash_min(key, HASH_BITS(name))], member)
> +
> +/**
> + * hash_for_each_possible_safe - iterate over all possible objects hashing to the
> + * same bucket safe against removals
> + * @name: hashtable to iterate
> + * @obj: the type * to use as a loop cursor for each entry
> + * @node: the &struct list_head to use as a loop cursor for each entry
> + * @tmp: a &struct used for temporary storage
> + * @member: the name of the hlist_node within the struct
> + * @key: the key of the objects to iterate over
> + */
> +#define hash_for_each_possible_safe(name, obj, node, tmp, member, key) \
> + hlist_for_each_entry_safe(obj, node, tmp, \
> + &name[hash_min(key, HASH_BITS(name))], member)
> +
> +
> +#endif
> --
> 1.7.12.4
>
--
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH RFC] pkt_sched: enable QFQ to support TSO/GSO
From: Cong Wang @ 2012-10-29 11:31 UTC (permalink / raw)
To: Paolo Valente
Cc: jhs, davem, shemminger, linux-kernel, netdev, rizzo, fchecconi
In-Reply-To: <508E6783.2080403@unimore.it>
On 10/29/2012 07:24 PM, Paolo Valente wrote:
> Il 29/10/2012 12:08, Cong Wang ha scritto:
>>
>> Please respect people who helps you to test it:
>>
>> Tested-by: Cong Wang <amwang@redhat.com>
> Oops, really sorry about that. I did not mean to hide your contribution.
> I am just not familiar with the process.
No problem. :)
> Thanks, I will integrate this and possible other suggestions in a new
> version of the patch.
While you are on it, please check your patch with
./scripts/checkpatch.pl, it complains there are some trailing
whitespaces in your patch. ;)
^ permalink raw reply
* Re: [PATCH v7 06/16] tracepoint: use new hashtable implementation
From: Mathieu Desnoyers @ 2012-10-29 11:35 UTC (permalink / raw)
To: Sasha Levin
Cc: torvalds, tj, akpm, linux-kernel, linux-mm, paul.gortmaker, davem,
rostedt, mingo, ebiederm, aarcange, ericvh, netdev, josh,
eric.dumazet, axboe, agk, dm-devel, neilb, ccaulfie, teigland,
Trond.Myklebust, bfields, fweisbec, jesse, venkat.x.venkatsubra,
ejt, snitzer, edumazet, linux-nfs, dev, rds-devel, lw
In-Reply-To: <1351450948-15618-6-git-send-email-levinsasha928@gmail.com>
* Sasha Levin (levinsasha928@gmail.com) wrote:
> Switch tracepoints to use the new hashtable implementation. This reduces the amount of
> generic unrelated code in the tracepoints.
>
> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> ---
> kernel/tracepoint.c | 27 +++++++++++----------------
> 1 file changed, 11 insertions(+), 16 deletions(-)
>
> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> index d96ba22..854df92 100644
> --- a/kernel/tracepoint.c
> +++ b/kernel/tracepoint.c
> @@ -26,6 +26,7 @@
> #include <linux/slab.h>
> #include <linux/sched.h>
> #include <linux/static_key.h>
> +#include <linux/hashtable.h>
>
> extern struct tracepoint * const __start___tracepoints_ptrs[];
> extern struct tracepoint * const __stop___tracepoints_ptrs[];
> @@ -49,8 +50,7 @@ static LIST_HEAD(tracepoint_module_list);
> * Protected by tracepoints_mutex.
> */
> #define TRACEPOINT_HASH_BITS 6
> -#define TRACEPOINT_TABLE_SIZE (1 << TRACEPOINT_HASH_BITS)
> -static struct hlist_head tracepoint_table[TRACEPOINT_TABLE_SIZE];
> +static DEFINE_HASHTABLE(tracepoint_table, TRACEPOINT_HASH_BITS);
>
[...]
>
> @@ -722,6 +715,8 @@ struct notifier_block tracepoint_module_nb = {
>
> static int init_tracepoints(void)
> {
> + hash_init(tracepoint_table);
> +
> return register_module_notifier(&tracepoint_module_nb);
> }
> __initcall(init_tracepoints);
So we have a hash table defined in .bss (therefore entirely initialized
to NULL), and you add a call to "hash_init", which iterates on the whole
array and initialize it to NULL (again) ?
This extra initialization is redundant. I think it should be removed
from here, and hashtable.h should document that hash_init() don't need
to be called on zeroed memory (which includes static/global variables,
kzalloc'd memory, etc).
Thanks,
Mathieu
--
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox