* [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
* [PATCH net-next 1/3] r8152: change the descriptor
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>
The r8152 could support RTL8153. Update the relative descriptor.
Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
drivers/net/usb/Kconfig | 5 +++--
drivers/net/usb/r8152.c | 2 +-
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig
index 47b0f73..6b638a0 100644
--- a/drivers/net/usb/Kconfig
+++ b/drivers/net/usb/Kconfig
@@ -92,11 +92,12 @@ config USB_RTL8150
module will be called rtl8150.
config USB_RTL8152
- tristate "Realtek RTL8152 Based USB 2.0 Ethernet Adapters"
+ tristate "Realtek RTL8152/RTL8153 Based USB Ethernet Adapters"
select MII
help
This option adds support for Realtek RTL8152 based USB 2.0
- 10/100 Ethernet adapters.
+ 10/100 Ethernet adapters and RTL8153 based USB 3.0 10/100/1000
+ Ethernet adapters.
To compile this driver as a module, choose M here: the
module will be called r8152.
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index bf7d549..38f3c14 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -26,7 +26,7 @@
/* Version Information */
#define DRIVER_VERSION "v1.03.0 (2013/12/26)"
#define DRIVER_AUTHOR "Realtek linux nic maintainers <nic_swsd@realtek.com>"
-#define DRIVER_DESC "Realtek RTL8152 Based USB 2.0 Ethernet Adapters"
+#define DRIVER_DESC "Realtek RTL8152/RTL8153 Based USB Ethernet Adapters"
#define MODULENAME "r8152"
#define R8152_PHY_ID 32
--
1.8.4.2
^ permalink raw reply related
* [PATCH net-next 0/3] r8152: behavior modification
From: Hayes Wang @ 2014-01-09 6:50 UTC (permalink / raw)
To: oliver, netdev; +Cc: nic_swsd, linux-kernel, linux-usb, Hayes Wang
The purpose is to add a choice for determining whether add the
limitation between r8152 and ecm drivers or not.
Hayes Wang (3):
r8152: change the descriptor
r8152: fix the warnings and a error from checkpatch.pl
r8152: add supporting the vendor mode only
drivers/net/usb/Kconfig | 14 ++++++++--
drivers/net/usb/cdc_ether.c | 2 +-
drivers/net/usb/r8152.c | 62 ++++++++++++++++++++++-----------------------
drivers/net/usb/r815x.c | 4 +--
4 files changed, 45 insertions(+), 37 deletions(-)
--
1.8.4.2
^ permalink raw reply
* Re: [PATCH net-next v2 3/4] virtio-net: auto-tune mergeable rx buffer size for improved performance
From: Michael S. Tsirkin @ 2014-01-09 6:48 UTC (permalink / raw)
To: Michael Dalton; +Cc: netdev, lf-virt, Eric Dumazet, David S. Miller
In-Reply-To: <CANJ5vPLvDff5PMYNGvtt-dd=md8b46ZXAYLv-KnerNcfzDNHpw@mail.gmail.com>
On Wed, Jan 08, 2014 at 07:41:58PM -0800, Michael Dalton wrote:
> Sorry, forgot to mention - if we want to explore combining the buffer
> address and truesize into a single void *, we could also exploit the
> fact that our size ranges from aligned GOOD_PACKET_LEN to PAGE_SIZE, and
> potentially encode fewer values for truesize (and require a smaller
> alignment than 256). The prior e-mails discussion of 256 byte alignment
> with 256 values is just one potential design point.
>
> Best,
>
> Mike
Good point. I think we should keep the option to
make buffers bigger than 4K, so I think we should start with 256
alignment, then see if there are workloads that are improved by smaller
alignment.
Can you add wrapper inline functions to pack/unpack size and
buffer pointer to/from void *?
This way it will be easy to experiment with different alignments.
^ permalink raw reply
* [PATCH net-next] qlcnic: Convert vmalloc/memset to kcalloc
From: Joe Perches @ 2014-01-09 6:42 UTC (permalink / raw)
To: Jitendra Kalsaria; +Cc: linux-driver, netdev, LKML
vmalloc is a limited resource. Don't use it unnecessarily.
It seems this allocation should work with kcalloc.
Remove unnecessary memset(,0,) of buf as it's completely
overwritten as the previously only unset field in
struct qlcnic_pci_func_cfg is now set to 0.
Use kfree instead of vfree.
Use ETH_ALEN instead of 6.
Signed-off-by: Joe Perches <joe@perches.com>
---
drivers/net/ethernet/qlogic/qlcnic/qlcnic.h | 2 +-
drivers/net/ethernet/qlogic/qlcnic/qlcnic_sysfs.c | 16 ++++++----------
2 files changed, 7 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic.h b/drivers/net/ethernet/qlogic/qlcnic/qlcnic.h
index 35d4876..8d7aa4c 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic.h
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic.h
@@ -1267,7 +1267,7 @@ struct qlcnic_pci_func_cfg {
u16 port_num;
u8 pci_func;
u8 func_state;
- u8 def_mac_addr[6];
+ u8 def_mac_addr[ETH_ALEN];
};
struct qlcnic_npar_func_cfg {
diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_sysfs.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_sysfs.c
index b529667..c9b704d 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_sysfs.c
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_sysfs.c
@@ -6,7 +6,6 @@
*/
#include <linux/slab.h>
-#include <linux/vmalloc.h>
#include <linux/interrupt.h>
#include "qlcnic.h"
@@ -927,38 +926,35 @@ static ssize_t qlcnic_sysfs_read_pci_config(struct file *file,
u32 pci_func_count = qlcnic_get_pci_func_count(adapter);
struct qlcnic_pci_func_cfg *pci_cfg;
struct qlcnic_pci_info *pci_info;
- size_t pci_info_sz, pci_cfg_sz;
+ size_t pci_cfg_sz;
int i, ret;
pci_cfg_sz = pci_func_count * sizeof(*pci_cfg);
if (size != pci_cfg_sz)
return QL_STATUS_INVALID_PARAM;
- pci_info_sz = pci_func_count * sizeof(*pci_info);
- pci_info = vmalloc(pci_info_sz);
+ pci_info = kcalloc(pci_func_count, sizeof(*pci_info), GFP_KERNEL);
if (!pci_info)
return -ENOMEM;
- memset(pci_info, 0, pci_info_sz);
- memset(buf, 0, pci_cfg_sz);
- pci_cfg = (struct qlcnic_pci_func_cfg *)buf;
-
ret = qlcnic_get_pci_info(adapter, pci_info);
if (ret) {
- vfree(pci_info);
+ kfree(pci_info);
return ret;
}
+ pci_cfg = (struct qlcnic_pci_func_cfg *)buf;
for (i = 0; i < pci_func_count; i++) {
pci_cfg[i].pci_func = pci_info[i].id;
pci_cfg[i].func_type = pci_info[i].type;
+ pci_cfg[i].func_state = 0;
pci_cfg[i].port_num = pci_info[i].default_port;
pci_cfg[i].min_bw = pci_info[i].tx_min_bw;
pci_cfg[i].max_bw = pci_info[i].tx_max_bw;
memcpy(&pci_cfg[i].def_mac_addr, &pci_info[i].mac, ETH_ALEN);
}
- vfree(pci_info);
+ kfree(pci_info);
return size;
}
^ permalink raw reply related
* Re: [PATCH net-next v2 3/4] virtio-net: auto-tune mergeable rx buffer size for improved performance
From: Michael S. Tsirkin @ 2014-01-09 6:42 UTC (permalink / raw)
To: Michael Dalton; +Cc: netdev, lf-virt, Eric Dumazet, David S. Miller
In-Reply-To: <CANJ5vP+_aK0PMc6-hwbu6PduVNDe8jbWR19cN5C+k4mWQzdTGg@mail.gmail.com>
On Wed, Jan 08, 2014 at 07:16:18PM -0800, Michael Dalton wrote:
> Hi Michael,
>
> On Wed, Jan 8, 2014 at 5:42 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > Sorry that I didn't notice early, but there seems to be a bug here.
> > See below.
> Yes, that is definitely a bug. Virtio spec permits OOO completions,
> but current code assumes in-order completion. Thanks for catching this.
>
> > Don't need full int really, it's up to 4K/cache line size,
> > 1 byte would be enough, maximum 2 ...
> > So if all we want is extra 1-2 bytes per buffer, we don't really
> > need this extra level of indirection I think.
> > We can just allocate them before the header together with an skb.
> I'm not sure if I'm parsing the above correctly, but do you mean using a
> few bytes at the beginning of the packet buffer to store truesize? I
> think that will break Jason's virtio-net RX frag coalescing
> code. To coalesce consecutive RX packet buffers, our packet buffers must
> be physically adjacent, and any extra bytes before the start of the
> buffer would break that.
>
> We could allocate an SKB per packet buffer, but if we have multi-buffer
> packets often(e.g., netperf benefiting from GSO/GRO), we would be
> allocating 1 SKB per packet buffer instead of 1 SKB per MAX_SKB_FRAGS
> buffers. How do you feel about any of the below alternatives:
>
> (1) Modify the existing mrg_buf_ctx to chain together free entries
> We can use the 'buf' pointer in mergeable_receive_buf_ctx to chain
> together free entries so that we can support OOO completions. This would
> be similar to how virtio-queue manages free sg entries.
>
> (2) Combine the buffer pointer and truesize into a single void* value
> Your point about there only being a byte needed to encode truesize is
> spot on, and I think we could leverage this to eliminate the out-of-band
> metadata ring entirely. If we were willing to change the packet buffer
> alignment from L1_CACHE_BYTES to 256 (or min (256, L1_CACHE_SIZE)),
I think you mean max here.
> we
> could encode the truesize in the least significant 8 bits of the buffer
> address (encoded as truesize >> 8 as we know all sizes are a multiple
> of 256). This would allow packet buffers up to 64KB in length.
>
> Is there another approach you would prefer to any of these? If the
> cleanliness issues and larger alignment aren't too bad, I think (2)
> sounds promising and allow us to eliminate the metadata ring
> entirely while still permitting RX frag coalescing.
>
> Best,
>
> Mike
I agree, this sounds like a better approach. It's certainly no worse than
current net-next code that always allocates about 1.5K,
and struct sk_buff itself is about 256 bytes on 64 bit.
--
MST
^ permalink raw reply
* Re: [PATCH net-next V2 1/3] net: Add GRO support for UDP encapsulating protocols
From: Or Gerlitz @ 2014-01-09 6:35 UTC (permalink / raw)
To: Jerry Chu, Eric Dumazet, David Miller
Cc: Eric Dumazet, Herbert Xu, netdev@vger.kernel.org, Yan Burman,
Shlomo Pongratz
In-Reply-To: <CAPshTChSSB8mycXMXSJ656tpBGDqyjUF37+V2S2eAPQs47gJ2A@mail.gmail.com>
On 09/01/2014 05:12, Jerry Chu wrote:
> On Wed, Jan 8, 2014 at 12:02 AM, Or Gerlitz <ogerlitz@mellanox.com> wrote:
>> On 08/01/2014 00:11, Jerry Chu wrote:
>>> On Tue, Jan 7, 2014 at 12:37 PM, Or Gerlitz <or.gerlitz@gmail.com> wrote:
>>>> On Tue, Jan 7, 2014 at 10:32 PM, Eric Dumazet <eric.dumazet@gmail.com>
>>>> wrote:
>>>>> On Tue, 2014-01-07 at 22:19 +0200, Or Gerlitz wrote:
>>>>>> On Tue, Jan 7, 2014 at 6:33 PM, Eric Dumazet <eric.dumazet@gmail.com>
>>>>>> wrote:
>>>>>>> On Tue, 2014-01-07 at 17:29 +0200, Or Gerlitz wrote:
>>>>>>>> +
>>>>>>>> +#define MAX_UDP_PORT (1 << 16)
>>>>>>>> +extern const struct net_offload __rcu *udp_offloads[MAX_UDP_PORT];
>>>>>>> Thats 512 KB of memory.
>>>>>>> This will greatly impact forwarding performance of UDP packets with
>>>>>>> random ports, and will increase kernel memory size for embedded
>>>>>>> devices.
>>>>>> Re forwarding, are you referring to the case where the forwarded
>>>>>> packets are encapsulated? packets which are not encapusalted will be
>>>>>> flushed in the gro receive handler (this went out by mistake in V2 but
>>>>>> exists in V1) if skb->encapsulation isn't set.
>>>>>>
>>>>> How do you know encapsulation must be tried for a given incoming
>>>>> packet ? NIC do not magically sets skb->encapsulation I think...
>>>> So here's the thing, per my understanding we want to GRO only received
>>>> **encapsulated** packets whose checksum status is != CHECKSUM_NONE
>>> What's wrong with GRO'ing pkts whose csum == CHECKSUM_NONE?
>>
>> I am not sure, intuitively it sounds a bit wrong to me, empirically, it
>> doesn't work for udp encapsulated / vxlan
>> traffic, I got drops from the tcp stack in tcp_rcv_established() -- if
>> GRO-ed packets carry CHECKSUM_NONE
>> we arrive to the csum_error label, which means that
>> tcp_checksum_complete_user() failed for them
> This is odd because if pkts have been aggregated successfully,
> tcp4_gro_receive() should've skb_checksum() and turned CHECKSUM_NONE
> into CHECKSUM_UNNECESSARY. (I think i've already tested this
> case with my GRE-GRO patch on a NIC that sends up pkts w/ CHECKSUM_NONE.
>
> But granted there are a lot of csum related bugs in the current code. I just spent half a day scratching my head on a very low thruput number with my GRE patch over a GRE tunnel w/ csum flag on. I just tracked it down to be buggy TSO/GRE code that will produce bad csum on the tx side.
The "there are a lot of csum related bugs in the current code" comment
sounds really bad, how do we get into a better place? can you point on
the buggy TSO code that produced bad csum on the tx side?
^ permalink raw reply
* Re: [PATCH net-next V3 3/3] net: Add GRO support for vxlan traffic
From: Or Gerlitz @ 2014-01-09 6:32 UTC (permalink / raw)
To: Eric Dumazet; +Cc: hkchu, edumazet, herbert, netdev, davem, yanb, shlomop
In-Reply-To: <1389219101.31367.21.camel@edumazet-glaptop2.roam.corp.google.com>
On 09/01/2014 00:11, Eric Dumazet wrote:
> On Wed, 2014-01-08 at 22:34 +0200, Or Gerlitz wrote:
>
>> +
>> /* Notify netdevs that UDP port started listening */
>> -static void vxlan_notify_add_rx_port(struct sock *sk)
>> +static void vxlan_notify_add_rx_port(struct vxlan_sock *vs)
>> {
>> struct net_device *dev;
>> + struct sock *sk = vs->sock->sk;
>> struct net *net = sock_net(sk);
>> sa_family_t sa_family = sk->sk_family;
>> __be16 port = inet_sk(sk)->inet_sport;
>> @@ -569,12 +671,16 @@ static void vxlan_notify_add_rx_port(struct sock *sk)
>> port);
>> }
>> rcu_read_unlock();
>> +
>> + if (sa_family == AF_INET)
>> + call_rcu(&vs->rcu, vxlan_add_udp_offload);
> Why waiting RCU grace period here?
Basically the add operation can be done right away, however, since the
delete operation can't be done
instantly when we want it, I wanted to protect against a series of
add/del/add in times T1 < T2 < T3
T1 add(X)
T2 del(X)
T3 add(X)
where the delete is deferred and as a result the 2nd add is done before
the delete and @ the end offload X is not added in the 2nd time.From
your other comment below I conclude that I probably miss something about
the rcu usage here, so will give it further thought.
>
>> }
>>
>> /* Notify netdevs that UDP port is no more listening */
>> -static void vxlan_notify_del_rx_port(struct sock *sk)
>> +static void vxlan_notify_del_rx_port(struct vxlan_sock *vs)
>> {
>> struct net_device *dev;
>> + struct sock *sk = vs->sock->sk;
>> struct net *net = sock_net(sk);
>> sa_family_t sa_family = sk->sk_family;
>> __be16 port = inet_sk(sk)->inet_sport;
>> @@ -586,6 +692,9 @@ static void vxlan_notify_del_rx_port(struct sock *sk)
>> port);
>> }
>> rcu_read_unlock();
>> +
>> + if (sa_family == AF_INET)
>> + call_rcu(&vs->rcu, vxlan_del_udp_offload);
>> }
> This looks buggy.
>
> You need to :
>
> 1) remove the offload structure from list
> 2) Then wait rcu grace period, and finally free the memory
>
>
>
^ permalink raw reply
* RE: [patch] cxgb4: silence shift wrapping static checker warning
From: Dimitrios Michailidis @ 2014-01-09 5:48 UTC (permalink / raw)
To: Dan Carpenter, Kumar A S
Cc: netdev@vger.kernel.org, kernel-janitors@vger.kernel.org
In-Reply-To: <20140109053400.GF1265@elgon.mountain>
> I don't know how large "tp->vlan_shift" is but static checkers worry
> about shift wrapping bugs here.
Indeed.
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Acked-by: Dimitris Michailidis <dm@chelsio.com>
> diff --git a/drivers/net/ethernet/chelsio/cxgb4/l2t.c
> b/drivers/net/ethernet/chelsio/cxgb4/l2t.c
> index cb05be905def..81e8402a74b4 100644
> --- a/drivers/net/ethernet/chelsio/cxgb4/l2t.c
> +++ b/drivers/net/ethernet/chelsio/cxgb4/l2t.c
> @@ -423,7 +423,7 @@ u64 cxgb4_select_ntuple(struct net_device *dev,
> * in the Compressed Filter Tuple.
> */
> if (tp->vlan_shift >= 0 && l2t->vlan != VLAN_NONE)
> - ntuple |= (F_FT_VLAN_VLD | l2t->vlan) << tp->vlan_shift;
> + ntuple |= (u64)(F_FT_VLAN_VLD | l2t->vlan) << tp->vlan_shift;
>
> if (tp->port_shift >= 0)
> ntuple |= (u64)l2t->lport << tp->port_shift;
^ permalink raw reply
* Re: [PATCH net-next V3 3/3] net: Add GRO support for vxlan traffic
From: Or Gerlitz @ 2014-01-09 6:28 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1389218977.31367.18.camel@edumazet-glaptop2.roam.corp.google.com>
On 09/01/2014 00:09, Eric Dumazet wrote:
> On Wed, 2014-01-08 at 22:34 +0200, Or Gerlitz wrote:
>> +
>> +static int vxlan_gro_complete(struct sk_buff *skb, int nhoff)
>> +{
>> + struct ethhdr *eh;
>> + struct packet_offload *ptype;
>> + __be16 type;
>> + /* 22 = 8 bytes for the vlxan header + 14 bytes for the inner eth header */
>> + int vxlan_len = 22;
>
>
> I am pretty sure this can use existing macros or sizeof(...)
sure, will fix
>
^ permalink raw reply
* Re: [PATCH net-next V3 1/3] net: Add GRO support for UDP encapsulating protocols
From: Or Gerlitz @ 2014-01-09 6:25 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+mtBx9yKSW5GFz-zrFSDdkshAmGmdVhds5wgwQKJd0SDEdBCw@mail.gmail.com>
On 08/01/2014 23:58, Tom Herbert wrote:
>> +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)
> Is gro_receive == NULL ever valid? Maybe we can assert on registration instead of checking on every packet.
I see your point, however, the offload structure contains entries for
both gro and gso, asserting on registration could somehow limit the use
cases, isn't that?
> Maybe make this poffload->port == uh->dest and goto "flush = 0". Check below that list end was reached becomes unnecessary.
Sure, will use goto "flush = 0" and if we didn't go there we'll go to
out_unlock
^ permalink raw reply
* Device tree binding for Managed Switch
From: Tama @ 2014-01-09 6:12 UTC (permalink / raw)
To: netdev@vger.kernel.org
Hi All,
I am new to this mailing this. I am using IMX28 based custom designed board, based on TQ Components TQMA28 module. In our board we are using KSZ8863 and KSZ8895 managed switches instead of standard phy device. How to add support for managed switch in device tree.
I am new to device tree configuration. At present I have got
mac0: ethernet@800f0000 {
compatible = "fsl,imx28-fec";
reg = <0x800f0000 0x4000>;
interrupts = <101>;
status = "disabled";
};
mac1: ethernet@800f4000 {
compatible = "fsl,imx28-fec";
reg = <0x800f4000 0x4000>;
interrupts = <102>;
status = "disabled";
};
In my device tree imx28.dtsi and in imx28-mba28.dts I have got
/* FCC1 management to switch */
eth0: ethernet@800f0000 {
device_type = "network";
compatible = "micrel,ksz8895";
reg = <0x800f0000 0x4000>;
local-mac-address = [ 00 01 02 03 04 07 ];
interrupts = <101>;
fixed-link = <1 1 1000 0 0>;
phy-handle = <&phy0>;
phy0: phy@0 {
fixed-link;
full-duplex;
speed = <1000>;
};
};
/* FCC1 management to switch */
eth1: ethernet@800f4000 {
device_type = "network";
compatible = "micrel, ksz8895";
reg = <0x800f0000 0x4000>;
local-mac-address = [ 00 01 02 03 04 08 ];
interrupts = <102>;
fixed-link = <1 1 1000 0 0>;
phy-handle = <&phy1>;
phy1: phy@1 {
fixed-link;
full-duplex;
speed = <1000>;
};
};
I read threads on this mailing list and modified the device tree for managed switch. I am not 100% sure if my configuration is correct. When the kernel is booting it is scanning for phy devices and as it doesnt get a phy device id it doesnt register any device.
Can some one please share information on how to configure the device tree for device and not a phy.
Thanks
Pritam
^ permalink raw reply
* [patch] cxgb4: silence shift wrapping static checker warning
From: Dan Carpenter @ 2014-01-09 5:34 UTC (permalink / raw)
To: Dimitris Michailidis, Kumar Sanghvi; +Cc: netdev, kernel-janitors
I don't know how large "tp->vlan_shift" is but static checkers worry
about shift wrapping bugs here.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/drivers/net/ethernet/chelsio/cxgb4/l2t.c b/drivers/net/ethernet/chelsio/cxgb4/l2t.c
index cb05be905def..81e8402a74b4 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/l2t.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/l2t.c
@@ -423,7 +423,7 @@ u64 cxgb4_select_ntuple(struct net_device *dev,
* in the Compressed Filter Tuple.
*/
if (tp->vlan_shift >= 0 && l2t->vlan != VLAN_NONE)
- ntuple |= (F_FT_VLAN_VLD | l2t->vlan) << tp->vlan_shift;
+ ntuple |= (u64)(F_FT_VLAN_VLD | l2t->vlan) << tp->vlan_shift;
if (tp->port_shift >= 0)
ntuple |= (u64)l2t->lport << tp->port_shift;
^ permalink raw reply related
* Re: [PATCH] netfilter: nf_conntrack: fix RCU race in nf_conntrack_find_get
From: Andrew Vagin @ 2014-01-09 5:24 UTC (permalink / raw)
To: Eric Dumazet, Florian Westphal
Cc: Andrey Vagin, netfilter-devel, netfilter, coreteam, netdev,
linux-kernel, vvs, Pablo Neira Ayuso, Patrick McHardy,
Jozsef Kadlecsik, David S. Miller, Cyrill Gorcunov
In-Reply-To: <1389107305.26646.20.camel@edumazet-glaptop2.roam.corp.google.com>
On Tue, Jan 07, 2014 at 07:08:25AM -0800, Eric Dumazet wrote:
> On Tue, 2014-01-07 at 14:31 +0400, Andrey Vagin wrote:
> > Lets look at destroy_conntrack:
> >
> > hlist_nulls_del_rcu(&ct->tuplehash[IP_CT_DIR_ORIGINAL].hnnode);
> > ...
> > nf_conntrack_free(ct)
> > kmem_cache_free(net->ct.nf_conntrack_cachep, ct);
> >
> > net->ct.nf_conntrack_cachep is created with SLAB_DESTROY_BY_RCU.
> >
> > The hash is protected by rcu, so readers look up conntracks without
> > locks.
> > A conntrack is removed from the hash, but in this moment a few readers
> > still can use the conntrack. Then this conntrack is released and another
> > thread creates conntrack with the same address and the equal tuple.
> > After this a reader starts to validate the conntrack:
> > * It's not dying, because a new conntrack was created
> > * nf_ct_tuple_equal() returns true.
> >
> > But this conntrack is not initialized yet, so it can not be used by two
> > threads concurrently. In this case BUG_ON may be triggered from
> > nf_nat_setup_info().
> >
> > Florian Westphal suggested to check the confirm bit too. I think it's
> > right.
> >
> > task 1 task 2 task 3
> > nf_conntrack_find_get
> > ____nf_conntrack_find
> > destroy_conntrack
> > hlist_nulls_del_rcu
> > nf_conntrack_free
> > kmem_cache_free
> > __nf_conntrack_alloc
> > kmem_cache_alloc
> > memset(&ct->tuplehash[IP_CT_DIR_MAX],
> > if (nf_ct_is_dying(ct))
> > if (!nf_ct_tuple_equal()
> >
> > I'm not sure, that I have ever seen this race condition in a real life.
> > Currently we are investigating a bug, which is reproduced on a few node.
> > In our case one conntrack is initialized from a few tasks concurrently,
> > we don't have any other explanation for this.
>
> >
> > Cc: Florian Westphal <fw@strlen.de>
> > Cc: Pablo Neira Ayuso <pablo@netfilter.org>
> > Cc: Patrick McHardy <kaber@trash.net>
> > Cc: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
> > Cc: "David S. Miller" <davem@davemloft.net>
> > Cc: Cyrill Gorcunov <gorcunov@openvz.org>
> > Signed-off-by: Andrey Vagin <avagin@openvz.org>
> > ---
> > net/netfilter/nf_conntrack_core.c | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
> > index 43549eb..7a34bb2 100644
> > --- a/net/netfilter/nf_conntrack_core.c
> > +++ b/net/netfilter/nf_conntrack_core.c
> > @@ -387,8 +387,12 @@ begin:
> > !atomic_inc_not_zero(&ct->ct_general.use)))
> > h = NULL;
> > else {
> > + /* A conntrack can be recreated with the equal tuple,
> > + * so we need to check that the conntrack is initialized
> > + */
> > if (unlikely(!nf_ct_tuple_equal(tuple, &h->tuple) ||
> > - nf_ct_zone(ct) != zone)) {
> > + nf_ct_zone(ct) != zone) ||
> > + !nf_ct_is_confirmed(ct)) {
> > nf_ct_put(ct);
> > goto begin;
> > }
>
> I do not think this is the right way to fix this problem (if said
> problem is confirmed)
>
> Remember the rule about SLAB_DESTROY_BY_RCU :
>
> When a struct is freed, then reused, its important to set the its refcnt
> (from 0 to 1) only when the structure is fully ready for use.
>
> If a lookup finds a structure which is not yet setup, the
> atomic_inc_not_zero() will fail.
>
> Take a look at sk_clone_lock() and Documentation/RCU/rculist_nulls.txt
>
I have one more question. Looks like I found another problem.
init_conntrack:
hlist_nulls_add_head_rcu(&ct->tuplehash[IP_CT_DIR_ORIGINAL].hnnode,
&net->ct.unconfirmed);
__nf_conntrack_hash_insert:
hlist_nulls_add_head_rcu(&ct->tuplehash[IP_CT_DIR_ORIGINAL].hnnode,
&net->ct.hash[hash]);
We use one hlist_nulls_node to add a conntrack into two different lists.
As far as I understand, it's unacceptable in case of
SLAB_DESTROY_BY_RCU.
Lets imagine that we have two threads. The first one enumerates objects
from a first list, the second one recreates an object and insert it in a
second list. If a first thread in this moment stays on the object, it
can read "next", when it's in the second list, so it will continue
to enumerate objects from the second list. It is not what we want, isn't
it?
cpu1 cpu2
hlist_nulls_for_each_entry_rcu(ct)
destroy_conntrack
kmem_cache_free
init_conntrack
hlist_nulls_add_head_rcu
ct = ct->next
^ permalink raw reply
* Re: [PATCH v4 net-next 2/4] sh_eth: Add support for r7s72100
From: Simon Horman @ 2014-01-09 5:03 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: David S. Miller, netdev, linux-sh, linux-arm-kernel, Magnus Damm
In-Reply-To: <52CDBBE1.1010301@cogentembedded.com>
On Wed, Jan 08, 2014 at 11:58:09PM +0300, Sergei Shtylyov wrote:
> On 01/08/2014 11:02 AM, Simon Horman wrote:
>
> >This is a fast ethernet controller.
>
> >Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
>
> [...]
>
> >diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
> >index 4b38533..cc6d4af 100644
> >--- a/drivers/net/ethernet/renesas/sh_eth.c
> >+++ b/drivers/net/ethernet/renesas/sh_eth.c
> >@@ -190,6 +190,59 @@ static const u16 sh_eth_offset_fast_rcar[SH_ETH_MAX_REGISTER_OFFSET] = {
> > [TRIMD] = 0x027c,
> > };
> >
> >+static const u16 sh_eth_offset_fast_rz[SH_ETH_MAX_REGISTER_OFFSET] = {
> [...]
> >+ [ECMR] = 0x0500,
> >+ [ECSR] = 0x0510,
> >+ [ECSIPR] = 0x0518,
> >+ [PIR] = 0x0520,
> >+ [APR] = 0x0554,
> >+ [MPR] = 0x0558,
> >+ [PFTCR] = 0x055c,
> >+ [PFRCR] = 0x0560,
> >+ [TPAUSER] = 0x0564,
> >+ [MAHR] = 0x05c0,
> >+ [MALR] = 0x05c8,
> >+ [CEFCR] = 0x0740,
> >+ [FRECR] = 0x0748,
> >+ [TSFRCR] = 0x0750,
> >+ [TLFRCR] = 0x0758,
> >+ [RFCR] = 0x0760,
> >+ [MAFCR] = 0x0778,
>
> You've missed RFLR @ 0x0508. It's a vital register which the
> driver requires to be always mapped.
Thanks, I will fix that.
> >+
> >+ [ARSTR] = 0x0000,
> >+ [TSU_CTRST] = 0x0004,
> >+ [TSU_VTAG0] = 0x0058,
> >+ [TSU_ADSBSY] = 0x0060,
> >+ [TSU_TEN] = 0x0064,
> >+ [TSU_ADRH0] = 0x0100,
> >+ [TSU_ADRL0] = 0x0104,
> >+ [TSU_ADRH31] = 0x01f8,
> >+ [TSU_ADRL31] = 0x01fc,
>
> Looking at the manual, you've missed [TR]X[NA]LCR regs starting
> at offset 0x0080 from TSU block.
Thanks, I will add them.
> I see that both E-MAC and TSU blocks turned out to be different
> from the Gigabit version upon further scrutiny...
>
> >+};
> >+
> > static const u16 sh_eth_offset_fast_sh4[SH_ETH_MAX_REGISTER_OFFSET] = {
> > [ECMR] = 0x0100,
> > [RFLR] = 0x0108,
> [...]
> >@@ -701,6 +762,35 @@ static struct sh_eth_cpu_data r8a7740_data = {
> > .shift_rd0 = 1,
> > };
> >
> >+/* R7S72100 */
> >+static struct sh_eth_cpu_data r7s72100_data = {
> >+ .chip_reset = sh_eth_chip_reset,
> >+ .set_duplex = sh_eth_set_duplex,
> >+
> >+ .register_type = SH_ETH_REG_FAST_RZ,
> >+
> >+ .ecsr_value = ECSR_ICD,
> >+ .ecsipr_value = ECSIPR_ICDIP,
> >+ .eesipr_value = 0xff7f009f,
> >+
> >+ .tx_check = EESR_TC1 | EESR_FTC,
> >+ .eesr_err_check = EESR_TWB1 | EESR_TWB | EESR_TABT | EESR_RABT |
> >+ EESR_RFE | EESR_RDE | EESR_RFRMER | EESR_TFE |
> >+ EESR_TDE | EESR_ECI,
> >+ .fdr_value = 0x0000070f,
> >+ .rmcr_value = RMCR_RNC,
> >+
> >+ .apr = 1,
> >+ .mpr = 1,
> >+ .tpauser = 1,
> >+ .hw_swap = 1,
> >+ .rpadir = 1,
> >+ .rpadir_value = 2 << 16,
> >+ .no_trimd = 1,
> >+ .tsu = 1,
> >+ .shift_rd0 = 1,
>
> Perhaps this field should be renamed to something talking about
> check summing support (since bits 0..15 of RD0 contain a frame check
> sum for those SoCs). Or maybe it should be just merged with the
> 'hw_crc' field...
I have no feelings about that one way or another.
But it seems to be orthogonal to this patch.
> Well, now the comments about your initializer: you've missed to
> set the 'no_psr' field -- this SoC doesn't have PSR (which usually
> holds the LINK signal status). It's not fatal since you're setting
> 'no_ether_link' in the platform data but should be fixed anyway.
> You've also missed to set 'no_ade' field, though 'eesipr_value'
> correctly has EESIPR.ADEIP cleared. And it looks like you've also
> missed to set 'hw_crc' field since this SoC has CSMR...
Thanks, I will add those fields.
> [...]
> >@@ -880,6 +970,8 @@ static unsigned long sh_eth_get_edtrr_trns(struct sh_eth_private *mdp)
> > {
> > if (sh_eth_is_gether(mdp))
> > return EDTRR_TRNS_GETHER;
> >+ else if (sh_eth_is_rz_fast_ether(mdp))
> >+ return EDTRR_TRNS_RZ_ETHER;
>
> I'd just merge this with the GEther case.
Sure, but in that case should we change the name.
As both you and Magnus pointed out to me, the rz is not gigabit.
>
> > else
> > return EDTRR_TRNS_ETHER;
> > }
> [...]
> >@@ -1062,7 +1155,8 @@ static void sh_eth_ring_format(struct net_device *ndev)
> > if (i == 0) {
> > /* Tx descriptor address set */
> > sh_eth_write(ndev, mdp->tx_desc_dma, TDLAR);
> >- if (sh_eth_is_gether(mdp))
> >+ if (sh_eth_is_gether(mdp) ||
> >+ sh_eth_is_rz_fast_ether(mdp))
> > sh_eth_write(ndev, mdp->tx_desc_dma, TDFAR);
>
> Hmm, TDFAR exists also on SH4 Ethers...
Lets fix that separately.
>
> [...]
> >@@ -2564,6 +2666,9 @@ static const u16 *sh_eth_get_register_offset(int register_type)
> > case SH_ETH_REG_FAST_RCAR:
> > reg_offset = sh_eth_offset_fast_rcar;
> > break;
> >+ case SH_ETH_REG_FAST_RZ:
> >+ reg_offset = sh_eth_offset_fast_rz;
> >+ break;
>
> I think it should precede the R-Car case as this chip is newer
> than R-Car and the SoC families appear here in the reverse order.
Sure, will do.
> > case SH_ETH_REG_FAST_SH4:
> > reg_offset = sh_eth_offset_fast_sh4;
> > break;
> [...]
> >diff --git a/drivers/net/ethernet/renesas/sh_eth.h b/drivers/net/ethernet/renesas/sh_eth.h
> >index 0fe35b7..0bcde90 100644
> >--- a/drivers/net/ethernet/renesas/sh_eth.h
> >+++ b/drivers/net/ethernet/renesas/sh_eth.h
> >@@ -156,6 +156,7 @@ enum {
> > enum {
> > SH_ETH_REG_GIGABIT,
> > SH_ETH_REG_FAST_RCAR,
> >+ SH_ETH_REG_FAST_RZ,
>
> I think it should precede the R-Car value.
Sure, will do.
> > SH_ETH_REG_FAST_SH4,
> > SH_ETH_REG_FAST_SH3_SH2
> > };
> >@@ -169,7 +170,7 @@ enum {
> >
> > /* Register's bits
> > */
> >-/* EDSR : sh7734, sh7757, sh7763, and r8a7740 only */
> >+/* EDSR : sh7734, sh7757, sh7763, r8a7740 and r7s72100 only */
>
> Need comma before "and". Sorry for the grammar nitpicking. :-)
Will do.
> > enum EDSR_BIT {
> > EDSR_ENT = 0x01, EDSR_ENR = 0x02,
> > };
> >@@ -191,6 +192,7 @@ enum DMAC_M_BIT {
> > /* EDTRR */
> > enum DMAC_T_BIT {
> > EDTRR_TRNS_GETHER = 0x03,
> >+ EDTRR_TRNS_RZ_ETHER = 0x03,
>
> I doubt we need a special case here. You didn't introduce one for
> the software reset bits.
True, but RZ is not Gigabit. So I think we either need two names
or to choose a more generic name.
>
> > EDTRR_TRNS_ETHER = 0x01,
> > };
>
> WBR, Sergei
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH net-next v2 3/4] virtio-net: auto-tune mergeable rx buffer size for improved performance
From: Michael Dalton @ 2014-01-09 3:41 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, lf-virt, Eric Dumazet, David S. Miller
In-Reply-To: <CANJ5vP+_aK0PMc6-hwbu6PduVNDe8jbWR19cN5C+k4mWQzdTGg@mail.gmail.com>
Sorry, forgot to mention - if we want to explore combining the buffer
address and truesize into a single void *, we could also exploit the
fact that our size ranges from aligned GOOD_PACKET_LEN to PAGE_SIZE, and
potentially encode fewer values for truesize (and require a smaller
alignment than 256). The prior e-mails discussion of 256 byte alignment
with 256 values is just one potential design point.
Best,
Mike
^ permalink raw reply
* Re: [PATCH 31/31] staging: r8188eu: Fix smatch error
From: Greg KH @ 2014-01-09 3:18 UTC (permalink / raw)
To: Larry Finger; +Cc: Dan Carpenter, devel, netdev
In-Reply-To: <52C5E5F5.8030200@lwfinger.net>
On Thu, Jan 02, 2014 at 04:19:33PM -0600, Larry Finger wrote:
> On 01/02/2014 02:44 AM, Dan Carpenter wrote:
> >On Sun, Dec 22, 2013 at 05:37:02PM -0600, Larry Finger wrote:
> >>Smatch shows the following:
> >>
> >> CHECK drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
> >>drivers/staging/rtl8188eu/core/rtw_mlme_ext.c:1401 OnAssocReq() error: buffer overflow 'pstapriv->sta_aid' 32 <= 32
> >>
> >
> >This is a false positive in Smatch. Don't do work arounds for buggy
> >tools.
> >
> >If you have the cross function database built on the latest version of
> >Smatch then I think it understands the code correctly and doesn't print
> >a warning.
>
> When I analyzed the loop in question, I thought it resulted in a subscript
> of [NUM_STA], but I now see that the largest one is [NUM_STA-1]. I will drop
> this patch in the next round.
>
> Thanks for the info regarding a new version of Smatch. I'll update now.
Ok, I've applied the first 30, but dropped this patch.
thanks,
greg k-h
^ 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 3:16 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, lf-virt, Eric Dumazet, David S. Miller
In-Reply-To: <20140109014255.GA19321@redhat.com>
Hi Michael,
On Wed, Jan 8, 2014 at 5:42 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> Sorry that I didn't notice early, but there seems to be a bug here.
> See below.
Yes, that is definitely a bug. Virtio spec permits OOO completions,
but current code assumes in-order completion. Thanks for catching this.
> Don't need full int really, it's up to 4K/cache line size,
> 1 byte would be enough, maximum 2 ...
> So if all we want is extra 1-2 bytes per buffer, we don't really
> need this extra level of indirection I think.
> We can just allocate them before the header together with an skb.
I'm not sure if I'm parsing the above correctly, but do you mean using a
few bytes at the beginning of the packet buffer to store truesize? I
think that will break Jason's virtio-net RX frag coalescing
code. To coalesce consecutive RX packet buffers, our packet buffers must
be physically adjacent, and any extra bytes before the start of the
buffer would break that.
We could allocate an SKB per packet buffer, but if we have multi-buffer
packets often(e.g., netperf benefiting from GSO/GRO), we would be
allocating 1 SKB per packet buffer instead of 1 SKB per MAX_SKB_FRAGS
buffers. How do you feel about any of the below alternatives:
(1) Modify the existing mrg_buf_ctx to chain together free entries
We can use the 'buf' pointer in mergeable_receive_buf_ctx to chain
together free entries so that we can support OOO completions. This would
be similar to how virtio-queue manages free sg entries.
(2) Combine the buffer pointer and truesize into a single void* value
Your point about there only being a byte needed to encode truesize is
spot on, and I think we could leverage this to eliminate the out-of-band
metadata ring entirely. If we were willing to change the packet buffer
alignment from L1_CACHE_BYTES to 256 (or min (256, L1_CACHE_SIZE)), we
could encode the truesize in the least significant 8 bits of the buffer
address (encoded as truesize >> 8 as we know all sizes are a multiple
of 256). This would allow packet buffers up to 64KB in length.
Is there another approach you would prefer to any of these? If the
cleanliness issues and larger alignment aren't too bad, I think (2)
sounds promising and allow us to eliminate the metadata ring
entirely while still permitting RX frag coalescing.
Best,
Mike
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox