* [PATCH v4 2/5] alx: add constants for the stats fields
From: Sabrina Dubroca @ 2014-01-09 9:09 UTC (permalink / raw)
To: davem; +Cc: bhutchings, johannes, stephen, netdev, Sabrina Dubroca
In-Reply-To: <1389258571-10083-1-git-send-email-sd@queasysnail.net>
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
drivers/net/ethernet/atheros/alx/reg.h | 52 +++++++++++++++++++++++++++++++---
1 file changed, 48 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/atheros/alx/reg.h b/drivers/net/ethernet/atheros/alx/reg.h
index e4358c9..af006b4 100644
--- a/drivers/net/ethernet/atheros/alx/reg.h
+++ b/drivers/net/ethernet/atheros/alx/reg.h
@@ -404,15 +404,59 @@
/* MIB */
#define ALX_MIB_BASE 0x1700
+
#define ALX_MIB_RX_OK (ALX_MIB_BASE + 0)
+#define ALX_MIB_RX_BCAST (ALX_MIB_BASE + 4)
+#define ALX_MIB_RX_MCAST (ALX_MIB_BASE + 8)
+#define ALX_MIB_RX_PAUSE (ALX_MIB_BASE + 12)
+#define ALX_MIB_RX_CTRL (ALX_MIB_BASE + 16)
+#define ALX_MIB_RX_FCS_ERR (ALX_MIB_BASE + 20)
+#define ALX_MIB_RX_LEN_ERR (ALX_MIB_BASE + 24)
+#define ALX_MIB_RX_BYTE_CNT (ALX_MIB_BASE + 28)
+#define ALX_MIB_RX_RUNT (ALX_MIB_BASE + 32)
+#define ALX_MIB_RX_FRAG (ALX_MIB_BASE + 36)
+#define ALX_MIB_RX_SZ_64B (ALX_MIB_BASE + 40)
+#define ALX_MIB_RX_SZ_127B (ALX_MIB_BASE + 44)
+#define ALX_MIB_RX_SZ_255B (ALX_MIB_BASE + 48)
+#define ALX_MIB_RX_SZ_511B (ALX_MIB_BASE + 52)
+#define ALX_MIB_RX_SZ_1023B (ALX_MIB_BASE + 56)
+#define ALX_MIB_RX_SZ_1518B (ALX_MIB_BASE + 60)
+#define ALX_MIB_RX_SZ_MAX (ALX_MIB_BASE + 64)
+#define ALX_MIB_RX_OV_SZ (ALX_MIB_BASE + 68)
+#define ALX_MIB_RX_OV_RXF (ALX_MIB_BASE + 72)
+#define ALX_MIB_RX_OV_RRD (ALX_MIB_BASE + 76)
+#define ALX_MIB_RX_ALIGN_ERR (ALX_MIB_BASE + 80)
+#define ALX_MIB_RX_BCCNT (ALX_MIB_BASE + 84)
+#define ALX_MIB_RX_MCCNT (ALX_MIB_BASE + 88)
#define ALX_MIB_RX_ERRADDR (ALX_MIB_BASE + 92)
+
#define ALX_MIB_TX_OK (ALX_MIB_BASE + 96)
+#define ALX_MIB_TX_BCAST (ALX_MIB_BASE + 100)
+#define ALX_MIB_TX_MCAST (ALX_MIB_BASE + 104)
+#define ALX_MIB_TX_PAUSE (ALX_MIB_BASE + 108)
+#define ALX_MIB_TX_EXC_DEFER (ALX_MIB_BASE + 112)
+#define ALX_MIB_TX_CTRL (ALX_MIB_BASE + 116)
+#define ALX_MIB_TX_DEFER (ALX_MIB_BASE + 120)
+#define ALX_MIB_TX_BYTE_CNT (ALX_MIB_BASE + 124)
+#define ALX_MIB_TX_SZ_64B (ALX_MIB_BASE + 128)
+#define ALX_MIB_TX_SZ_127B (ALX_MIB_BASE + 132)
+#define ALX_MIB_TX_SZ_255B (ALX_MIB_BASE + 136)
+#define ALX_MIB_TX_SZ_511B (ALX_MIB_BASE + 140)
+#define ALX_MIB_TX_SZ_1023B (ALX_MIB_BASE + 144)
+#define ALX_MIB_TX_SZ_1518B (ALX_MIB_BASE + 148)
+#define ALX_MIB_TX_SZ_MAX (ALX_MIB_BASE + 152)
+#define ALX_MIB_TX_SINGLE_COL (ALX_MIB_BASE + 156)
+#define ALX_MIB_TX_MULTI_COL (ALX_MIB_BASE + 160)
+#define ALX_MIB_TX_LATE_COL (ALX_MIB_BASE + 164)
+#define ALX_MIB_TX_ABORT_COL (ALX_MIB_BASE + 168)
+#define ALX_MIB_TX_UNDERRUN (ALX_MIB_BASE + 172)
+#define ALX_MIB_TX_TRD_EOP (ALX_MIB_BASE + 176)
+#define ALX_MIB_TX_LEN_ERR (ALX_MIB_BASE + 180)
+#define ALX_MIB_TX_TRUNC (ALX_MIB_BASE + 184)
+#define ALX_MIB_TX_BCCNT (ALX_MIB_BASE + 188)
#define ALX_MIB_TX_MCCNT (ALX_MIB_BASE + 192)
+#define ALX_MIB_UPDATE (ALX_MIB_BASE + 196)
-#define ALX_RX_STATS_BIN ALX_MIB_RX_OK
-#define ALX_RX_STATS_END ALX_MIB_RX_ERRADDR
-#define ALX_TX_STATS_BIN ALX_MIB_TX_OK
-#define ALX_TX_STATS_END ALX_MIB_TX_MCCNT
#define ALX_ISR 0x1600
#define ALX_ISR_DIS BIT(31)
--
1.8.5.2
^ permalink raw reply related
* [PATCH v4 1/5] alx: add a hardware stats structure
From: Sabrina Dubroca @ 2014-01-09 9:09 UTC (permalink / raw)
To: davem; +Cc: bhutchings, johannes, stephen, netdev, Sabrina Dubroca
In-Reply-To: <1389258571-10083-1-git-send-email-sd@queasysnail.net>
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
drivers/net/ethernet/atheros/alx/hw.h | 62 +++++++++++++++++++++++++++++++++++
1 file changed, 62 insertions(+)
diff --git a/drivers/net/ethernet/atheros/alx/hw.h b/drivers/net/ethernet/atheros/alx/hw.h
index 96f3b43..872b362 100644
--- a/drivers/net/ethernet/atheros/alx/hw.h
+++ b/drivers/net/ethernet/atheros/alx/hw.h
@@ -381,6 +381,68 @@ struct alx_rrd {
ALX_ISR_RX_Q6 | \
ALX_ISR_RX_Q7)
+/* Statistics counters collected by the MAC */
+struct alx_hw_stats {
+ /* rx */
+ u64 rx_ok; /* good RX packets */
+ u64 rx_bcast; /* good RX broadcast packets */
+ u64 rx_mcast; /* good RX multicast packets */
+ u64 rx_pause; /* RX pause frames */
+ u64 rx_ctrl; /* RX control packets other than pause frames */
+ u64 rx_fcs_err; /* RX packets with bad FCS */
+ u64 rx_len_err; /* RX packets with length != actual size */
+ u64 rx_byte_cnt; /* good bytes received. FCS is NOT included */
+ u64 rx_runt; /* RX packets < 64 bytes with good FCS */
+ u64 rx_frag; /* RX packets < 64 bytes with bad FCS */
+ u64 rx_sz_64B; /* 64 byte RX packets */
+ u64 rx_sz_127B; /* 65-127 byte RX packets */
+ u64 rx_sz_255B; /* 128-255 byte RX packets */
+ u64 rx_sz_511B; /* 256-511 byte RX packets */
+ u64 rx_sz_1023B; /* 512-1023 byte RX packets */
+ u64 rx_sz_1518B; /* 1024-1518 byte RX packets */
+ u64 rx_sz_max; /* 1519 byte to MTU RX packets */
+ u64 rx_ov_sz; /* truncated RX packets, size > MTU */
+ u64 rx_ov_rxf; /* frames dropped due to RX FIFO overflow */
+ u64 rx_ov_rrd; /* frames dropped due to RRD overflow */
+ u64 rx_align_err; /* alignment errors */
+ u64 rx_bc_byte_cnt; /* RX broadcast bytes, excluding FCS */
+ u64 rx_mc_byte_cnt; /* RX multicast bytes, excluding FCS */
+ u64 rx_err_addr; /* packets dropped due to address filtering */
+
+ /* tx */
+ u64 tx_ok; /* good TX packets */
+ u64 tx_bcast; /* good TX broadcast packets */
+ u64 tx_mcast; /* good TX multicast packets */
+ u64 tx_pause; /* TX pause frames */
+ u64 tx_exc_defer; /* TX packets deferred excessively */
+ u64 tx_ctrl; /* TX control frames, excluding pause frames */
+ u64 tx_defer; /* TX packets deferred */
+ u64 tx_byte_cnt; /* bytes transmitted, FCS is NOT included */
+ u64 tx_sz_64B; /* 64 byte TX packets */
+ u64 tx_sz_127B; /* 65-127 byte TX packets */
+ u64 tx_sz_255B; /* 128-255 byte TX packets */
+ u64 tx_sz_511B; /* 256-511 byte TX packets */
+ u64 tx_sz_1023B; /* 512-1023 byte TX packets */
+ u64 tx_sz_1518B; /* 1024-1518 byte TX packets */
+ u64 tx_sz_max; /* 1519 byte to MTU TX packets */
+ u64 tx_single_col; /* packets TX after a single collision */
+ u64 tx_multi_col; /* packets TX after multiple collisions */
+ u64 tx_late_col; /* TX packets with late collisions */
+ u64 tx_abort_col; /* TX packets aborted w/excessive collisions */
+ u64 tx_underrun; /* TX packets aborted due to TX FIFO underrun
+ * or TRD FIFO underrun
+ */
+ u64 tx_trd_eop; /* reads beyond the EOP into the next frame
+ * when TRD was not written timely
+ */
+ u64 tx_len_err; /* TX packets where length != actual size */
+ u64 tx_trunc; /* TX packets truncated due to size > MTU */
+ u64 tx_bc_byte_cnt; /* broadcast bytes transmitted, excluding FCS */
+ u64 tx_mc_byte_cnt; /* multicast bytes transmitted, excluding FCS */
+ u64 update;
+};
+
+
/* maximum interrupt vectors for msix */
#define ALX_MAX_MSIX_INTRS 16
--
1.8.5.2
^ permalink raw reply related
* [PATCH v4 0/5] alx: add statistics
From: Sabrina Dubroca @ 2014-01-09 9:09 UTC (permalink / raw)
To: davem; +Cc: bhutchings, johannes, stephen, netdev, Sabrina Dubroca
Currently, the alx driver doesn't support statistics [1,2]. The
original alx driver [3] that Johannes Berg modified provided
statistics. This patch is an adaptation of the statistics code from
the original driver to the alx driver included in the kernel.
v4:
- modified the assignements of hw stats to netstats (Ben Hutchings)
- added comments to describe the stats fields (copied from atlx)
v3:
- renamed __alx_update_hw_stats to alx_update_hw_stats (Stephen Hemminger)
v2:
- use u64 instead of unsigned long (Ben Hutchings)
- implement ndo_get_stats64 instead of ndo_get_stats (Ben Hutchings)
- use EINVAL instead of ENOTSUPP (Ben Hutchings)
- add BUILD_BUG_ON to check the size of the stats (Johannes Berg, Ben
Hutchings)
- add a comment regarding persistence of the stats (Stephen Hemminger)
- align assignments in __alx_update_hw_stats
[1] https://bugzilla.kernel.org/show_bug.cgi?id=63401
[2] http://www.spinics.net/lists/netdev/msg245544.html
[3] https://github.com/mcgrof/alx
Sabrina Dubroca (5):
alx: add a hardware stats structure
alx: add constants for the stats fields
alx: add stats update function
alx: add alx_get_stats64 operation
alx: add stats to ethtool
drivers/net/ethernet/atheros/alx/alx.h | 3 +
drivers/net/ethernet/atheros/alx/ethtool.c | 101 +++++++++++++++++++++++++++++
drivers/net/ethernet/atheros/alx/hw.c | 58 +++++++++++++++++
drivers/net/ethernet/atheros/alx/hw.h | 71 ++++++++++++++++++++
drivers/net/ethernet/atheros/alx/main.c | 50 ++++++++++++++
drivers/net/ethernet/atheros/alx/reg.h | 52 +++++++++++++--
6 files changed, 331 insertions(+), 4 deletions(-)
--
1.8.5.2
^ permalink raw reply
* RE: [PATCH net-next 0/2] Pack struct xfrm_usersa_info and struct xfrm_userpolicy_info
From: David Laight @ 2014-01-09 9:07 UTC (permalink / raw)
To: 'Fan Du'
Cc: Steffen Klassert, davem@davemloft.net, stephen@networkplumber.org,
dev@lists.strongswan.org, netdev@vger.kernel.org
In-Reply-To: <52CE5F21.1070800@windriver.com>
> From: Fan Du
> > Packing the structures is definitely wrong. Some 32bit systems (IIRC sparc)
> > align 64bit items on 8 byte boundaries. Not to mention the expensive byte
> > by byte accesses this forces on some systems.
>
> I don't know much about sparc, if I read your message right, you mean 32bit sparc
> system also has padding even if pack attribute is supplied.
Without packing:
On 32bit x86 64bit integers are aligned on 4byte boundaries.
On 32bit sparc 64bit integers are aligned on 8byte boundaries.
If the structure is packed then, for sparc (etc), the compiler has
to generate code to read everything using byte accesses.
David
^ permalink raw reply
* Re: [PATCH net-next v2 3/4] virtio-net: auto-tune mergeable rx buffer size for improved performance
From: Michael Dalton @ 2014-01-09 9:02 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, lf-virt, Eric Dumazet, David S. Miller
In-Reply-To: <CANJ5vPJp-AwvFDtY1R7SJg6L2dkEU6xPivgKcg4f0+05JCMEfw@mail.gmail.com>
If the prior code snippet looks good to you, I'll use something like
that as a baseline for a v3 patchset. I don't think we need a stricter
alignment than 64 to express values in the range (1536 ... 4096), as
the code snippet shows, which is great for x86 4KB pages.
On other architectures that have larger page sizes > 4KB with <= 64b
cachelines, we may want to increase the alignment so that the max buffer
size will be >= PAGE_SIZE (max size allowed by skb_page_frag_refill).
If we use a minimum alignment of 128, our maximum theoretical
packet buffer length is 1536 + 127 * 128 = 17792. With 256 byte
alignment, we can express a maximum packet buffer size > 65536.
Given the above, I think we want to select the min buffer alignment
based on the PAGE_SIZE:
<= 4KB PAGE_SIZE: 64b min alignment
<= 16KB PAGE_SIZE: 128b min alignment
> 16KB PAGE_SIZE: 256b min alignment
So the prior code snippet would be relatively unchanged, except that
references to the previous minimum alignment of 64 would be replaced by
a #define'd constant derived from PAGE_SIZE as shown above.
This would guarantee that we use the minimum alignment necessary to
ensure that virtio-net can post a max size (PAGE_SIZE) buffer, and for
x86 this means we won't increase the alignment beyond the x86's current
L1_CACHE_BYTES value (64). Also, sorry I haven't had a chance to respond
yet to the debugfs feedback, I will get to that soon (just wanted to do
a further deep dive on some of the sysfs/debugfs tradeoffs).
Best,
Mike
^ permalink raw reply
* [PATCH net-next v2 3/3] ipv4: introduce hardened ip_no_pmtu_disc mode
From: hannes @ 2014-01-09 9:01 UTC (permalink / raw)
To: netdev
Cc: eric.dumazet, davem, johnwheffner, steffen.klassert, fweimer,
Hannes Frederic Sowa
In-Reply-To: <1389258077-23282-1-git-send-email-hannes@stressinduktion.org>
From: Hannes Frederic Sowa <hannes@stressinduktion.org>
This new ip_no_pmtu_disc mode only allowes fragmentation-needed errors
to be honored by protocols which do more stringent validation on the
ICMP's packet payload. This knob is useful for people who e.g. want to
run an unmodified DNS server in a namespace where they need to use pmtu
for TCP connections (as they are used for zone transfers or fallback
for requests) but don't want to use possibly spoofed UDP pmtu information.
Currently the whitelisted protocols are TCP, SCTP and DCCP as they check
if the returned packet is in the window or if the association is valid.
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: David Miller <davem@davemloft.net>
Cc: John Heffner <johnwheffner@gmail.com>
Suggested-by: Florian Weimer <fweimer@redhat.com>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
Documentation/networking/ip-sysctl.txt | 13 ++++++++++++-
include/net/protocol.h | 7 ++++++-
net/dccp/ipv4.c | 1 +
net/ipv4/af_inet.c | 1 +
net/ipv4/icmp.c | 28 ++++++++++++++++++++++++----
net/sctp/protocol.c | 1 +
6 files changed, 45 insertions(+), 6 deletions(-)
diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt
index 0d71fa9..c97932c 100644
--- a/Documentation/networking/ip-sysctl.txt
+++ b/Documentation/networking/ip-sysctl.txt
@@ -26,7 +26,18 @@ ip_no_pmtu_disc - INTEGER
discarded. Outgoing frames are handled the same as in mode 1,
implicitly setting IP_PMTUDISC_DONT on every created socket.
- Possible values: 0-2
+ Mode 3 is a hardend pmtu discover mode. The kernel will only
+ accept fragmentation-needed errors if the underlying protocol
+ can verify them besides a plain socket lookup. Current
+ protocols for which pmtu events will be honored are TCP, SCTP
+ and DCCP as they verify e.g. the sequence number or the
+ association. This mode should not be enabled globally but is
+ only intended to secure e.g. name servers in namespaces where
+ TCP path mtu must still work but path MTU information of other
+ protocols should be discarded. If enabled globally this mode
+ could break other protocols.
+
+ Possible values: 0-3
Default: FALSE
min_pmtu - INTEGER
diff --git a/include/net/protocol.h b/include/net/protocol.h
index fbf7676..0e5f866 100644
--- a/include/net/protocol.h
+++ b/include/net/protocol.h
@@ -43,7 +43,12 @@ struct net_protocol {
int (*handler)(struct sk_buff *skb);
void (*err_handler)(struct sk_buff *skb, u32 info);
unsigned int no_policy:1,
- netns_ok:1;
+ netns_ok:1,
+ /* does the protocol do more stringent
+ * icmp tag validation than simple
+ * socket lookup?
+ */
+ icmp_strict_tag_validation:1;
};
#if IS_ENABLED(CONFIG_IPV6)
diff --git a/net/dccp/ipv4.c b/net/dccp/ipv4.c
index 88299c2..22b5d81 100644
--- a/net/dccp/ipv4.c
+++ b/net/dccp/ipv4.c
@@ -989,6 +989,7 @@ static const struct net_protocol dccp_v4_protocol = {
.err_handler = dccp_v4_err,
.no_policy = 1,
.netns_ok = 1,
+ .icmp_strict_tag_validation = 1,
};
static const struct proto_ops inet_dccp_ops = {
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 6268a47..ecd2c3f 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -1545,6 +1545,7 @@ static const struct net_protocol tcp_protocol = {
.err_handler = tcp_v4_err,
.no_policy = 1,
.netns_ok = 1,
+ .icmp_strict_tag_validation = 1,
};
static const struct net_protocol udp_protocol = {
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index fb3c563..0134663 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -668,6 +668,16 @@ static void icmp_socket_deliver(struct sk_buff *skb, u32 info)
rcu_read_unlock();
}
+static bool icmp_tag_validation(int proto)
+{
+ bool ok;
+
+ rcu_read_lock();
+ ok = rcu_dereference(inet_protos[proto])->icmp_strict_tag_validation;
+ rcu_read_unlock();
+ return ok;
+}
+
/*
* Handle ICMP_DEST_UNREACH, ICMP_TIME_EXCEED, ICMP_QUENCH, and
* ICMP_PARAMETERPROB.
@@ -705,12 +715,22 @@ static void icmp_unreach(struct sk_buff *skb)
case ICMP_PORT_UNREACH:
break;
case ICMP_FRAG_NEEDED:
- if (net->ipv4.sysctl_ip_no_pmtu_disc == 2) {
- goto out;
- } else if (net->ipv4.sysctl_ip_no_pmtu_disc) {
+ /* for documentation of the ip_no_pmtu_disc
+ * values please see
+ * Documentation/networking/ip-sysctl.txt
+ */
+ switch (net->ipv4.sysctl_ip_no_pmtu_disc) {
+ default:
LIMIT_NETDEBUG(KERN_INFO pr_fmt("%pI4: fragmentation needed and DF set\n"),
&iph->daddr);
- } else {
+ break;
+ case 2:
+ goto out;
+ case 3:
+ if (!icmp_tag_validation(iph->protocol))
+ goto out;
+ /* fall through */
+ case 0:
info = ntohs(icmph->un.frag.mtu);
if (!info)
goto out;
diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index 34b7726..7c16108 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -1030,6 +1030,7 @@ static const struct net_protocol sctp_protocol = {
.err_handler = sctp_v4_err,
.no_policy = 1,
.netns_ok = 1,
+ .icmp_strict_tag_validation = 1,
};
/* IPv4 address related functions. */
--
1.8.4.2
^ permalink raw reply related
* [PATCH net-next v3 2/3] ipv6: introduce ip6_dst_mtu_forward and protect forwarding path with it
From: hannes @ 2014-01-09 9:01 UTC (permalink / raw)
To: netdev
Cc: eric.dumazet, davem, johnwheffner, steffen.klassert, fweimer,
Hannes Frederic Sowa
In-Reply-To: <1389258077-23282-1-git-send-email-hannes@stressinduktion.org>
From: Hannes Frederic Sowa <hannes@stressinduktion.org>
In the IPv6 forwarding path we are only concerend about the outgoing
interface MTU, but also respect locked MTUs on routes. Tunnel provider
or IPSEC already have to recheck and if needed send PtB notifications
to the sending host in case the data does not fit into the packet with
added headers (we only know the final header sizes there, while also
using path MTU information).
The reason for this change is, that path MTU information can be injected
into the kernel via e.g. icmp_err protocol handler without verification
of local sockets. As such, this could cause the IPv6 forwarding path to
wrongfully emit Packet-too-Big errors and drop IPv6 packets.
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: David Miller <davem@davemloft.net>
Cc: John Heffner <johnwheffner@gmail.com>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
net/ipv6/ip6_output.c | 23 ++++++++++++++++++++++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index d1de956..ef02b26 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -321,6 +321,27 @@ static inline int ip6_forward_finish(struct sk_buff *skb)
return dst_output(skb);
}
+static unsigned int ip6_dst_mtu_forward(const struct dst_entry *dst)
+{
+ unsigned int mtu;
+ struct inet6_dev *idev;
+
+ if (dst_metric_locked(dst, RTAX_MTU)) {
+ mtu = dst_metric_raw(dst, RTAX_MTU);
+ if (mtu)
+ return mtu;
+ }
+
+ mtu = IPV6_MIN_MTU;
+ rcu_read_lock();
+ idev = __in6_dev_get(dst->dev);
+ if (idev)
+ mtu = idev->cnf.mtu6;
+ rcu_read_unlock();
+
+ return mtu;
+}
+
int ip6_forward(struct sk_buff *skb)
{
struct dst_entry *dst = skb_dst(skb);
@@ -441,7 +462,7 @@ int ip6_forward(struct sk_buff *skb)
}
}
- mtu = dst_mtu(dst);
+ mtu = ip6_dst_mtu_forward(dst);
if (mtu < IPV6_MIN_MTU)
mtu = IPV6_MIN_MTU;
--
1.8.4.2
^ permalink raw reply related
* [PATCH net-next v4 1/3] ipv4: introduce ip_dst_mtu_maybe_forward and protect forwarding path against pmtu spoofing
From: hannes @ 2014-01-09 9:01 UTC (permalink / raw)
To: netdev
Cc: eric.dumazet, davem, johnwheffner, steffen.klassert, fweimer,
Hannes Frederic Sowa
In-Reply-To: <1389258077-23282-1-git-send-email-hannes@stressinduktion.org>
From: Hannes Frederic Sowa <hannes@stressinduktion.org>
While forwarding we should not use the protocol path mtu to calculate
the mtu for a forwarded packet but instead use the interface mtu.
We mark forwarded skbs in ip_forward with IPSKB_FORWARDED, which was
introduced for multicast forwarding. But as it does not conflict with
our usage in unicast code path it is perfect for reuse.
I moved the functions ip_sk_accept_pmtu, ip_sk_use_pmtu and ip_skb_dst_mtu
along with the new ip_dst_mtu_maybe_forward to net/ip.h to fix circular
dependencies because of IPSKB_FORWARDED.
Because someone might have written a software which does probe
destinations manually and expects the kernel to honour those path mtus
I introduced a new per-namespace "ip_forward_use_pmtu" knob so someone
can disable this new behaviour. We also still use mtus which are locked on a
route for forwarding.
The reason for this change is, that path mtus information can be injected
into the kernel via e.g. icmp_err protocol handler without verification
of local sockets. As such, this could cause the IPv4 forwarding path to
wrongfully emit fragmentation needed notifications or start to fragment
packets along a path.
Tunnel and ipsec output paths clear IPCB again, thus IPSKB_FORWARDED
won't be set and further fragmentation logic will use the path mtu to
determine the fragmentation size. They also recheck packet size with
help of path mtu discovery and report appropriate errors.
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: David Miller <davem@davemloft.net>
Cc: John Heffner <johnwheffner@gmail.com>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
Documentation/networking/ip-sysctl.txt | 13 +++++++++++++
include/net/ip.h | 33 +++++++++++++++++++++++++++++++++
include/net/netns/ipv4.h | 1 +
include/net/route.h | 19 +++----------------
net/ipv4/ip_forward.c | 7 +++++--
net/ipv4/ip_output.c | 8 +++++---
net/ipv4/route.c | 3 ---
net/ipv4/sysctl_net_ipv4.c | 7 +++++++
8 files changed, 67 insertions(+), 24 deletions(-)
diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt
index 7373115..0d71fa9 100644
--- a/Documentation/networking/ip-sysctl.txt
+++ b/Documentation/networking/ip-sysctl.txt
@@ -32,6 +32,19 @@ ip_no_pmtu_disc - INTEGER
min_pmtu - INTEGER
default 552 - minimum discovered Path MTU
+ip_forward_use_pmtu - BOOLEAN
+ By default we don't trust protocol path MTUs while forwarding
+ because they could be easily forged and can lead to unwanted
+ fragmentation by the router.
+ You only need to enable this if you have user-space software
+ which tries to discover path mtus by itself and depends on the
+ kernel honoring this information. This is normally not the
+ case.
+ Default: 0 (disabled)
+ Possible values:
+ 0 - disabled
+ 1 - enabled
+
route/max_size - INTEGER
Maximum number of routes allowed in the kernel. Increase
this when using large numbers of interfaces and/or routes.
diff --git a/include/net/ip.h b/include/net/ip.h
index 5356644..0dab95c 100644
--- a/include/net/ip.h
+++ b/include/net/ip.h
@@ -263,6 +263,39 @@ int ip_dont_fragment(struct sock *sk, struct dst_entry *dst)
!(dst_metric_locked(dst, RTAX_MTU)));
}
+static inline bool ip_sk_accept_pmtu(const struct sock *sk)
+{
+ return inet_sk(sk)->pmtudisc != IP_PMTUDISC_INTERFACE;
+}
+
+static inline bool ip_sk_use_pmtu(const struct sock *sk)
+{
+ return inet_sk(sk)->pmtudisc < IP_PMTUDISC_PROBE;
+}
+
+static inline unsigned int ip_dst_mtu_maybe_forward(const struct dst_entry *dst,
+ bool forwarding)
+{
+ struct net *net = dev_net(dst->dev);
+
+ if (net->ipv4.sysctl_ip_fwd_use_pmtu ||
+ dst_metric_locked(dst, RTAX_MTU) ||
+ !forwarding)
+ return dst_mtu(dst);
+
+ return min(dst->dev->mtu, IP_MAX_MTU);
+}
+
+static inline unsigned int ip_skb_dst_mtu(const struct sk_buff *skb)
+{
+ if (!skb->sk || ip_sk_use_pmtu(skb->sk)) {
+ bool forwarding = IPCB(skb)->flags & IPSKB_FORWARDED;
+ return ip_dst_mtu_maybe_forward(skb_dst(skb), forwarding);
+ } else {
+ return min(skb_dst(skb)->dev->mtu, IP_MAX_MTU);
+ }
+}
+
void __ip_select_ident(struct iphdr *iph, struct dst_entry *dst, int more);
static inline void ip_select_ident(struct sk_buff *skb, struct dst_entry *dst, struct sock *sk)
diff --git a/include/net/netns/ipv4.h b/include/net/netns/ipv4.h
index 929a668..80f500a 100644
--- a/include/net/netns/ipv4.h
+++ b/include/net/netns/ipv4.h
@@ -70,6 +70,7 @@ struct netns_ipv4 {
int sysctl_tcp_ecn;
int sysctl_ip_no_pmtu_disc;
+ int sysctl_ip_fwd_use_pmtu;
kgid_t sysctl_ping_group_range[2];
diff --git a/include/net/route.h b/include/net/route.h
index 638e3eb..9d1f423 100644
--- a/include/net/route.h
+++ b/include/net/route.h
@@ -36,6 +36,9 @@
#include <linux/cache.h>
#include <linux/security.h>
+/* IPv4 datagram length is stored into 16bit field (tot_len) */
+#define IP_MAX_MTU 0xFFFFU
+
#define RTO_ONLINK 0x01
#define RT_CONN_FLAGS(sk) (RT_TOS(inet_sk(sk)->tos) | sock_flag(sk, SOCK_LOCALROUTE))
@@ -311,20 +314,4 @@ static inline int ip4_dst_hoplimit(const struct dst_entry *dst)
return hoplimit;
}
-static inline bool ip_sk_accept_pmtu(const struct sock *sk)
-{
- return inet_sk(sk)->pmtudisc != IP_PMTUDISC_INTERFACE;
-}
-
-static inline bool ip_sk_use_pmtu(const struct sock *sk)
-{
- return inet_sk(sk)->pmtudisc < IP_PMTUDISC_PROBE;
-}
-
-static inline int ip_skb_dst_mtu(const struct sk_buff *skb)
-{
- return (!skb->sk || ip_sk_use_pmtu(skb->sk)) ?
- dst_mtu(skb_dst(skb)) : skb_dst(skb)->dev->mtu;
-}
-
#endif /* _ROUTE_H */
diff --git a/net/ipv4/ip_forward.c b/net/ipv4/ip_forward.c
index 694de3b..e9f1217 100644
--- a/net/ipv4/ip_forward.c
+++ b/net/ipv4/ip_forward.c
@@ -54,6 +54,7 @@ static int ip_forward_finish(struct sk_buff *skb)
int ip_forward(struct sk_buff *skb)
{
+ u32 mtu;
struct iphdr *iph; /* Our header */
struct rtable *rt; /* Route we use */
struct ip_options *opt = &(IPCB(skb)->opt);
@@ -88,11 +89,13 @@ int ip_forward(struct sk_buff *skb)
if (opt->is_strictroute && rt->rt_uses_gateway)
goto sr_failed;
- if (unlikely(skb->len > dst_mtu(&rt->dst) && !skb_is_gso(skb) &&
+ IPCB(skb)->flags |= IPSKB_FORWARDED;
+ mtu = ip_dst_mtu_maybe_forward(&rt->dst, true);
+ if (unlikely(skb->len > mtu && !skb_is_gso(skb) &&
(ip_hdr(skb)->frag_off & htons(IP_DF))) && !skb->local_df) {
IP_INC_STATS(dev_net(rt->dst.dev), IPSTATS_MIB_FRAGFAILS);
icmp_send(skb, ICMP_DEST_UNREACH, ICMP_FRAG_NEEDED,
- htonl(dst_mtu(&rt->dst)));
+ htonl(mtu));
goto drop;
}
diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index df18461..9a78804 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -449,6 +449,7 @@ int ip_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *))
__be16 not_last_frag;
struct rtable *rt = skb_rtable(skb);
int err = 0;
+ bool forwarding = IPCB(skb)->flags & IPSKB_FORWARDED;
dev = rt->dst.dev;
@@ -458,12 +459,13 @@ int ip_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *))
iph = ip_hdr(skb);
+ mtu = ip_dst_mtu_maybe_forward(&rt->dst, forwarding);
if (unlikely(((iph->frag_off & htons(IP_DF)) && !skb->local_df) ||
(IPCB(skb)->frag_max_size &&
- IPCB(skb)->frag_max_size > dst_mtu(&rt->dst)))) {
+ IPCB(skb)->frag_max_size > mtu))) {
IP_INC_STATS(dev_net(dev), IPSTATS_MIB_FRAGFAILS);
icmp_send(skb, ICMP_DEST_UNREACH, ICMP_FRAG_NEEDED,
- htonl(ip_skb_dst_mtu(skb)));
+ htonl(mtu));
kfree_skb(skb);
return -EMSGSIZE;
}
@@ -473,7 +475,7 @@ int ip_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *))
*/
hlen = iph->ihl * 4;
- mtu = dst_mtu(&rt->dst) - hlen; /* Size of data space */
+ mtu = mtu - hlen; /* Size of data space */
#ifdef CONFIG_BRIDGE_NETFILTER
if (skb->nf_bridge)
mtu -= nf_bridge_mtu_reduction(skb);
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index f8da282..25071b4 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -112,9 +112,6 @@
#define RT_FL_TOS(oldflp4) \
((oldflp4)->flowi4_tos & (IPTOS_RT_MASK | RTO_ONLINK))
-/* IPv4 datagram length is stored into 16bit field (tot_len) */
-#define IP_MAX_MTU 0xFFFF
-
#define RT_GC_TIMEOUT (300*HZ)
static int ip_rt_max_size;
diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
index 1d2480a..44eba05 100644
--- a/net/ipv4/sysctl_net_ipv4.c
+++ b/net/ipv4/sysctl_net_ipv4.c
@@ -831,6 +831,13 @@ static struct ctl_table ipv4_net_table[] = {
.mode = 0644,
.proc_handler = proc_dointvec
},
+ {
+ .procname = "ip_forward_use_pmtu",
+ .data = &init_net.ipv4.sysctl_ip_fwd_use_pmtu,
+ .maxlen = sizeof(int),
+ .mode = 0644,
+ .proc_handler = proc_dointvec,
+ },
{ }
};
--
1.8.4.2
^ permalink raw reply related
* [PATCH net-next v4 0/3] path mtu hardening patches
From: hannes @ 2014-01-09 9:01 UTC (permalink / raw)
To: netdev; +Cc: eric.dumazet, davem, johnwheffner, steffen.klassert, fweimer
Hi all!
After a lot of back and forth I want to propose these changes regarding
path mtu hardening and give an outline why I think this is the best way
how to proceed:
This set contains the following patches:
* ipv4: introduce ip_dst_mtu_maybe_forward and protect forwarding path against pmtu spoofing
* ipv6: introduce ip6_dst_mtu_forward and protect forwarding path with it
* ipv4: introduce hardened ip_no_pmtu_disc mode
The first one switches the forwarding path of IPv4 to use the interface
mtu by default and ignore a possible discovered path mtu. It provides
a sysctl to switch back to the original behavior (see discussion below).
The second patch does the same thing unconditionally for IPv6. I don't
provide a knob for IPv6 to switch to original behavior (please see
below).
The third patch introduces a hardened pmtu mode, where only pmtu
information are accepted where the protocol is able to do more stringent
checks on the icmp piggyback payload (please see the patch commit msg
for further details).
Why is this change necessary?
First of all, RFC 1191 4. Router specification says:
"When a router is unable to forward a datagram because it exceeds the
MTU of the next-hop network and its Don't Fragment bit is set, the
router is required to return an ICMP Destination Unreachable message
to the source of the datagram, with the Code indicating
"fragmentation needed and DF set". ..."
For some time now fragmentation has been considered problematic, e.g.:
* http://www.hpl.hp.com/techreports/Compaq-DEC/WRL-87-3.pdf
* http://tools.ietf.org/search/rfc4963
Most of them seem to agree that fragmentation should be avoided because
of efficiency, data corruption or security concerns.
Recently it was shown possible that correctly guessing IP ids could lead
to data injection on DNS packets:
<https://sites.google.com/site/hayashulman/files/fragmentation-poisoning.pdf>
While we can try to completly stop fragmentation on the end host
(this is e.g. implemented via IP_PMTUDISC_INTERFACE), we cannot stop
fragmentation completly on the forwarding path. On the end host the
application has to deal with MTUs and has to choose fallback methods
if fragmentation could be an attack vector. This is already the case for
most DNS software, where a maximum UDP packet size can be configured. But
until recently they had no control over local fragmentation and could
thus emit fragmented packets.
On the forwarding path we can just try to delay the fragmentation to
the last hop where this is really necessary. Current kernel already does
that but only because routers don't receive feedback of path mtus, these are
only send back to the end host system. But it is possible to maliciously
insert path mtu inforamtion via ICMP packets which have an icmp echo_reply
payload, because we cannot validate those notifications against local
sockets. DHCP clients which establish an any-bound RAW-socket could also
start processing unwanted fragmentation-needed packets.
Why does IPv4 has a knob to revert to old behavior while IPv6 doesn't?
IPv4 does fragmentation on the path while IPv6 does always respond with
packet-too-big errors. The interface MTU will always be greater than
the path MTU information. So we would discard packets we could actually
forward because of malicious information. After this change we would
let the hop, which really could not forward the packet, notify the host
of this problem.
IPv4 allowes fragmentation mid-path. In case someone does use a software
which tries to discover such paths and assumes that the kernel is handling
the discovered pmtu information automatically. This should be an extremly
rare case, but because I could not exclude the possibility this knob is
provided. Also this software could insert non-locked mtu information
into the kernel. We cannot distinguish that from path mtu information
currently. Premature fragmentation could solve some problems in wrongly
configured networks, thus this switch is provided.
One frag-needed packet could reduce the path mtu down to 522 bytes
(route/min_pmtu).
Misc:
IPv6 neighbor discovery could advertise mtu information for an
interface. These information update the ipv6-specific interface mtu and
thus get used by the forwarding path.
Tunnel and xfrm output path will still honour path mtu and also respond
with Packet-too-Big or fragmentation-needed errors if needed.
Changelog for all patches:
# ipv4: introduce ip_dst_mtu_maybe_forward and protect forwarding path against pmtu spoofing
v2)
* enabled ip_forward_use_pmtu by default
* reworded
v3)
* disabled ip_forward_use_pmtu by default
* reworded
v4)
* renamed ip_dst_mtu_secure to ip_dst_mtu_maybe_forward
* updated changelog accordingly
* removed unneeded !!(... & ...) double negations
# ipv6: introduce ip6_dst_mtu_forward and protect forwarding path with it
v2)
* by default we honour pmtu information
3)
* only honor interface mtu
* rewritten and simplified
* no knob to fall back to old mode any more
# ipv4: introduce hardened ip_no_pmtu_disc mode
v2)
* reworded Documentation
Diffstat:
Documentation/networking/ip-sysctl.txt | 26 +++++++++++++++++++++++++-
include/net/ip.h | 33 +++++++++++++++++++++++++++++++++
include/net/netns/ipv4.h | 1 +
include/net/protocol.h | 7 ++++++-
include/net/route.h | 19 +++----------------
net/dccp/ipv4.c | 1 +
net/ipv4/af_inet.c | 1 +
net/ipv4/icmp.c | 28 ++++++++++++++++++++++++----
net/ipv4/ip_forward.c | 7 +++++--
net/ipv4/ip_output.c | 8 +++++---
net/ipv4/route.c | 3 ---
net/ipv4/sysctl_net_ipv4.c | 7 +++++++
net/ipv6/ip6_output.c | 23 ++++++++++++++++++++++-
net/sctp/protocol.c | 1 +
14 files changed, 134 insertions(+), 31 deletions(-)
Thanks,
Hannes
^ permalink raw reply
* Re: [PATCH net 1/2] macvlan: forbid L2 fowarding offload for macvtap
From: Jason Wang @ 2014-01-09 8:55 UTC (permalink / raw)
To: Michael S. Tsirkin, John Fastabend
Cc: John Fastabend, Neil Horman, davem, netdev, linux-kernel,
Vlad Yasevich
In-Reply-To: <20140109071721.GD19559@redhat.com>
On 01/09/2014 03:17 PM, Michael S. Tsirkin wrote:
> On Wed, Jan 08, 2014 at 11:05:42AM -0800, John Fastabend wrote:
>> [...]
>>
>>>>> OK I think I'm finally putting all the pieces together thanks.
>>>>>
>>>>> Do you know why macvtap is setting dev->tx_queue_len by default? If you
>>>>> zero this then the noqueue_qdisc is used and the q->enqueue check in
>>>>> dev_queue_xmit will fail.
>>>> It was introduced in commit 8a35747a5d13b99e076b0222729e0caa48cb69b6
>>>> ("macvtap: Limit packet queue length") to limit the length of socket
>>>> receive queue of macvtap. But I'm not sure whether the qdisc is a
>>>> byproduct of this commit, maybe we can switch to use another name
>>>> instead of just reuse dev->tx_queue_length.
>>> You mean tx_queue_len really, right?
>>>
>>> Problem is tx_queue_len can be accessed using netlink sysfs or ioctl,
>>> so if someone uses these to control or check the # of packets that
>>> can be queued by device, this will break.
>>>
>>> How about adding ndo_set_tx_queue_len then?
>>>
>>> At some point we wanted to decouple queue length from tx_queue_length
>>> for tun as well, so that would be benefitial there as well.
>> On the receive side we need to limit the receive queue and the
>> dev->tx_queue_len value was used to do this.
>>
>> However on the tx side when dev->tx_queue_len is set the default
>> qdisc pfifo_fast or mq is used depending on if there is multiqueue
>> or not. Unless the user specifies some numtxqueues when creating
>> the macvtap device then it will be a single queue device and use
>> the pfifo_fast qdisc.
>>
>> This is the default behaviour users could zero txqueuelen when
>> they create the device because it is a stacked device with
>> NETIF_F_LLTX using the lower devices qdisc makes sense but this
>> would appear (from code inspection) to break macvtap_forward()?
>>
>> if (skb_queue_len(&q->sk.sk_receive_queue) >= dev->tx_queue_len)
>> goto drop;
>>
>> I'm not sure any of this is a problem other than its a bit
>> confusing to overload tx_queue_len for the rx_queue_len but the
>> precedent has been there for sometime.
> So how about ndo ops to access tx_queue_len then?
> This way we can set tx_queue_len to 0 and use some
> other field to store the rx_queue_len without users noticing.
> Along the lines of the below (it's a partial patch just
> to give you the idea):
>
> diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
> index 5b7d0e1..e526b46 100644
> --- a/net/core/dev_ioctl.c
> +++ b/net/core/dev_ioctl.c
> @@ -167,7 +167,10 @@ static int dev_ifsioc_locked(struct net *net, struct ifreq *ifr, unsigned int cm
> return 0;
>
> case SIOCGIFTXQLEN:
> - ifr->ifr_qlen = dev->tx_queue_len;
> + if (dev->netdev_ops->ndo_get_tx_queue_len)
> + ifr->ifr_qlen = dev->netdev_ops->ndo_get_tx_queue_len(dev);
> + else
> + ifr->ifr_qlen = dev->tx_queue_len;
> return 0;
>
> default:
> @@ -296,7 +299,10 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, unsigned int cmd)
> case SIOCSIFTXQLEN:
> if (ifr->ifr_qlen < 0)
> return -EINVAL;
> - dev->tx_queue_len = ifr->ifr_qlen;
> + if (dev->netdev_ops->ndo_set_tx_queue_len)
> + dev->netdev_ops->ndo_set_tx_queue_len(dev, ifr->ifr_qlen);
> + else
> + dev->tx_queue_len = ifr->ifr_qlen;
> return 0;
>
> case SIOCSIFNAME:
> diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
> index d954b56..f2fd9d5 100644
> --- a/net/core/net-sysfs.c
> +++ b/net/core/net-sysfs.c
> @@ -280,10 +280,31 @@ NETDEVICE_SHOW_RW(flags, fmt_hex);
>
> static int change_tx_queue_len(struct net_device *net, unsigned long new_len)
> {
> - net->tx_queue_len = new_len;
> + if (dev->netdev_ops->ndo_set_tx_queue_len)
> + dev->netdev_ops->ndo_set_tx_queue_len(dev, new_len);
> + else
> + dev->tx_queue_len = new_len;
> return 0;
> }
>
> +static ssize_t format_tx_queue_len(const struct net_device *net, char *buf)
> +{
> + unsigned long tx_queue_len;
> +
> + if (dev->netdev_ops->ndo_get_tx_queue_len)
> + tx_queue_len = dev->netdev_ops->ndo_get_tx_queue_len(dev);
> + else
> + tx_queue_len = dev->tx_queue_len;
> +
> + return sprintf(buf, fmt_ulong, tx_queue_len);
> +}
> +
> +static ssize_t tx_queue_len_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + return netdev_show(dev, attr, buf, format_tx_queue_len);
> +}
> +
> static ssize_t tx_queue_len_store(struct device *dev,
> struct device_attribute *attr,
> const char *buf, size_t len)
> @@ -293,7 +314,7 @@ static ssize_t tx_queue_len_store(struct device *dev,
>
> return netdev_store(dev, attr, buf, len, change_tx_queue_len);
> }
> -NETDEVICE_SHOW_RW(tx_queue_len, fmt_ulong);
> +DEVICE_ATTR_RW(tx_queue_len);
>
> static ssize_t ifalias_store(struct device *dev, struct device_attribute *attr,
> const char *buf, size_t len)
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 2a0e21d..9276e17 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -876,6 +876,7 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
> struct nlattr *attr, *af_spec;
> struct rtnl_af_ops *af_ops;
> struct net_device *upper_dev = netdev_master_upper_dev_get(dev);
> + unsigned long tx_queue_len;
>
> ASSERT_RTNL();
> nlh = nlmsg_put(skb, pid, seq, type, sizeof(*ifm), flags);
> @@ -890,8 +891,13 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
> ifm->ifi_flags = dev_get_flags(dev);
> ifm->ifi_change = change;
>
> + if (dev->netdev_ops->ndo_get_tx_queue_len)
> + tx_queue_len = dev->netdev_ops->ndo_get_tx_queue_len(dev);
> + else
> + tx_queue_len = dev->tx_queue_len;
> +
> if (nla_put_string(skb, IFLA_IFNAME, dev->name) ||
> - nla_put_u32(skb, IFLA_TXQLEN, dev->tx_queue_len) ||
> + nla_put_u32(skb, IFLA_TXQLEN, tx_queue_len) ||
> nla_put_u8(skb, IFLA_OPERSTATE,
> netif_running(dev) ? dev->operstate : IF_OPER_DOWN) ||
> nla_put_u8(skb, IFLA_LINKMODE, dev->link_mode) ||
> @@ -1453,8 +1459,14 @@ static int do_setlink(struct net_device *dev, struct ifinfomsg *ifm,
> modified = 1;
> }
>
> - if (tb[IFLA_TXQLEN])
> - dev->tx_queue_len = nla_get_u32(tb[IFLA_TXQLEN]);
> + if (tb[IFLA_TXQLEN]) {
> + u32 new_len = nla_get_u32(tb[IFLA_TXQLEN]);
> +
> + if (dev->netdev_ops->ndo_set_tx_queue_len)
> + dev->netdev_ops->ndo_set_tx_queue_len(dev, new_len);
> + else
> + dev->tx_queue_len = new_len;
> + }
>
> if (tb[IFLA_OPERSTATE])
> set_operstate(dev, nla_get_u8(tb[IFLA_OPERSTATE]));
> @@ -1692,8 +1704,14 @@ struct net_device *rtnl_create_link(struct net *net,
> if (tb[IFLA_BROADCAST])
> memcpy(dev->broadcast, nla_data(tb[IFLA_BROADCAST]),
> nla_len(tb[IFLA_BROADCAST]));
> - if (tb[IFLA_TXQLEN])
> - dev->tx_queue_len = nla_get_u32(tb[IFLA_TXQLEN]);
> + if (tb[IFLA_TXQLEN]) {
> + u32 new_len = nla_get_u32(tb[IFLA_TXQLEN]);
> +
> + if (dev->netdev_ops->ndo_set_tx_queue_len)
> + dev->netdev_ops->ndo_set_tx_queue_len(dev, new_len);
> + else
> + dev->tx_queue_len = new_len;
> + }
> if (tb[IFLA_OPERSTATE])
> set_operstate(dev, nla_get_u8(tb[IFLA_OPERSTATE]));
> if (tb[IFLA_LINKMODE])
>
> Anyone objects?
What if use do want a qdisc and want to change the its queue length for
tun/macvlan? And the the name tx_queue_length is misleading. For tun it
may make sense since it was used in transmission path. For macvtap it
was not. So maybe what we need is just a new ioctl for both tun/macvtap
and a new feature flag. If user create the device with new feature flag,
the socket receive queue length could be changed by ioctl instead of
dev->tx_queue_length. If not, the old behaviour could be kept.
>> It is a change in behaviour
>> though in net-next. Previously dev_queue_xmit() was not used so
>> the qdisc layer was never hit on the macvtap device. Now with
>> dev_queue_xmit() if the user attaches multiple macvlan queues to a
>> single qdisc queue this should still work but wont be optimal. Ideally
>> the user should create as many qdisc queues at creation time via
>> numtxqueues as macvtap queues will be used during runtime so that there
>> is a 1:1 mapping or use some heuristic to avoid cases where there
>> is a many to 1 mapping.
>>
>> From my perspective it would be OK to push this configuration/policy
>> to the management layer. After all it is the entity that knows how
>> to distribute system resources amongst the objects running over the
>> macvtap devices. The relevance for me is I defaulted the l2 offloaded
>> macvlans to single queue devices and wanted to note in net-next this
>> is the same policy used in the non-offloaded case.
>>
>> Bit long-winded there.
>>
>> Thanks,
>> John
> I think it's a real problem you are pointing out - a performance
> regression for multiqueue devices.
> If we really think no qdisc is typically the right thing to do,
> we should just make it the default I think, but I agree
> just making tx_queue_len 0 doesn't work without other changes.
>
> What do others think about extra ndo ops so devices can decouple
> the internal tx_queue_len from the userspace-visible value?
>
>
^ permalink raw reply
* [PATCH] HHF qdisc: fix jiffies-time conversion.
From: Terry Lam @ 2014-01-09 8:40 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, Nandita Dukkipati, Stephen Hemminger, Terry Lam
This is to be compatible with the use of "get_time" (i.e. default
time unit in us) in iproute2 patch for HHF as requested by Stephen.
Signed-off-by: Terry Lam <vtlam@google.com>
---
net/sched/sch_hhf.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/net/sched/sch_hhf.c b/net/sched/sch_hhf.c
index cf7f614..1cf84a9 100644
--- a/net/sched/sch_hhf.c
+++ b/net/sched/sch_hhf.c
@@ -574,18 +574,18 @@ static int hhf_change(struct Qdisc *sch, struct nlattr *opt)
q->hh_flows_limit = nla_get_u32(tb[TCA_HHF_HH_FLOWS_LIMIT]);
if (tb[TCA_HHF_RESET_TIMEOUT]) {
- u32 ms = nla_get_u32(tb[TCA_HHF_RESET_TIMEOUT]);
+ u32 us = nla_get_u32(tb[TCA_HHF_RESET_TIMEOUT]);
- q->hhf_reset_timeout = msecs_to_jiffies(ms);
+ q->hhf_reset_timeout = usecs_to_jiffies(us);
}
if (tb[TCA_HHF_ADMIT_BYTES])
q->hhf_admit_bytes = nla_get_u32(tb[TCA_HHF_ADMIT_BYTES]);
if (tb[TCA_HHF_EVICT_TIMEOUT]) {
- u32 ms = nla_get_u32(tb[TCA_HHF_EVICT_TIMEOUT]);
+ u32 us = nla_get_u32(tb[TCA_HHF_EVICT_TIMEOUT]);
- q->hhf_evict_timeout = msecs_to_jiffies(ms);
+ q->hhf_evict_timeout = usecs_to_jiffies(us);
}
qlen = sch->q.qlen;
@@ -684,10 +684,10 @@ static int hhf_dump(struct Qdisc *sch, struct sk_buff *skb)
nla_put_u32(skb, TCA_HHF_QUANTUM, q->quantum) ||
nla_put_u32(skb, TCA_HHF_HH_FLOWS_LIMIT, q->hh_flows_limit) ||
nla_put_u32(skb, TCA_HHF_RESET_TIMEOUT,
- jiffies_to_msecs(q->hhf_reset_timeout)) ||
+ jiffies_to_usecs(q->hhf_reset_timeout)) ||
nla_put_u32(skb, TCA_HHF_ADMIT_BYTES, q->hhf_admit_bytes) ||
nla_put_u32(skb, TCA_HHF_EVICT_TIMEOUT,
- jiffies_to_msecs(q->hhf_evict_timeout)) ||
+ jiffies_to_usecs(q->hhf_evict_timeout)) ||
nla_put_u32(skb, TCA_HHF_NON_HH_WEIGHT, q->hhf_non_hh_weight))
goto nla_put_failure;
--
1.8.5.1
^ permalink raw reply related
* Re: [PATCH net-next 1/2] include/uapi/linux/xfrm.h: Pack struct xfrm_userpolicy_info
From: Fan Du @ 2014-01-09 8:39 UTC (permalink / raw)
To: Sergei Shtylyov; +Cc: steffen.klassert, stephen, davem, dev, netdev
In-Reply-To: <52CC851B.2060401@cogentembedded.com>
On 2014年01月08日 06:52, Sergei Shtylyov wrote:
> Hello.
>
> On 07-01-2014 10:48, Fan Du wrote:
>
>> Otherwise 64bits kernel has sizeof(struct xfrm_userpolicy_info) 168 bytes,
>> while 32bits compiled iproute2 see the same structure as 164 bytes, which
>> leading deficit xfrm policy, in turn broken IPsec connectivity.
>
>> Fix this by packing the structure.
>
> This will force byte-by-byte access to all members on some arches like ARM...
>
>> Signed-off-by: Fan Du <fan.du@windriver.com>
>> ---
>> include/uapi/linux/xfrm.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>> diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
>> index a8cd6a4..470bfae 100644
>> --- a/include/uapi/linux/xfrm.h
>> +++ b/include/uapi/linux/xfrm.h
>> @@ -405,7 +405,7 @@ struct xfrm_userpolicy_info {
>> /* Automatically expand selector to include matching ICMP payloads. */
>> #define XFRM_POLICY_ICMP 2
>> __u8 share;
>> -};
>> +} __attribute__((packed));
>
> Please use the __packed macro instead. I guess you haven't run checkpatch.pl?
Lucky me, I run checkpatch every time before sending patch out.
--
浮沉随浪只记今朝笑
--fan
_______________________________________________
Dev mailing list
Dev@lists.strongswan.org
https://lists.strongswan.org/mailman/listinfo/dev
^ permalink raw reply
* Re: [PATCH net-next 0/2] Pack struct xfrm_usersa_info and struct xfrm_userpolicy_info
From: Fan Du @ 2014-01-09 8:34 UTC (permalink / raw)
To: David Laight
Cc: Steffen Klassert, stephen@networkplumber.org, davem@davemloft.net,
dev@lists.strongswan.org, netdev@vger.kernel.org
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6D453AEE@AcuExch.aculab.com>
On 2014年01月07日 18:00, David Laight wrote:
>> From: Fan Du
>> > Sent: 07 January 2014 08:00
>> > On 2014?01?07? 15:47, Steffen Klassert wrote:
>>> > > On Tue, Jan 07, 2014 at 02:48:57PM +0800, Fan Du wrote:
>>>> > >> When trying to setup IPsec configuration on a 64bits host with
>>>> > >> iproute2(32bits compiled), the intened xfrm policy and sa is
>>>> > >> either deficit or wrong when kernel trying to parse user land
>>>> > >> information.
>>>> > >>
>>>> > >> Further investigatino shows that:
>>>> > >> L: kernel
>>>> > >> R: iproute2
>>>> > >>
>>>> > >> sizeof userpolicy usersa
>>>> > >> 64bits(unpacked) 168/168 224/224
>>>> > >> 32bits(unpacked) 164/164 220/220
>>>> > >> ^ ^
>>>> > >> L R
>>>> > >>
>>>> > >> To keep kernel and user land see a consistent structure, after
>>>> > >> add packing attribute, now it looks like this:
>>>> > >>
>>>> > >> 64bits( packed) 164/164 217/217
>>>> > >> 32bits( packed) 164/164 217/217
>>>> > >> ^ ^
>>>> > >> L R
>>>> > >>
>>> > >
>>> > > We don't change userspace exported structures. This breaks
>>> > > existing userspace tools.
>>> > >
>> >
>> > Then user with 32bits iproute2 or StrongSwan has to rebuild as 64bits?
> The kernel has support (in various places) for different structure
> layouts for 32bit and 64bit processes.
> Looks like it needs one here as well (I'm not volunteering though).
>
> At a guess the structures contain a 64bit field that is on an 8n+4
> byte boundary. On i386 64bit fields are only 4 byte aligned, on
> amd64 they are 8 byte aligned.
>
> Packing the structures is definitely wrong. Some 32bit systems (IIRC sparc)
> align 64bit items on 8 byte boundaries. Not to mention the expensive byte
> by byte accesses this forces on some systems.
I don't know much about sparc, if I read your message right, you mean 32bit sparc
system also has padding even if pack attribute is supplied.
And in this scenario, the code path here is only for configuration, i.e., not hot
path, so performance issue is not so crucial here.
>
> The structure could have been defined with the 64bit field marked
> __attribute__((aligned(4))) - so using the same layout on 32bit
> and 64 bit systems , but it is too late to do that now.
Thanks for this useful information.
--
浮沉随浪只记今朝笑
--fan
_______________________________________________
Dev mailing list
Dev@lists.strongswan.org
https://lists.strongswan.org/mailman/listinfo/dev
^ permalink raw reply
* Re: [PATCH net-next v2 3/4] virtio-net: auto-tune mergeable rx buffer size for improved performance
From: Michael Dalton @ 2014-01-09 8:28 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, lf-virt, Eric Dumazet, David S. Miller
In-Reply-To: <20140109064807.GC19559@redhat.com>
Hi Michael,
Here's a quick sketch of some code that enforces a minimum buffer
alignment of only 64, and has a maximum theoretical buffer size of
aligned GOOD_PACKET_LEN + (BUF_ALIGN - 1) * BUF_ALIGN, which is at least
1536 + 63 * 64 = 5568. On x86, we already use a 64 byte alignment, and
this code supports all current buffer sizes, from 1536 to PAGE_SIZE.
#if L1_CACHE_BYTES < 64
#define MERGEABLE_BUFFER_ALIGN 64
#define MERGEABLE_BUFFER_SHIFT 6
#else
#define MERGEABLE_BUFFER_ALIGN L1_CACHE_BYTES
#define MERGEABLE_BUFFER_SHIFT L1_CACHE_SHIFT
#endif
#define MERGEABLE_BUFFER_MIN ALIGN(GOOD_PACKET_LEN +
sizeof(virtio_net_hdr_mrg_rbuf),
MERGEABLE_BUFFER_ALIGN)
#define MERGEABLE_BUFFER_MAX min(MERGEABLE_BUFFER_MIN +
(MERGEABLE_BUFFER_ALIGN - 1) *
MERGEABLE_BUFFER_ALIGN, PAGE_SIZE)
/* Extract buffer length from a mergeable buffer context. */
static u16 get_mergeable_buf_ctx_len(void *ctx) {
u16 len = (uintptr_t)ctx & (MERGEABLE_BUFFER_ALIGN - 1);
return MERGEABLE_BUFFER_MIN + (len << MERGEABLE_BUFFER_SHIFT);
}
/* Extract buffer base address from a mergeable buffer context. */
static void *get_mergeable_buf_ctx_base(void *ctx) {
return (void *) ((uintptr)ctx & -MERGEABLE_BUFFER_ALIGN);
}
/* Convert a base address and length to a mergeable buffer context. */
static void *to_mergeable_buf_ctx(void *base, u16 len) {
len -= MERGEABLE_BUFFER_MIN;
return (void *) ((uintptr)base | (len >> MERGEABLE_BUFFER_SHIFT));
}
/* Compute the packet buffer length for a receive queue. */
static u16 get_mergeable_buffer_len(struct receive_queue *rq) {
u16 len = clamp_t(u16, MERGEABLE_BUFFER_MIN,
ewma_read(&rq->avg_pkt_len),
MERGEABLE_BUFFER_MAX);
return ALIGN(len, MERGEABLE_BUFFER_ALIGN);
}
Best,
Mike
^ permalink raw reply
* Re: [PATCH net 2/2] net: core: explicitly select a txq before doing l2 forwarding
From: Jason Wang @ 2014-01-09 8:28 UTC (permalink / raw)
To: Neil Horman; +Cc: mst, e1000-devel, netdev, linux-kernel, John Fastabend, davem
In-Reply-To: <20140108144025.GA17802@neilslaptop.think-freely.org>
On 01/08/2014 10:40 PM, Neil Horman wrote:
> On Wed, Jan 08, 2014 at 11:21:21AM +0800, Jason Wang wrote:
>> On 01/07/2014 09:17 PM, Neil Horman wrote:
>>> On Tue, Jan 07, 2014 at 11:42:24AM +0800, Jason Wang wrote:
>>>> On 01/06/2014 08:42 PM, Neil Horman wrote:
>>>>> On Mon, Jan 06, 2014 at 11:21:07AM +0800, Jason Wang wrote:
>>>>>> Currently, the tx queue were selected implicitly in ndo_dfwd_start_xmit(). The
>>>>>> will cause several issues:
>>>>>>
>>>>>> - NETIF_F_LLTX was forced for macvlan device in this case which lead extra lock
>>>>>> contention.
>>>>>> - dev_hard_start_xmit() was called with NULL txq which bypasses the net device
>>>>>> watchdog
>>>>>> - dev_hard_start_xmit() does not check txq everywhere which will lead a crash
>>>>>> when tso is disabled for lower device.
>>>>>>
>>>>>> Fix this by explicitly introducing a select queue method just for l2 forwarding
>>>>>> offload (ndo_dfwd_select_queue), and introducing dfwd_direct_xmit() to do the
>>>>>> queue selecting and transmitting for l2 forwarding.
>>>>>>
>>>>>> With this fixes, NETIF_F_LLTX could be preserved for macvlan and there's no need
>>>>>> to check txq against NULL in dev_hard_start_xmit().
>>>>>>
>>>>>> In the future, it was also required for macvtap l2 forwarding support since it
>>>>>> provides a necessary synchronization method.
>>>>>>
>>>>>> Cc: John Fastabend <john.r.fastabend@intel.com>
>>>>>> Cc: Neil Horman <nhorman@tuxdriver.com>
>>>>>> Cc: e1000-devel@lists.sourceforge.net
>>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>>> Instead of creating another operation here to do special queue selection, why
>>>>> not just have ndo_dfwd_start_xmit include a pointer to a pointer in its argument
>>>>> list, so it can pass the txq it used back to the caller (dev_hard_start_xmit)?
>>>>> ndo_dfwd_start_xmit already knows which queue set to pick from (since their
>>>>> reserved for the device doing the transmitting). It seems more clear to me than
>>>>> creating a new netdevice operation.
>>>> See commit 8ffab51b3dfc54876f145f15b351c41f3f703195 ("macvlan: lockless
>>>> tx path"). The point is keep the tx path lockless to be efficient and
>>>> simplicity for management. And macvtap multiqueue was also implemented
>>>> with this assumption. The real contention should be done in the txq of
>>>> lower device instead of macvlan itself. This is also needed for
>>>> multiqueue macvtap.
>>> Ok, I see how you're preserving LLTX here, and thats great, but it doesn't
>>> really buy us anything that I can see. If a macvlan is using hardware
>>> acceleration, it needs to arbitrate access to that hardware. Weather thats done
>>> by locking the lowerdev's tx queue lock or by enforcing locking on the macvlan
>>> itself is equivalent. The decision to use dfwd hardware acceleration is made on
>>> open, so its not like theres any traffic that can avoid the lock, as it all goes
>>> through the hardware. All I see that this has bought us is an extra net_device
>>> method (which isn't a big deal, but not necessecary as I see it).
>> As I replied to patch 1/2, looking at the code itself again. The locking
>> on the lowerdev's tx queue is really need since we need synchronize with
>> other control path. Two examples are dev watchdog and ixgbe_down() both
>> of which will try to hold tx lock to synchronize the with transmission.
>> Without holding the lowerdev tx lock, we may have more serious issues.
>> Also, it's a little strange for a net device has two modes. Future
>> developers need to care about two different tx lock paths which is sub
>> optimal.
>>
> Ok, having looked at this for a few hours, I agree, locking in the lowerdev has
> some definiate advantages in plugging the holes you've pointed out.
>
>> For the issue of an extra net_device method, if you don't like we can
>> reuse the ndo_select_queue by also passing the accel_priv to that method.
> I do, that actually simplifies things, since it lets us use the entire
> dev_hard_start_xmit path unmodified, which gives us the locking your looking for
> without having to create a new slimmed down variant of dev_hard_start_xmit.
>
> Regards
> Neil
Right, will post V2.
------------------------------------------------------------------------------
CenturyLink Cloud: The Leader in Enterprise Cloud Services.
Learn Why More Businesses Are Choosing CenturyLink Cloud For
Critical Workloads, Development Environments & Everything In Between.
Get a Quote or Start a Free Trial Today.
http://pubads.g.doubleclick.net/gampad/clk?id=119420431&iu=/4140/ostg.clktrk
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
^ permalink raw reply
* Re: [PATCH net-next 2/2] include/uapi/linux/xfrm.h: Pack struct xfrm_usersa_info
From: Fan Du @ 2014-01-09 8:24 UTC (permalink / raw)
To: Ben Hutchings; +Cc: steffen.klassert, stephen, davem, dev, netdev
In-Reply-To: <1389213183.1644.31.camel@bwh-desktop.uk.level5networks.com>
On 2014年01月09日 04:33, Ben Hutchings wrote:
> On Tue, 2014-01-07 at 14:48 +0800, Fan Du wrote:
>> Otherwise 64bits kernel has sizeof(struct xfrm_usersa_info) 224 bytes,
>> while 32bits compiled iproute2 see the same structure as 220 bytes, which
>> leading deficit xfrm sa, in turn broken IPsec connectivity.
>>
>> Fix this by packing the structure.
>>
>> Signed-off-by: Fan Du<fan.du@windriver.com>
>> ---
>> include/uapi/linux/xfrm.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
>> index 470bfae..61460c4 100644
>> --- a/include/uapi/linux/xfrm.h
>> +++ b/include/uapi/linux/xfrm.h
>> @@ -366,7 +366,7 @@ struct xfrm_usersa_info {
>> #define XFRM_STATE_AF_UNSPEC 32
>> #define XFRM_STATE_ALIGN4 64
>> #define XFRM_STATE_ESN 128
>> -};
>> +} __attribute__((packed));
>>
>> #define XFRM_SA_XFLAG_DONT_ENCAP_DSCP 1
>>
>
> That change will make access to the structure very slow on some
> architectures, and I suspect it will cause other compatibility problems.
>
> I think the right thing to do is to reduce the minimum length of the
> structure in the netlink policy so that padding at the end is not
> required.
Could you please be more specific about this? Thanks.
I'm afraid we can only rearrange structure member order to reduce size
on 64bits, alas that's not feasible here :(
(It looks like all field offsets will be the same on all
> 32/64-bit architecture pairs and there is only a differing amount of
> padding at the end of the structure for 32/64-bit alignment.)
>
> Ben.
>
--
浮沉随浪只记今朝笑
--fan
_______________________________________________
Dev mailing list
Dev@lists.strongswan.org
https://lists.strongswan.org/mailman/listinfo/dev
^ permalink raw reply
* Re: [PATCH net-next 0/2] Pack struct xfrm_usersa_info and struct xfrm_userpolicy_info
From: Fan Du @ 2014-01-09 8:24 UTC (permalink / raw)
To: David Miller; +Cc: steffen.klassert, stephen, dev, netdev
In-Reply-To: <20140107.130749.215535043457683103.davem@davemloft.net>
On 2014年01月08日 02:07, David Miller wrote:
> From: Fan Du<fan.du@windriver.com>
> Date: Tue, 7 Jan 2014 14:48:57 +0800
>
>> > When trying to setup IPsec configuration on a 64bits host with
>> > iproute2(32bits compiled), the intened xfrm policy and sa is
>> > either deficit or wrong when kernel trying to parse user land
>> > information.
> You can't make this change without breaking userspace.
>
> We'll have to translate the data structures somehow with a compat
> layer like we have for 32/64 bit compatability for system calls.
Do you mean below code snippet?
compat_sys_sendmsg
-> __sys_sendmsg
-> ___sys_sendmsg
-> get_compat_msghdr <- translate the structure here
It's a bit of late in the code flow to do the translation in
xfrm_user code, or I missed what you actually mean.
--
浮沉随浪只记今朝笑
--fan
_______________________________________________
Dev mailing list
Dev@lists.strongswan.org
https://lists.strongswan.org/mailman/listinfo/dev
^ permalink raw reply
* [PATCH] iproute2 support for Heavy Hitter Filter (HHF) qdisc.
From: Terry Lam @ 2014-01-09 8:23 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev, Nandita Dukkipati, Terry Lam
$tc qdisc add dev eth0 hhf help
Usage: ... hhf [ limit PACKETS ] [ quantum BYTES]
[ hh_limit NUMBER ]
[ reset_timeout TIME ]
[ admit_bytes BYTES ]
[ evict_timeout TIME ]
[ non_hh_weight NUMBER ]
$tc -s -d qdisc show dev eth0
qdisc hhf 8005: root refcnt 32 limit 1000p quantum 1514 hh_limit 2048
reset_timeout 40.0ms admit_bytes 131072 evict_timeout 1.0s non_hh_weight 2
Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
backlog 0b 0p requeues 0
drop_overlimit 0 hh_overlimit 0 tot_hh 0 cur_hh 0
HHF qdisc parameters:
- limit: max number of packets in qdisc (default 1000)
- quantum: max deficit per RR round (default 1 MTU)
- hh_limit: max number of HHs to keep states (default 2048)
- reset_timeout: time to reset HHF counters (default 40ms)
- admit_bytes: counter thresh to classify as HH (default 128KB)
- evict_timeout: threshold to evict idle HHs (default 1s)
- non_hh_weight: DRR weight for mice (default 2)
Signed-off-by: Terry Lam <vtlam@google.com>
---
include/linux/pkt_sched.h | 23 ++++++
tc/Makefile | 1 +
tc/q_hhf.c | 199 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 223 insertions(+)
create mode 100644 tc/q_hhf.c
diff --git a/include/linux/pkt_sched.h b/include/linux/pkt_sched.h
index a806687..ddd6577 100644
--- a/include/linux/pkt_sched.h
+++ b/include/linux/pkt_sched.h
@@ -790,4 +790,27 @@ struct tc_fq_qd_stats {
__u32 throttled_flows;
__u32 pad;
};
+
+/* Heavy-Hitter Filter */
+
+enum {
+ TCA_HHF_UNSPEC,
+ TCA_HHF_BACKLOG_LIMIT,
+ TCA_HHF_QUANTUM,
+ TCA_HHF_HH_FLOWS_LIMIT,
+ TCA_HHF_RESET_TIMEOUT,
+ TCA_HHF_ADMIT_BYTES,
+ TCA_HHF_EVICT_TIMEOUT,
+ TCA_HHF_NON_HH_WEIGHT,
+ __TCA_HHF_MAX
+};
+
+#define TCA_HHF_MAX (__TCA_HHF_MAX - 1)
+
+struct tc_hhf_xstats {
+ __u32 drop_overlimit; /* number of time qdisc packet limit was hit */
+ __u32 hh_overlimit; /* number of time max heavy-hitters was hit */
+ __u32 hh_tot_count; /* number of captured heavy-hitters so far */
+ __u32 hh_cur_count; /* number of current heavy-hitters */
+};
#endif
diff --git a/tc/Makefile b/tc/Makefile
index 84215c0..7c6e66e 100644
--- a/tc/Makefile
+++ b/tc/Makefile
@@ -53,6 +53,7 @@ TCMODULES += q_mqprio.o
TCMODULES += q_codel.o
TCMODULES += q_fq_codel.o
TCMODULES += q_fq.o
+TCMODULES += q_hhf.o
ifeq ($(TC_CONFIG_IPSET), y)
ifeq ($(TC_CONFIG_XT), y)
diff --git a/tc/q_hhf.c b/tc/q_hhf.c
new file mode 100644
index 0000000..06ec8a2
--- /dev/null
+++ b/tc/q_hhf.c
@@ -0,0 +1,199 @@
+/* q_hhf.c Heavy-Hitter Filter (HHF)
+ *
+ * Copyright (C) 2013 Terry Lam <vtlam@google.com>
+ */
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <syslog.h>
+#include <fcntl.h>
+#include <sys/socket.h>
+#include <netinet/in.h>
+#include <arpa/inet.h>
+#include <string.h>
+
+#include "utils.h"
+#include "tc_util.h"
+
+static void explain(void)
+{
+ fprintf(stderr, "Usage: ... hhf [ limit PACKETS ] [ quantum BYTES]\n");
+ fprintf(stderr, " [ hh_limit NUMBER ]\n");
+ fprintf(stderr, " [ reset_timeout TIME ]\n");
+ fprintf(stderr, " [ admit_bytes BYTES ]\n");
+ fprintf(stderr, " [ evict_timeout TIME ]\n");
+ fprintf(stderr, " [ non_hh_weight NUMBER ]\n");
+}
+
+static int hhf_parse_opt(struct qdisc_util *qu, int argc, char **argv,
+ struct nlmsghdr *n)
+{
+ unsigned limit = 0;
+ unsigned quantum = 0;
+ unsigned hh_limit = 0;
+ unsigned reset_timeout = 0;
+ unsigned admit_bytes = 0;
+ unsigned evict_timeout = 0;
+ unsigned non_hh_weight = 0;
+ struct rtattr *tail;
+
+ while (argc > 0) {
+ if (strcmp(*argv, "limit") == 0) {
+ NEXT_ARG();
+ if (get_unsigned(&limit, *argv, 0)) {
+ fprintf(stderr, "Illegal \"limit\"\n");
+ return -1;
+ }
+ } else if (strcmp(*argv, "quantum") == 0) {
+ NEXT_ARG();
+ if (get_unsigned(&quantum, *argv, 0)) {
+ fprintf(stderr, "Illegal \"quantum\"\n");
+ return -1;
+ }
+ } else if (strcmp(*argv, "hh_limit") == 0) {
+ NEXT_ARG();
+ if (get_unsigned(&hh_limit, *argv, 0)) {
+ fprintf(stderr, "Illegal \"hh_limit\"\n");
+ return -1;
+ }
+ } else if (strcmp(*argv, "reset_timeout") == 0) {
+ NEXT_ARG();
+ if (get_time(&reset_timeout, *argv)) {
+ fprintf(stderr, "Illegal \"reset_timeout\"\n");
+ return -1;
+ }
+ } else if (strcmp(*argv, "admit_bytes") == 0) {
+ NEXT_ARG();
+ if (get_unsigned(&admit_bytes, *argv, 0)) {
+ fprintf(stderr, "Illegal \"admit_bytes\"\n");
+ return -1;
+ }
+ } else if (strcmp(*argv, "evict_timeout") == 0) {
+ NEXT_ARG();
+ if (get_time(&evict_timeout, *argv)) {
+ fprintf(stderr, "Illegal \"evict_timeout\"\n");
+ return -1;
+ }
+ } else if (strcmp(*argv, "non_hh_weight") == 0) {
+ NEXT_ARG();
+ if (get_unsigned(&non_hh_weight, *argv, 0)) {
+ fprintf(stderr, "Illegal \"non_hh_weight\"\n");
+ return -1;
+ }
+ } else if (strcmp(*argv, "help") == 0) {
+ explain();
+ return -1;
+ } else {
+ fprintf(stderr, "What is \"%s\"?\n", *argv);
+ explain();
+ return -1;
+ }
+ argc--; argv++;
+ }
+
+ tail = NLMSG_TAIL(n);
+ addattr_l(n, 1024, TCA_OPTIONS, NULL, 0);
+ if (limit)
+ addattr_l(n, 1024, TCA_HHF_BACKLOG_LIMIT, &limit,
+ sizeof(limit));
+ if (quantum)
+ addattr_l(n, 1024, TCA_HHF_QUANTUM, &quantum, sizeof(quantum));
+ if (hh_limit)
+ addattr_l(n, 1024, TCA_HHF_HH_FLOWS_LIMIT, &hh_limit,
+ sizeof(hh_limit));
+ if (reset_timeout)
+ addattr_l(n, 1024, TCA_HHF_RESET_TIMEOUT, &reset_timeout,
+ sizeof(reset_timeout));
+ if (admit_bytes)
+ addattr_l(n, 1024, TCA_HHF_ADMIT_BYTES, &admit_bytes,
+ sizeof(admit_bytes));
+ if (evict_timeout)
+ addattr_l(n, 1024, TCA_HHF_EVICT_TIMEOUT, &evict_timeout,
+ sizeof(evict_timeout));
+ if (non_hh_weight)
+ addattr_l(n, 1024, TCA_HHF_NON_HH_WEIGHT, &non_hh_weight,
+ sizeof(non_hh_weight));
+ tail->rta_len = (void *) NLMSG_TAIL(n) - (void *) tail;
+ return 0;
+}
+
+static int hhf_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
+{
+ struct rtattr *tb[TCA_HHF_MAX + 1];
+ unsigned limit;
+ unsigned quantum;
+ unsigned hh_limit;
+ unsigned reset_timeout;
+ unsigned admit_bytes;
+ unsigned evict_timeout;
+ unsigned non_hh_weight;
+ SPRINT_BUF(b1);
+
+ if (opt == NULL)
+ return 0;
+
+ parse_rtattr_nested(tb, TCA_HHF_MAX, opt);
+
+ if (tb[TCA_HHF_BACKLOG_LIMIT] &&
+ RTA_PAYLOAD(tb[TCA_HHF_BACKLOG_LIMIT]) >= sizeof(__u32)) {
+ limit = rta_getattr_u32(tb[TCA_HHF_BACKLOG_LIMIT]);
+ fprintf(f, "limit %up ", limit);
+ }
+ if (tb[TCA_HHF_QUANTUM] &&
+ RTA_PAYLOAD(tb[TCA_HHF_QUANTUM]) >= sizeof(__u32)) {
+ quantum = rta_getattr_u32(tb[TCA_HHF_QUANTUM]);
+ fprintf(f, "quantum %u ", quantum);
+ }
+ if (tb[TCA_HHF_HH_FLOWS_LIMIT] &&
+ RTA_PAYLOAD(tb[TCA_HHF_HH_FLOWS_LIMIT]) >= sizeof(__u32)) {
+ hh_limit = rta_getattr_u32(tb[TCA_HHF_HH_FLOWS_LIMIT]);
+ fprintf(f, "hh_limit %u ", hh_limit);
+ }
+ if (tb[TCA_HHF_RESET_TIMEOUT] &&
+ RTA_PAYLOAD(tb[TCA_HHF_RESET_TIMEOUT]) >= sizeof(__u32)) {
+ reset_timeout = rta_getattr_u32(tb[TCA_HHF_RESET_TIMEOUT]);
+ fprintf(f, "reset_timeout %s ", sprint_time(reset_timeout, b1));
+ }
+ if (tb[TCA_HHF_ADMIT_BYTES] &&
+ RTA_PAYLOAD(tb[TCA_HHF_ADMIT_BYTES]) >= sizeof(__u32)) {
+ admit_bytes = rta_getattr_u32(tb[TCA_HHF_ADMIT_BYTES]);
+ fprintf(f, "admit_bytes %u ", admit_bytes);
+ }
+ if (tb[TCA_HHF_EVICT_TIMEOUT] &&
+ RTA_PAYLOAD(tb[TCA_HHF_EVICT_TIMEOUT]) >= sizeof(__u32)) {
+ evict_timeout = rta_getattr_u32(tb[TCA_HHF_EVICT_TIMEOUT]);
+ fprintf(f, "evict_timeout %s ", sprint_time(evict_timeout, b1));
+ }
+ if (tb[TCA_HHF_NON_HH_WEIGHT] &&
+ RTA_PAYLOAD(tb[TCA_HHF_NON_HH_WEIGHT]) >= sizeof(__u32)) {
+ non_hh_weight = rta_getattr_u32(tb[TCA_HHF_NON_HH_WEIGHT]);
+ fprintf(f, "non_hh_weight %u ", non_hh_weight);
+ }
+ return 0;
+}
+
+static int hhf_print_xstats(struct qdisc_util *qu, FILE *f,
+ struct rtattr *xstats)
+{
+ struct tc_hhf_xstats *st;
+
+ if (xstats == NULL)
+ return 0;
+
+ if (RTA_PAYLOAD(xstats) < sizeof(*st))
+ return -1;
+
+ st = RTA_DATA(xstats);
+
+ fprintf(f, " drop_overlimit %u hh_overlimit %u tot_hh %u cur_hh %u",
+ st->drop_overlimit, st->hh_overlimit,
+ st->hh_tot_count, st->hh_cur_count);
+ return 0;
+}
+
+struct qdisc_util hhf_qdisc_util = {
+ .id = "hhf",
+ .parse_qopt = hhf_parse_opt,
+ .print_qopt = hhf_print_opt,
+ .print_xstats = hhf_print_xstats,
+};
--
1.8.5.1
^ permalink raw reply related
* Re: mlx4 w/ IOMMU broken, kernel 3.12.6
From: Amir Vadai @ 2014-01-09 7:54 UTC (permalink / raw)
To: David Lamparter; +Cc: netdev
In-Reply-To: <20140108134917.GC351037@jupiter.n2.diac24.net>
Hi David,
What is the firmware version?
You can get it by issuing 'ethtool -i <interface>'
Please make sure you're using the latest FW from mellanox.com.
We still don't have MLNX_OFED package that supports this kernel.
Amir.
^ permalink raw reply
* Re: [PATCH net-next V2 1/3] net: Add GRO support for UDP encapsulating protocols
From: Or Gerlitz @ 2014-01-09 7:19 UTC (permalink / raw)
To: Jerry Chu
Cc: Eric Dumazet, Eric Dumazet, Herbert Xu, netdev@vger.kernel.org,
David Miller, Yan Burman, Shlomo Pongratz
In-Reply-To: <CAPshTChSSB8mycXMXSJ656tpBGDqyjUF37+V2S2eAPQs47gJ2A@mail.gmail.com>
On 09/01/2014 05:12, Jerry Chu wrote:
>> >
>> >
>>> >>Also "udp_offload" is a little misleading - you are not trying to GRO UDP
>>> >>pkts where UDP is the real transport. You are only trying to GRO UDP
>>> >>encapped TCP pkts.
>> >
>> >
>> >Indeed -- however, I just plugged into what was there for GSO, e.g stack
>> >will not do GSO for plain UDP
>> >packets, only for those who encapsulate something the code that does this is
>> >udp_offloads.c -- any suggestion
>> >how to phrase/frame the change you envision?
> There is already udpv4_offload for real udp gso (ufo) offload. How about "udp_encap_offload" for your stuff?
>
well, I don't see how it can work... the problem with your suggestion is
that we'll have two structures to compete on the UDP protocol entry in
the inet offloads structure, one that holds the current udp gso offloads
and one with the udp gro offloads.
^ permalink raw reply
* Re: [PATCH net 1/2] macvlan: forbid L2 fowarding offload for macvtap
From: Michael S. Tsirkin @ 2014-01-09 7:17 UTC (permalink / raw)
To: John Fastabend
Cc: Jason Wang, John Fastabend, Neil Horman, davem, netdev,
linux-kernel, Vlad Yasevich
In-Reply-To: <52CDA186.1080601@gmail.com>
On Wed, Jan 08, 2014 at 11:05:42AM -0800, John Fastabend wrote:
> [...]
>
> >>>OK I think I'm finally putting all the pieces together thanks.
> >>>
> >>>Do you know why macvtap is setting dev->tx_queue_len by default? If you
> >>>zero this then the noqueue_qdisc is used and the q->enqueue check in
> >>>dev_queue_xmit will fail.
> >>
> >>It was introduced in commit 8a35747a5d13b99e076b0222729e0caa48cb69b6
> >>("macvtap: Limit packet queue length") to limit the length of socket
> >>receive queue of macvtap. But I'm not sure whether the qdisc is a
> >>byproduct of this commit, maybe we can switch to use another name
> >>instead of just reuse dev->tx_queue_length.
> >
> >You mean tx_queue_len really, right?
> >
> >Problem is tx_queue_len can be accessed using netlink sysfs or ioctl,
> >so if someone uses these to control or check the # of packets that
> >can be queued by device, this will break.
> >
> >How about adding ndo_set_tx_queue_len then?
> >
> >At some point we wanted to decouple queue length from tx_queue_length
> >for tun as well, so that would be benefitial there as well.
>
> On the receive side we need to limit the receive queue and the
> dev->tx_queue_len value was used to do this.
>
> However on the tx side when dev->tx_queue_len is set the default
> qdisc pfifo_fast or mq is used depending on if there is multiqueue
> or not. Unless the user specifies some numtxqueues when creating
> the macvtap device then it will be a single queue device and use
> the pfifo_fast qdisc.
>
> This is the default behaviour users could zero txqueuelen when
> they create the device because it is a stacked device with
> NETIF_F_LLTX using the lower devices qdisc makes sense but this
> would appear (from code inspection) to break macvtap_forward()?
>
> if (skb_queue_len(&q->sk.sk_receive_queue) >= dev->tx_queue_len)
> goto drop;
>
> I'm not sure any of this is a problem other than its a bit
> confusing to overload tx_queue_len for the rx_queue_len but the
> precedent has been there for sometime.
So how about ndo ops to access tx_queue_len then?
This way we can set tx_queue_len to 0 and use some
other field to store the rx_queue_len without users noticing.
Along the lines of the below (it's a partial patch just
to give you the idea):
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index 5b7d0e1..e526b46 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -167,7 +167,10 @@ static int dev_ifsioc_locked(struct net *net, struct ifreq *ifr, unsigned int cm
return 0;
case SIOCGIFTXQLEN:
- ifr->ifr_qlen = dev->tx_queue_len;
+ if (dev->netdev_ops->ndo_get_tx_queue_len)
+ ifr->ifr_qlen = dev->netdev_ops->ndo_get_tx_queue_len(dev);
+ else
+ ifr->ifr_qlen = dev->tx_queue_len;
return 0;
default:
@@ -296,7 +299,10 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, unsigned int cmd)
case SIOCSIFTXQLEN:
if (ifr->ifr_qlen < 0)
return -EINVAL;
- dev->tx_queue_len = ifr->ifr_qlen;
+ if (dev->netdev_ops->ndo_set_tx_queue_len)
+ dev->netdev_ops->ndo_set_tx_queue_len(dev, ifr->ifr_qlen);
+ else
+ dev->tx_queue_len = ifr->ifr_qlen;
return 0;
case SIOCSIFNAME:
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index d954b56..f2fd9d5 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -280,10 +280,31 @@ NETDEVICE_SHOW_RW(flags, fmt_hex);
static int change_tx_queue_len(struct net_device *net, unsigned long new_len)
{
- net->tx_queue_len = new_len;
+ if (dev->netdev_ops->ndo_set_tx_queue_len)
+ dev->netdev_ops->ndo_set_tx_queue_len(dev, new_len);
+ else
+ dev->tx_queue_len = new_len;
return 0;
}
+static ssize_t format_tx_queue_len(const struct net_device *net, char *buf)
+{
+ unsigned long tx_queue_len;
+
+ if (dev->netdev_ops->ndo_get_tx_queue_len)
+ tx_queue_len = dev->netdev_ops->ndo_get_tx_queue_len(dev);
+ else
+ tx_queue_len = dev->tx_queue_len;
+
+ return sprintf(buf, fmt_ulong, tx_queue_len);
+}
+
+static ssize_t tx_queue_len_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ return netdev_show(dev, attr, buf, format_tx_queue_len);
+}
+
static ssize_t tx_queue_len_store(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t len)
@@ -293,7 +314,7 @@ static ssize_t tx_queue_len_store(struct device *dev,
return netdev_store(dev, attr, buf, len, change_tx_queue_len);
}
-NETDEVICE_SHOW_RW(tx_queue_len, fmt_ulong);
+DEVICE_ATTR_RW(tx_queue_len);
static ssize_t ifalias_store(struct device *dev, struct device_attribute *attr,
const char *buf, size_t len)
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 2a0e21d..9276e17 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -876,6 +876,7 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
struct nlattr *attr, *af_spec;
struct rtnl_af_ops *af_ops;
struct net_device *upper_dev = netdev_master_upper_dev_get(dev);
+ unsigned long tx_queue_len;
ASSERT_RTNL();
nlh = nlmsg_put(skb, pid, seq, type, sizeof(*ifm), flags);
@@ -890,8 +891,13 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
ifm->ifi_flags = dev_get_flags(dev);
ifm->ifi_change = change;
+ if (dev->netdev_ops->ndo_get_tx_queue_len)
+ tx_queue_len = dev->netdev_ops->ndo_get_tx_queue_len(dev);
+ else
+ tx_queue_len = dev->tx_queue_len;
+
if (nla_put_string(skb, IFLA_IFNAME, dev->name) ||
- nla_put_u32(skb, IFLA_TXQLEN, dev->tx_queue_len) ||
+ nla_put_u32(skb, IFLA_TXQLEN, tx_queue_len) ||
nla_put_u8(skb, IFLA_OPERSTATE,
netif_running(dev) ? dev->operstate : IF_OPER_DOWN) ||
nla_put_u8(skb, IFLA_LINKMODE, dev->link_mode) ||
@@ -1453,8 +1459,14 @@ static int do_setlink(struct net_device *dev, struct ifinfomsg *ifm,
modified = 1;
}
- if (tb[IFLA_TXQLEN])
- dev->tx_queue_len = nla_get_u32(tb[IFLA_TXQLEN]);
+ if (tb[IFLA_TXQLEN]) {
+ u32 new_len = nla_get_u32(tb[IFLA_TXQLEN]);
+
+ if (dev->netdev_ops->ndo_set_tx_queue_len)
+ dev->netdev_ops->ndo_set_tx_queue_len(dev, new_len);
+ else
+ dev->tx_queue_len = new_len;
+ }
if (tb[IFLA_OPERSTATE])
set_operstate(dev, nla_get_u8(tb[IFLA_OPERSTATE]));
@@ -1692,8 +1704,14 @@ struct net_device *rtnl_create_link(struct net *net,
if (tb[IFLA_BROADCAST])
memcpy(dev->broadcast, nla_data(tb[IFLA_BROADCAST]),
nla_len(tb[IFLA_BROADCAST]));
- if (tb[IFLA_TXQLEN])
- dev->tx_queue_len = nla_get_u32(tb[IFLA_TXQLEN]);
+ if (tb[IFLA_TXQLEN]) {
+ u32 new_len = nla_get_u32(tb[IFLA_TXQLEN]);
+
+ if (dev->netdev_ops->ndo_set_tx_queue_len)
+ dev->netdev_ops->ndo_set_tx_queue_len(dev, new_len);
+ else
+ dev->tx_queue_len = new_len;
+ }
if (tb[IFLA_OPERSTATE])
set_operstate(dev, nla_get_u8(tb[IFLA_OPERSTATE]));
if (tb[IFLA_LINKMODE])
Anyone objects?
> It is a change in behaviour
> though in net-next. Previously dev_queue_xmit() was not used so
> the qdisc layer was never hit on the macvtap device. Now with
> dev_queue_xmit() if the user attaches multiple macvlan queues to a
> single qdisc queue this should still work but wont be optimal. Ideally
> the user should create as many qdisc queues at creation time via
> numtxqueues as macvtap queues will be used during runtime so that there
> is a 1:1 mapping or use some heuristic to avoid cases where there
> is a many to 1 mapping.
>
> From my perspective it would be OK to push this configuration/policy
> to the management layer. After all it is the entity that knows how
> to distribute system resources amongst the objects running over the
> macvtap devices. The relevance for me is I defaulted the l2 offloaded
> macvlans to single queue devices and wanted to note in net-next this
> is the same policy used in the non-offloaded case.
>
> Bit long-winded there.
>
> Thanks,
> John
I think it's a real problem you are pointing out - a performance
regression for multiqueue devices.
If we really think no qdisc is typically the right thing to do,
we should just make it the default I think, but I agree
just making tx_queue_len 0 doesn't work without other changes.
What do others think about extra ndo ops so devices can decouple
the internal tx_queue_len from the userspace-visible value?
^ permalink raw reply related
* Re: [PATCH net-next V3 1/3] net: Add GRO support for UDP encapsulating protocols
From: Or Gerlitz @ 2014-01-09 7:14 UTC (permalink / raw)
To: Tom Herbert
Cc: Jerry Chu, Eric Dumazet, Herbert Xu, Linux Netdev List,
David Miller, Yan Burman, Shlomo Pongratz
In-Reply-To: <CA+mtBx9OaN1i1qX3cf8jAqrCmYB06K1f-Fxh+GorWxMcNVj4JA@mail.gmail.com>
On 09/01/2014 09:09, Tom Herbert wrote:
>> + for (p = *head; p; p = p->next) {
>> >+ if (!NAPI_GRO_CB(p)->same_flow)
>> >+ continue;
>> >+
>> >+ uh2 = (struct udphdr *)(p->data + off);
>> >+ if ((*(u32 *)&uh->source != *(u32 *)&uh2->source)) {
>> >+ NAPI_GRO_CB(p)->same_flow = 0;
>> >+ continue;
>> >+ }
>> >+ goto found;
> I don't believe this is correct. If you exit on the first match, skb's
> that follow in the list can still be marked as same_flow. You need to
> walk the whole list I believe (just get rid of the goto).
>
Good catch, will fix.
Looking on the ipv4 and gre gro_receive callbacks they indeed go over
all the list where the tcp gro_receive code does allow itself do goto
found, probably b/c they are last in the chain?
^ permalink raw reply
* Re: [PATCH net-next V3 1/3] net: Add GRO support for UDP encapsulating protocols
From: Tom Herbert @ 2014-01-09 7:09 UTC (permalink / raw)
To: Or Gerlitz
Cc: Jerry Chu, Eric Dumazet, Herbert Xu, Linux Netdev List,
David Miller, Yan Burman, Shlomo Pongratz
In-Reply-To: <1389213278-2200-2-git-send-email-ogerlitz@mellanox.com>
On Wed, Jan 8, 2014 at 12:34 PM, Or Gerlitz <ogerlitz@mellanox.com> wrote:
> Add GRO handlers for protocols that do UDP encapsulation, with the intent of
> being able to coalesce packets which encapsulate packets belonging to
> the same TCP session.
>
> For GRO purposes, the destination UDP port takes the role of the ether type
> field in the ethernet header or the next protocol in the IP header.
>
> The UDP GRO handler will only attempt to coalesce packets whose destination
> port is registered to have gro handler.
>
> Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com>
> ---
> include/linux/netdevice.h | 10 +++-
> include/net/protocol.h | 3 +
> net/core/dev.c | 1 +
> net/ipv4/udp_offload.c | 129 +++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 142 insertions(+), 1 deletions(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index a2a70cc..360551a 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1652,7 +1652,9 @@ struct napi_gro_cb {
> unsigned long age;
>
> /* Used in ipv6_gro_receive() */
> - int proto;
> + u16 proto;
> +
> + u16 udp_mark;
>
> /* used to support CHECKSUM_COMPLETE for tunneling protocols */
> __wsum csum;
> @@ -1691,6 +1693,12 @@ struct packet_offload {
> struct list_head list;
> };
>
> +struct udp_offload {
> + __be16 port;
> + struct offload_callbacks callbacks;
> + struct list_head list;
> +};
> +
> /* often modified stats are per cpu, other are shared (netdev->stats) */
> struct pcpu_sw_netstats {
> u64 rx_packets;
> diff --git a/include/net/protocol.h b/include/net/protocol.h
> index fbf7676..fe9af94 100644
> --- a/include/net/protocol.h
> +++ b/include/net/protocol.h
> @@ -103,6 +103,9 @@ int inet_del_offload(const struct net_offload *prot, unsigned char num);
> void inet_register_protosw(struct inet_protosw *p);
> void inet_unregister_protosw(struct inet_protosw *p);
>
> +void udp_add_offload(struct udp_offload *prot);
> +void udp_del_offload(struct udp_offload *prot);
> +
> #if IS_ENABLED(CONFIG_IPV6)
> int inet6_add_protocol(const struct inet6_protocol *prot, unsigned char num);
> int inet6_del_protocol(const struct inet6_protocol *prot, unsigned char num);
> diff --git a/net/core/dev.c b/net/core/dev.c
> index ce01847..11f7acf 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3858,6 +3858,7 @@ static enum gro_result dev_gro_receive(struct napi_struct *napi, struct sk_buff
> NAPI_GRO_CB(skb)->same_flow = 0;
> NAPI_GRO_CB(skb)->flush = 0;
> NAPI_GRO_CB(skb)->free = 0;
> + NAPI_GRO_CB(skb)->udp_mark = 0;
>
> pp = ptype->callbacks.gro_receive(&napi->gro_list, skb);
> break;
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index 79c62bd..2846ade 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -13,6 +13,16 @@
> #include <linux/skbuff.h>
> #include <net/udp.h>
> #include <net/protocol.h>
> +/*
> +struct udp_offload {
> + __be16 port;
> + struct offload_callbacks callbacks;
> + struct list_head list;
> +};
> +*/
> +
> +static DEFINE_SPINLOCK(udp_offload_lock);
> +static struct list_head udp_offload_base __read_mostly;
>
> static int udp4_ufo_send_check(struct sk_buff *skb)
> {
> @@ -89,14 +99,133 @@ out:
> return segs;
> }
>
> +void udp_add_offload(struct udp_offload *uo)
> +{
> + struct list_head *head = &udp_offload_base;
> +
> + spin_lock(&udp_offload_lock);
> + list_add_rcu(&uo->list, head);
> + spin_unlock(&udp_offload_lock);
> +}
> +EXPORT_SYMBOL(udp_add_offload);
> +
> +void udp_del_offload(struct udp_offload *uo)
> +{
> + struct list_head *head = &udp_offload_base;
> + struct udp_offload *uo1;
> +
> + spin_lock(&udp_offload_lock);
> + list_for_each_entry(uo1, head, list) {
> + if (uo == uo1) {
> + list_del_rcu(&uo->list);
> + goto out;
> + }
> + }
> +
> + pr_warn("udp_remove_offload: %p not found port %d\n", uo, htons(uo->port));
> +out:
> + spin_unlock(&udp_offload_lock);
> +
> + synchronize_net();
> +}
> +EXPORT_SYMBOL(udp_del_offload);
> +
> +static struct sk_buff **udp_gro_receive(struct sk_buff **head, struct sk_buff *skb)
> +{
> + struct list_head *ohead = &udp_offload_base;
> + struct udp_offload *poffload;
> + struct sk_buff *p, **pp = NULL;
> + struct udphdr *uh, *uh2;
> + unsigned int hlen, off;
> + int flush = 1;
> +
> + if (NAPI_GRO_CB(skb)->udp_mark ||
> + (!skb->encapsulation && skb->ip_summed != CHECKSUM_COMPLETE))
> + goto out;
> +
> + /* mark that this skb passed once through the udp gro layer */
> + NAPI_GRO_CB(skb)->udp_mark = 1;
> +
> + off = skb_gro_offset(skb);
> + hlen = off + sizeof(*uh);
> + uh = skb_gro_header_fast(skb, off);
> + if (skb_gro_header_hard(skb, hlen)) {
> + uh = skb_gro_header_slow(skb, hlen, off);
> + if (unlikely(!uh))
> + goto out;
> + }
> +
> + rcu_read_lock();
> + list_for_each_entry_rcu(poffload, ohead, list) {
> + if (poffload->port != uh->dest || !poffload->callbacks.gro_receive)
> + continue;
> + break;
> + }
> +
> + if (&poffload->list == ohead)
> + goto out_unlock;
> +
> + flush = 0;
> +
> + for (p = *head; p; p = p->next) {
> + if (!NAPI_GRO_CB(p)->same_flow)
> + continue;
> +
> + uh2 = (struct udphdr *)(p->data + off);
> + if ((*(u32 *)&uh->source != *(u32 *)&uh2->source)) {
> + NAPI_GRO_CB(p)->same_flow = 0;
> + continue;
> + }
> + goto found;
I don't believe this is correct. If you exit on the first match, skb's
that follow in the list can still be marked as same_flow. You need to
walk the whole list I believe (just get rid of the goto).
> + }
> +
> +found:
> + skb_gro_pull(skb, sizeof(struct udphdr)); /* pull encapsulating udp header */
> + pp = poffload->callbacks.gro_receive(head, skb);
> +
> +out_unlock:
> + rcu_read_unlock();
> +out:
> + NAPI_GRO_CB(skb)->flush |= flush;
> +
> + return pp;
> +}
> +
> +static int udp_gro_complete(struct sk_buff *skb, int nhoff)
> +{
> + struct list_head *ohead = &udp_offload_base;
> + struct udp_offload *poffload;
> + __be16 newlen = htons(skb->len - nhoff);
> + struct udphdr *uh = (struct udphdr *)(skb->data + nhoff);
> + int err = -ENOSYS;
> +
> + uh->len = newlen;
> +
> + rcu_read_lock();
> + list_for_each_entry_rcu(poffload, ohead, list) {
> + if (poffload->port != uh->dest || !poffload->callbacks.gro_complete)
> + continue;
> + break;
> + }
> +
> + if (&poffload->list != ohead)
> + err = poffload->callbacks.gro_complete(skb, nhoff + sizeof(struct udphdr));
> +
> + rcu_read_unlock();
> + return err;
> +}
> +
> static const struct net_offload udpv4_offload = {
> .callbacks = {
> .gso_send_check = udp4_ufo_send_check,
> .gso_segment = udp4_ufo_fragment,
> + .gro_receive = udp_gro_receive,
> + .gro_complete = udp_gro_complete,
> },
> };
>
> int __init udpv4_offload_init(void)
> {
> + INIT_LIST_HEAD(&udp_offload_base);
> return inet_add_offload(&udpv4_offload, IPPROTO_UDP);
> }
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH net-next 3/3] r8152: add supporting the vendor mode only
From: Hayes Wang @ 2014-01-09 6:50 UTC (permalink / raw)
To: oliver, netdev; +Cc: nic_swsd, linux-kernel, linux-usb, Hayes Wang
In-Reply-To: <1389250232-8663-1-git-send-email-hayeswang@realtek.com>
Remove the limitation that the ecm and r8152 drivers couldn't coexist.
Besides, add the feature to support the vendor mode only. This let
someone who doesn't want to use ecm driver easy to use the vendor
driver without creating the udev rule.
Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
drivers/net/usb/Kconfig | 9 +++++++++
drivers/net/usb/cdc_ether.c | 2 +-
drivers/net/usb/r8152.c | 18 ++++++++++++++----
drivers/net/usb/r815x.c | 4 ++--
4 files changed, 26 insertions(+), 7 deletions(-)
diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig
index 6b638a0..d36640e 100644
--- a/drivers/net/usb/Kconfig
+++ b/drivers/net/usb/Kconfig
@@ -102,6 +102,15 @@ config USB_RTL8152
To compile this driver as a module, choose M here: the
module will be called r8152.
+config USB_RTL8152_VENDOR_MODE_ONLY
+ boolean "Force using the vendor mode"
+ depends on USB_RTL8152
+ default n
+ help
+ This would add the devices in the blacklist of the ECM driver.
+ That is, the ECM mode would be disabled, and only the vendor
+ mode could be used.
+
config USB_USBNET
tristate "Multi-purpose USB Networking Framework"
select MII
diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
index 640406a..5c04f8b 100644
--- a/drivers/net/usb/cdc_ether.c
+++ b/drivers/net/usb/cdc_ether.c
@@ -653,7 +653,7 @@ static const struct usb_device_id products[] = {
.driver_info = 0,
},
-#if defined(CONFIG_USB_RTL8152) || defined(CONFIG_USB_RTL8152_MODULE)
+#if defined(CONFIG_USB_RTL8152_VENDOR_MODE_ONLY)
/* Samsung USB Ethernet Adapters */
{
USB_DEVICE_AND_INTERFACE_INFO(SAMSUNG_VENDOR_ID, 0xa101, USB_CLASS_COMM,
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index eab078b..e59e99a 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -450,6 +450,13 @@ enum rtl8152_flags {
#define MCU_TYPE_PLA 0x0100
#define MCU_TYPE_USB 0x0000
+#if defined(CONFIG_USB_RTL8152_VENDOR_MODE_ONLY)
+ #define REALTEK_USB_DEVICE(vend, prod) USB_DEVICE(vend, prod)
+#else
+ #define REALTEK_USB_DEVICE(vend, prod) \
+ USB_DEVICE_INTERFACE_CLASS(vend, prod, USB_CLASS_VENDOR_SPEC)
+#endif
+
struct rx_desc {
__le32 opts1;
#define RX_LEN_MASK 0x7fff
@@ -2733,13 +2740,16 @@ static int rtl_ops_init(struct r8152 *tp, const struct usb_device_id *id)
static int rtl8152_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
+ struct usb_host_interface *alt = intf->cur_altsetting;
struct usb_device *udev = interface_to_usbdev(intf);
struct r8152 *tp;
struct net_device *netdev;
int ret;
- if (udev->actconfig->desc.bConfigurationValue != 1) {
+ if (alt->desc.bInterfaceClass != USB_CLASS_VENDOR_SPEC) {
+#if defined(CONFIG_USB_RTL8152_VENDOR_MODE_ONLY)
usb_driver_set_configuration(udev, 1);
+#endif
return -ENODEV;
}
@@ -2823,9 +2833,9 @@ static void rtl8152_disconnect(struct usb_interface *intf)
/* table of devices that work with this driver */
static struct usb_device_id rtl8152_table[] = {
- {USB_DEVICE(VENDOR_ID_REALTEK, PRODUCT_ID_RTL8152)},
- {USB_DEVICE(VENDOR_ID_REALTEK, PRODUCT_ID_RTL8153)},
- {USB_DEVICE(VENDOR_ID_SAMSUNG, PRODUCT_ID_SAMSUNG)},
+ {REALTEK_USB_DEVICE(VENDOR_ID_REALTEK, PRODUCT_ID_RTL8152)},
+ {REALTEK_USB_DEVICE(VENDOR_ID_REALTEK, PRODUCT_ID_RTL8153)},
+ {REALTEK_USB_DEVICE(VENDOR_ID_SAMSUNG, PRODUCT_ID_SAMSUNG)},
{}
};
diff --git a/drivers/net/usb/r815x.c b/drivers/net/usb/r815x.c
index 5fd2ca6..9934447 100644
--- a/drivers/net/usb/r815x.c
+++ b/drivers/net/usb/r815x.c
@@ -216,7 +216,7 @@ static const struct usb_device_id products[] = {
{
USB_DEVICE_AND_INTERFACE_INFO(REALTEK_VENDOR_ID, 0x8152, USB_CLASS_COMM,
USB_CDC_SUBCLASS_ETHERNET, USB_CDC_PROTO_NONE),
-#if defined(CONFIG_USB_RTL8152) || defined(CONFIG_USB_RTL8152_MODULE)
+#if defined(CONFIG_USB_RTL8152_VENDOR_MODE_ONLY)
.driver_info = 0,
#else
.driver_info = (unsigned long) &r8152_info,
@@ -226,7 +226,7 @@ static const struct usb_device_id products[] = {
{
USB_DEVICE_AND_INTERFACE_INFO(REALTEK_VENDOR_ID, 0x8153, USB_CLASS_COMM,
USB_CDC_SUBCLASS_ETHERNET, USB_CDC_PROTO_NONE),
-#if defined(CONFIG_USB_RTL8152) || defined(CONFIG_USB_RTL8152_MODULE)
+#if defined(CONFIG_USB_RTL8152_VENDOR_MODE_ONLY)
.driver_info = 0,
#else
.driver_info = (unsigned long) &r8153_info,
--
1.8.4.2
^ permalink raw reply related
* [PATCH net-next 2/3] r8152: fix the warnings and a error from checkpatch.pl
From: Hayes Wang @ 2014-01-09 6:50 UTC (permalink / raw)
To: oliver, netdev; +Cc: nic_swsd, linux-kernel, linux-usb, Hayes Wang
In-Reply-To: <1389250232-8663-1-git-send-email-hayeswang@realtek.com>
Fix the following warnings and error:
- WARNING: usb_free_urb(NULL) is safe this check is probably not required
- WARNING: kfree(NULL) is safe this check is probably not required
- ERROR: do not use C99 // comments
Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
drivers/net/usb/r8152.c | 42 +++++++++++++++---------------------------
1 file changed, 15 insertions(+), 27 deletions(-)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 38f3c14..eab078b 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1100,40 +1100,28 @@ static void free_all_mem(struct r8152 *tp)
int i;
for (i = 0; i < RTL8152_MAX_RX; i++) {
- if (tp->rx_info[i].urb) {
- usb_free_urb(tp->rx_info[i].urb);
- tp->rx_info[i].urb = NULL;
- }
+ usb_free_urb(tp->rx_info[i].urb);
+ tp->rx_info[i].urb = NULL;
- if (tp->rx_info[i].buffer) {
- kfree(tp->rx_info[i].buffer);
- tp->rx_info[i].buffer = NULL;
- tp->rx_info[i].head = NULL;
- }
+ kfree(tp->rx_info[i].buffer);
+ tp->rx_info[i].buffer = NULL;
+ tp->rx_info[i].head = NULL;
}
for (i = 0; i < RTL8152_MAX_TX; i++) {
- if (tp->tx_info[i].urb) {
- usb_free_urb(tp->tx_info[i].urb);
- tp->tx_info[i].urb = NULL;
- }
+ usb_free_urb(tp->tx_info[i].urb);
+ tp->tx_info[i].urb = NULL;
- if (tp->tx_info[i].buffer) {
- kfree(tp->tx_info[i].buffer);
- tp->tx_info[i].buffer = NULL;
- tp->tx_info[i].head = NULL;
- }
+ kfree(tp->tx_info[i].buffer);
+ tp->tx_info[i].buffer = NULL;
+ tp->tx_info[i].head = NULL;
}
- if (tp->intr_urb) {
- usb_free_urb(tp->intr_urb);
- tp->intr_urb = NULL;
- }
+ usb_free_urb(tp->intr_urb);
+ tp->intr_urb = NULL;
- if (tp->intr_buff) {
- kfree(tp->intr_buff);
- tp->intr_buff = NULL;
- }
+ kfree(tp->intr_buff);
+ tp->intr_buff = NULL;
}
static int alloc_all_mem(struct r8152 *tp)
@@ -2048,7 +2036,7 @@ static void r8153_first_init(struct r8152 *tp)
/* TX share fifo free credit full threshold */
ocp_write_dword(tp, MCU_TYPE_PLA, PLA_TXFIFO_CTRL, TXFIFO_THR_NORMAL2);
- // rx aggregation
+ /* rx aggregation */
ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_USB_CTRL);
ocp_data &= ~RX_AGG_DISABLE;
ocp_write_word(tp, MCU_TYPE_USB, USB_USB_CTRL, ocp_data);
--
1.8.4.2
^ 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