* [PATCH net-next 0/4] mlx5 update for 3.18
From: Eli Cohen @ 2014-09-30 13:02 UTC (permalink / raw)
To: davem; +Cc: netdev, ogerlitz, yevgenyp, Eli Cohen
In-Reply-To: <1412082135-30229-1-git-send-email-eli@mellanox.com>
Hi Dave,
This series integrates a new mechanism for populating and extracting field values
used in the driver/firmware interaction around command mailboxes.
Eli.
Eli Cohen (4):
net/mlx5_core: Update device capabilities handling
net/mlx5_core: Use hardware registers description header file
net/mlx5_core: use set/get macros in device caps
net/mlx5_core: Identify resources by their type
drivers/infiniband/hw/mlx5/cq.c | 8 +-
drivers/infiniband/hw/mlx5/mad.c | 2 +-
drivers/infiniband/hw/mlx5/main.c | 83 +-
drivers/infiniband/hw/mlx5/qp.c | 72 +-
drivers/infiniband/hw/mlx5/srq.c | 6 +-
drivers/net/ethernet/mellanox/mlx5/core/cmd.c | 77 +-
drivers/net/ethernet/mellanox/mlx5/core/eq.c | 14 +-
drivers/net/ethernet/mellanox/mlx5/core/fw.c | 81 +-
drivers/net/ethernet/mellanox/mlx5/core/main.c | 229 +-
drivers/net/ethernet/mellanox/mlx5/core/qp.c | 60 +-
drivers/net/ethernet/mellanox/mlx5/core/uar.c | 4 +-
include/linux/mlx5/device.h | 182 +-
include/linux/mlx5/driver.h | 118 +-
include/linux/mlx5/mlx5_ifc.h | 4464 ++++++++++++++++++++++++
include/linux/mlx5/qp.h | 3 +-
15 files changed, 4948 insertions(+), 455 deletions(-)
create mode 100644 include/linux/mlx5/mlx5_ifc.h
--
2.1.1
^ permalink raw reply
* [PATCH net-next 0/4] mlx5 update for 3.18
From: Eli Cohen @ 2014-09-30 13:02 UTC (permalink / raw)
To: davem; +Cc: netdev, ogerlitz, yevgenyp, Eli Cohen
Hi Dave,
This series integrates a new mechanism for populating and extracting field values
used in the driver/firmware interaction around command mailboxes.
Eli.
Eli Cohen (4):
net/mlx5_core: Update device capabilities handling
net/mlx5_core: Use hardware registers description header file
net/mlx5_core: use set/get macros in device caps
net/mlx5_core: Identify resources by their type
drivers/infiniband/hw/mlx5/cq.c | 8 +-
drivers/infiniband/hw/mlx5/mad.c | 2 +-
drivers/infiniband/hw/mlx5/main.c | 83 +-
drivers/infiniband/hw/mlx5/qp.c | 72 +-
drivers/infiniband/hw/mlx5/srq.c | 6 +-
drivers/net/ethernet/mellanox/mlx5/core/cmd.c | 77 +-
drivers/net/ethernet/mellanox/mlx5/core/eq.c | 14 +-
drivers/net/ethernet/mellanox/mlx5/core/fw.c | 81 +-
drivers/net/ethernet/mellanox/mlx5/core/main.c | 229 +-
drivers/net/ethernet/mellanox/mlx5/core/qp.c | 60 +-
drivers/net/ethernet/mellanox/mlx5/core/uar.c | 4 +-
include/linux/mlx5/device.h | 182 +-
include/linux/mlx5/driver.h | 118 +-
include/linux/mlx5/mlx5_ifc.h | 4464 ++++++++++++++++++++++++
include/linux/mlx5/qp.h | 3 +-
15 files changed, 4948 insertions(+), 455 deletions(-)
create mode 100644 include/linux/mlx5/mlx5_ifc.h
--
2.1.1
^ permalink raw reply
* Re: [PATCH] net: ethernet : stmicro: fixed power suspend and resume failure in stmmac driver
From: Giuseppe CAVALLARO @ 2014-09-30 13:00 UTC (permalink / raw)
To: hliang1025; +Cc: netdev, linux-kernel, adi-linux-docs
In-Reply-To: <1412056534-7995-1-git-send-email-hliang1025@gmail.com>
Hello Hao Liang
On 9/30/2014 7:55 AM, hliang1025@gmail.com wrote:
> From: Hao Liang <hliang1025@gmail.com>
>
> This is the fix for a power management issue caused by suspend and resume function in stmmac_main.c.
> After enable CONFIG_DEBUG_ATOMIC_SLEEP which enable sleep-inside atomic section checking, power
> managemet can not work normally. Board couldn't wakeup successfully after suspend. Command
> "echo mem > /sys/power/state" suspend the board.
>
> In suspend and resume function of stmmac driver, there are some sleep-inside function in atomic section
> created by spin lock. These functions will causes system warnings and wakeup issue when enable
> CONFIG_DEBUG_ATOMIC_SLEEP.
>
> This bug was fixed by:
> * replace some sleep function with non-sleep function
> clk_disable_unprepare -> clk_disable ...
> * decrease the atomic area created by spin lock function. The original atomic area in resume function is
> too large.
I had done something in this patch:
[PATCH (net.git)] stmmac: fix and review whole driver locking
Indeed I was not able to review it after some advice but I will do it!
So we can review your patch and then I will re-base my work on top of
yours.
I just wonder if the spinlock can be safely removed instead of
surrounding just the code to clear the descriptors that should
not be claimed by others. Maybe the only part shared is the
stmmac_hw_setup that can be invoked by open method.
peppe
> Signed-off-by: Hao Liang <hliang1025@gmail.com>
> ---
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 6e6ee22..2effbfe 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -2924,9 +2924,8 @@ int stmmac_suspend(struct net_device *ndev)
> if (priv->phydev)
> phy_stop(priv->phydev);
>
> - spin_lock_irqsave(&priv->lock, flags);
> -
> netif_device_detach(ndev);
> +
> netif_stop_queue(ndev);
>
> napi_disable(&priv->napi);
> @@ -2935,6 +2934,8 @@ int stmmac_suspend(struct net_device *ndev)
> priv->hw->dma->stop_tx(priv->ioaddr);
> priv->hw->dma->stop_rx(priv->ioaddr);
>
> + spin_lock_irqsave(&priv->lock, flags);
> +
> stmmac_clear_descriptors(priv);
>
> /* Enable Power down mode by programming the PMT regs */
> @@ -2945,7 +2946,7 @@ int stmmac_suspend(struct net_device *ndev)
> stmmac_set_mac(priv->ioaddr, false);
> pinctrl_pm_select_sleep_state(priv->device);
> /* Disable clock in case of PWM is off */
> - clk_disable_unprepare(priv->stmmac_clk);
> + clk_disable(priv->stmmac_clk);
> }
> spin_unlock_irqrestore(&priv->lock, flags);
>
> @@ -2977,12 +2978,14 @@ int stmmac_resume(struct net_device *ndev)
> } else {
> pinctrl_pm_select_default_state(priv->device);
> /* enable the clk prevously disabled */
> - clk_prepare_enable(priv->stmmac_clk);
> + clk_enable(priv->stmmac_clk);
> /* reset the phy so that it's ready */
> if (priv->mii)
> stmmac_mdio_reset(priv->mii);
> }
>
> + spin_unlock_irqrestore(&priv->lock, flags);
> +
> netif_device_attach(ndev);
>
> stmmac_hw_setup(ndev);
> @@ -2991,8 +2994,6 @@ int stmmac_resume(struct net_device *ndev)
>
> netif_start_queue(ndev);
>
> - spin_unlock_irqrestore(&priv->lock, flags);
> -
> if (priv->phydev)
> phy_start(priv->phydev);
>
>
^ permalink raw reply
* How exactly does CHECKSUM_COMPLETE works?
From: Yuval Mintz @ 2014-09-30 12:51 UTC (permalink / raw)
To: Tom Herbert (Partner - google), netdev
Hi,
I was pondering what it would take for a network device support
CHECKSUM_COMPLETE on its Rx path. And I simply failed to understand the
specifics when using the napi_gro_receive() interface.
Scenario 1 - TCP/IPv4 packet. This will eventually reach tcp4_gro_receive(), and from
there into skb_gro_checksum_validate().
Since this SKB is marked as COMPLETE, the pseudo TCP checksum will be computed
and skb_gro_checksum_validate_complete() be called.
Now, it seems to me as if the only way by which the flow won't actually try to
re-calculate the checksum over TCP header and payload is if NAPI_GRO_CB(skb)->csum
is actually the one's complement to the TCP pseudo header. But that sounds.... odd,
since that's the csum that should have been listed on the SKB - and it doesn't protect
any of the data, only the "metadata".
Scenario 2 - IP-GRE packet, with CSUM option on the GRE header.
This scenario will also eventually reach skb_gro_checksum_validate_complete(),
this time via gre_gro_receive().
This one is even odder, since in order for the flow not to re-calculate the checksum
The driver should have set the SKB's csum to 0xffffffff [as it's using the
null_compute_pseudo() which will return a pseudo_csum of 0].
>From the comment in skbuff.h regarding CHECKSUM_COMPLETE it looks as if all
the HW needs to do is calculate the checksum over the entire data, starting at the
network header.
So, to summarize my questions -
1. What should a driver set as the SKBs csum value when passing CHECKSUM_COMPLETE?
2. Does the GRO flow for non-encapsulated packets work for packets marked as
CHECKSUM_COMPLETE? And by work, I obviously mean 'doesn't re-calculate checksums'.
3. Same as question 2, but for encapsulated packets.
Thanks,
Yuval
________________________________
This message and any attached documents contain information from QLogic Corporation or its wholly-owned subsidiaries that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.
^ permalink raw reply
* [net-next PATCH] dql: add a burst attribute
From: Eric Dumazet @ 2014-09-30 12:51 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: netdev, David S. Miller, Tom Herbert, Hannes Frederic Sowa,
Florian Westphal, Daniel Borkmann, Jamal Hadi Salim,
Alexander Duyck, John Fastabend, Dave Taht, toke
In-Reply-To: <1412080482.30721.68.camel@edumazet-glaptop2.roam.corp.google.com>
From: Eric Dumazet <edumazet@google.com>
Add a new dql attribute, to control how much packets we are allowed to
burst from qdisc to device.
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
Jesper, please consider using this instead of 7 or 8 values you hard coded.
include/linux/dynamic_queue_limits.h | 1
lib/dynamic_queue_limits.c | 1
net/core/net-sysfs.c | 31 +++++++++++++++++++++++++
3 files changed, 33 insertions(+)
diff --git a/include/linux/dynamic_queue_limits.h b/include/linux/dynamic_queue_limits.h
index a4be70398ce1..8509053eb330 100644
--- a/include/linux/dynamic_queue_limits.h
+++ b/include/linux/dynamic_queue_limits.h
@@ -42,6 +42,7 @@ struct dql {
unsigned int num_queued; /* Total ever queued */
unsigned int adj_limit; /* limit + num_completed */
unsigned int last_obj_cnt; /* Count at last queuing */
+ unsigned int burst;
/* Fields accessed only by completion path (dql_completed) */
diff --git a/lib/dynamic_queue_limits.c b/lib/dynamic_queue_limits.c
index 0777c5a45fa0..acc0d6105191 100644
--- a/lib/dynamic_queue_limits.c
+++ b/lib/dynamic_queue_limits.c
@@ -132,6 +132,7 @@ int dql_init(struct dql *dql, unsigned hold_time)
dql->max_limit = DQL_MAX_LIMIT;
dql->min_limit = 0;
dql->slack_hold_time = hold_time;
+ dql->burst = 7;
dql_reset(dql);
return 0;
}
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 9dd06699b09c..80cf6f166706 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -978,6 +978,36 @@ static struct netdev_queue_attribute bql_hold_time_attribute =
__ATTR(hold_time, S_IRUGO | S_IWUSR, bql_show_hold_time,
bql_set_hold_time);
+static ssize_t bql_show_burst(struct netdev_queue *queue,
+ struct netdev_queue_attribute *attr,
+ char *buf)
+{
+ const struct dql *dql = &queue->dql;
+
+ return sprintf(buf, "%u\n", dql->burst);
+}
+
+static ssize_t bql_set_burst(struct netdev_queue *queue,
+ struct netdev_queue_attribute *attribute,
+ const char *buf, size_t len)
+{
+ struct dql *dql = &queue->dql;
+ unsigned int value;
+ int err;
+
+ err = kstrtouint(buf, 10, &value);
+ if (err < 0)
+ return err;
+
+ dql->burst = value;
+
+ return len;
+}
+
+static struct netdev_queue_attribute bql_burst_attribute =
+ __ATTR(burst, S_IRUGO | S_IWUSR, bql_show_burst,
+ bql_set_burst);
+
static ssize_t bql_show_inflight(struct netdev_queue *queue,
struct netdev_queue_attribute *attr,
char *buf)
@@ -1019,6 +1049,7 @@ static struct attribute *dql_attrs[] = {
&bql_limit_min_attribute.attr,
&bql_hold_time_attribute.attr,
&bql_inflight_attribute.attr,
+ &bql_burst_attribute.attr,
NULL
};
^ permalink raw reply related
* Re: [net-next PATCH V5] qdisc: bulk dequeue support for qdiscs with TCQ_F_ONETXQUEUE
From: Florian Westphal @ 2014-09-30 12:41 UTC (permalink / raw)
To: Eric Dumazet
Cc: Florian Westphal, Jesper Dangaard Brouer, netdev, David S. Miller,
Tom Herbert, Hannes Frederic Sowa, Daniel Borkmann,
Jamal Hadi Salim, Alexander Duyck, John Fastabend, Dave Taht,
toke
In-Reply-To: <1412080721.30721.72.camel@edumazet-glaptop2.roam.corp.google.com>
Eric Dumazet <eric.dumazet@gmail.com> wrote:
> I guess my question was more like : Should I wait you sending patches,
> or should I do this ?
Sure, if you have time to work on this go ahead.
Thanks Eric!
^ permalink raw reply
* Re: [net-next PATCH V5] qdisc: bulk dequeue support for qdiscs with TCQ_F_ONETXQUEUE
From: Eric Dumazet @ 2014-09-30 12:38 UTC (permalink / raw)
To: Florian Westphal
Cc: Jesper Dangaard Brouer, netdev, David S. Miller, Tom Herbert,
Hannes Frederic Sowa, Daniel Borkmann, Jamal Hadi Salim,
Alexander Duyck, John Fastabend, Dave Taht, toke
In-Reply-To: <20140930122509.GF11709@breakpoint.cc>
On Tue, 2014-09-30 at 14:25 +0200, Florian Westphal wrote:
> You're right, but this is not related to this patch specifically.
> But sure, we can look into moving validate_xmit_skb calls outside of the
> qdisc root locked sections.
Yes please.
>
> > 2) TSO support. Have you ideas how to perform this ?
>
> Yes, the idea was to just remove the skb_is_gso() test and limit
> dql_avail() result to e.g. 128k to avoid a '8-fat-gso-skbs-in-a-row' scenario.
Really I think you are mistaken about GSO/TSO being fat skb. This is not
a qdisc concern, but packets producer. Think of TSO autosizing which
already takes care of this.
>
> Did you have any other ideas in mind?
I guess my question was more like : Should I wait you sending patches,
or should I do this ?
We are approaching merge window, and I would like to get this sorted out
for linux-3.18
Thanks
^ permalink raw reply
* Re: [net-next PATCH V5] qdisc: bulk dequeue support for qdiscs with TCQ_F_ONETXQUEUE
From: Eric Dumazet @ 2014-09-30 12:34 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: netdev, David S. Miller, Tom Herbert, Hannes Frederic Sowa,
Florian Westphal, Daniel Borkmann, Jamal Hadi Salim,
Alexander Duyck, John Fastabend, Dave Taht, toke
In-Reply-To: <20140930085114.24043.81310.stgit@dragon>
On Tue, 2014-09-30 at 10:53 +0200, Jesper Dangaard Brouer wrote:
> For now, as a conservative approach, don't bulk dequeue GSO and
> segmented GSO packets, because testing showed requeuing occuring
> with segmented GSO packets.
Note that GSO happens after qdisc dequeue.
We should not care if part of a GSO train is stopped because TX ring is
full. If we stop a GSO train, then we only lower GRO aggregation for the
receiver.
Normally, if BQL is properly working, we should practically not hit TX
ring buffer limit, unless you send very small packets just to stress the
thing (synthetic benchmark, not a real workload)
We should not stop a GSO train because of BQL : Remind that BQL is to
avoid head of line blocking, but there is no way another high prio
packet can be sent on the wire before one if the packet resulting of GSO
segments.
So telling that requeueing is happening with GSO is kind of irrelevant.
^ permalink raw reply
* Re: [net-next PATCH V5] qdisc: bulk dequeue support for qdiscs with TCQ_F_ONETXQUEUE
From: Florian Westphal @ 2014-09-30 12:25 UTC (permalink / raw)
To: Eric Dumazet
Cc: Jesper Dangaard Brouer, netdev, David S. Miller, Tom Herbert,
Hannes Frederic Sowa, Florian Westphal, Daniel Borkmann,
Jamal Hadi Salim, Alexander Duyck, John Fastabend, Dave Taht,
toke
In-Reply-To: <1412077736.30721.54.camel@edumazet-glaptop2.roam.corp.google.com>
Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > struct sk_buff *skb = q->gso_skb;
> > @@ -70,10 +106,17 @@ static inline struct sk_buff *dequeue_skb(struct Qdisc *q)
> > } else
> > skb = NULL;
> > } else {
> > - if (!(q->flags & TCQ_F_ONETXQUEUE) || !netif_xmit_frozen_or_stopped(txq)) {
> > + if (!(q->flags & TCQ_F_ONETXQUEUE) ||
> > + !netif_xmit_frozen_or_stopped(txq)) {
> > + int bytelimit = qdisc_avail_bulklimit(txq);
> > +
> > skb = q->dequeue(q);
> > - if (skb)
> > + if (skb) {
> > + bytelimit -= skb->len;
> > skb = validate_xmit_skb(skb, qdisc_dev(q));
> > + }
> > + if (skb && qdisc_may_bulk(q))
> > + skb = try_bulk_dequeue_skb(q, skb, bytelimit);
> > }
> > }
>
> Okay...
>
> In my opinion, you guys missed some opportunities :
>
> 1) Ideally the algo should try to call validate_xmit_skb() outside of
> the locks (qdisc lock or device lock), because eventually doing checksum
> computation or full segmentation should allow other cpus doing enqueues.
You're right, but this is not related to this patch specifically.
But sure, we can look into moving validate_xmit_skb calls outside of the
qdisc root locked sections.
> 2) TSO support. Have you ideas how to perform this ?
Yes, the idea was to just remove the skb_is_gso() test and limit
dql_avail() result to e.g. 128k to avoid a '8-fat-gso-skbs-in-a-row' scenario.
Did you have any other ideas in mind?
Thanks,
Florian
^ permalink raw reply
* Re: [PATCH v2 net-next] mlx4: optimize xmit path
From: Eric Dumazet @ 2014-09-30 12:11 UTC (permalink / raw)
To: Amir Vadai
Cc: Or Gerlitz, Alexei Starovoitov, David S. Miller,
Jesper Dangaard Brouer, Eric Dumazet, John Fastabend,
Linux Netdev List, Or Gerlitz
In-Reply-To: <542A9BAC.6010208@mellanox.com>
On Tue, 2014-09-30 at 15:01 +0300, Amir Vadai wrote:
> Hi,
>
> Thanks for the great work Eric.
>
> I'm working to split the patch and pass it through some regression tests
> and performance benchmarking.
> Will send the split patch-set in few days.
This looks great, thanks Amir.
^ permalink raw reply
* Re: [PATCH] tun: make sure interface usage can not overflow
From: Hannes Frederic Sowa @ 2014-09-30 12:03 UTC (permalink / raw)
To: David Laight
Cc: Kees Cook, linux-kernel@vger.kernel.org, David S. Miller,
Jason Wang, Zhi Yong Wu, Michael S. Tsirkin, Tom Herbert,
Masatake YAMATO, Xi Wang, stephen hemminger,
netdev@vger.kernel.org
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6D174A0ECF@AcuExch.aculab.com>
On Di, 2014-09-30 at 11:18 +0000, David Laight wrote:
> From: Hannes Frederic
> > On Di, 2014-09-30 at 08:20 +0000, David Laight wrote:
> > > From: Hannes Frederic
> > > > On Mo, 2014-09-29 at 12:41 -0700, Kees Cook wrote:
> > > > > On Mon, Sep 29, 2014 at 4:04 AM, David Laight <David.Laight@aculab.com> wrote:
> > > > > > From: Kees Cook
> > > > > >> This makes the size argument a const, since it is always populated by
> > > > > >> the caller.
> > > > > >
> > > > > > There is almost no point making parameters 'const.
> > > > > > ('const foo *' makes sense).
> > > > > >
> > > > > >> Additionally double-checks to make sure the copy_from_user
> > > > > >> can never overflow, keeping CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy:
> > > > > >>
> > > > > >> In function 'copy_from_user',
> > > > > >> inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1871:7:
> > > > > >> ... copy_from_user() buffer size is not provably correct
> > > > > >
> > > > > > If 'ifreq_len' could be too big then you want to error the ioctl, not panic.
> > > > > > If it can't be too big you don't need the check.
> > > > >
> > > > > The ifreq_len comes from the callers, and is the output of "sizeof"
> > > > > which is const. Changing the function parameter to "const" means any
> > > > > changes made in the future where the incoming value isn't const, the
> > > > > compiler will throw a warning.
> > > >
> > > > Hmmm, I think you want something like BUILD_BUG_ON(!
> > > > __builtin_constant_p(var)). const in function argument only ensures that
> > > > the value cannot be modified in the function.
> > >
> > > You'd have to do something in the header file - nothing in the function
> > > body can do that check.
> >
> > Sure, it should work. You only need to make sure that gcc inlines the
> > function, so the value is constant (it is not enough that gcc knows the
> > value range, one specific constant is needed). So the simplest fix for
> > this is to specify __tun_chr_ioctl as __always_inline. ;)
>
> You are joking aren't you???
It would propagate the value from the compat and non-compat helper
functions to __tun_chr_ioctl and the __builtin_constant_p checks would
be true in the inlined copy_from/to_user, so no, I was not joking, but
because of my smiley I didn't considered this fix seriously.
> Look at the code.
I did.
> I'd suggest fixing whatever implements CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
> to be less picky.
This might involve gcc changes. E.g. on my gcc I don't get any errors.
But by looking at the code it might be plausible wrong estimations are
being made.
But somehow I really cannot follow the original bug report. Looks like
Kees' __builtin_object_size is broken?
I also checked my tun.i file, I really use gcc __builtin_object_size.
Strange...
Bye,
Hannes
^ permalink raw reply
* Re: [PATCH v2 net-next] mlx4: optimize xmit path
From: Amir Vadai @ 2014-09-30 12:01 UTC (permalink / raw)
To: Eric Dumazet, Or Gerlitz
Cc: Alexei Starovoitov, David S. Miller, Jesper Dangaard Brouer,
Eric Dumazet, John Fastabend, Linux Netdev List, Or Gerlitz
In-Reply-To: <1411964353.30721.6.camel@edumazet-glaptop2.roam.corp.google.com>
On 9/29/2014 7:19 AM, Eric Dumazet wrote:
> First I implemented skb->xmit_more support, and pktgen throughput
> went from ~5Mpps to ~10Mpps.
>
> Then, looking closely at this driver I found false sharing problems that
> should be addressed by this patch, as my pktgen now reaches 14.7 Mpps
> on a single TX queue, with a burst factor of 8.
>
> So this patch in a whole permits to improve raw performance on a single
> TX queue from about 5 Mpps to 14.9 Mpps.
>
> Note that if packets are below the inline_thold threshold (104 bytes),
> driver copies packets content into tx descriptor, and throughput
> is lowered to ~7.5 Mpps :
> -> We might reconsider inline strategy in a followup patch.
>
> I could split this patch into multiple components, but I prefer not
> spend days on this work.
Hi,
Thanks for the great work Eric.
I'm working to split the patch and pass it through some regression tests
and performance benchmarking.
Will send the split patch-set in few days.
Amir
^ permalink raw reply
* Re: Re: [PATCH net] gro: fix aggregation for skb using frag_list
From: Erez Shitrit @ 2014-09-30 11:51 UTC (permalink / raw)
To: erezsh, davem, eric.dumazet; +Cc: netdev
In-Reply-To: <07015a18f26f49518ceeabb35f2539ee@DB3PR05MB441.eurprd05.prod.outlook.com>
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Monday, September 29, 2014 10:19 PM
> To: eric.dumazet@gmail.com
> Cc: netdev@vger.kernel.org; Erez Shitrit
> Subject: Re: [PATCH net] gro: fix aggregation for skb using frag_list
>
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Mon, 29 Sep 2014 10:34:29 -0700
>
>> From: Eric Dumazet <edumazet@google.com>
>>
>> In commit 8a29111c7ca6 ("net: gro: allow to build full sized skb") I
>> added a regression for linear skb that traditionally force GRO to use
>> the frag_list fallback.
>>
>> Erez Shitrit found that at most two segments were aggregated and the
>> "if (skb_gro_len(p) != pinfo->gso_size)" test was failing.
>>
>> This is because pinfo at this spot still points to the last skb in the
>> chain, instead of the first one, where we find the correct gso_size
>> information.
>>
>> Signed-off-by: Eric Dumazet <edumazet@google.com>
>> Fixes: 8a29111c7ca6 ("net: gro: allow to build full sized skb")
>> Reported-by: Erez Shitrit <erezsh@mellanox.com>
> Applied and queued up for -stable, thanks Eric.
>
Tested. and it works for IPoIB driver. Thanks Eric.
Erez
^ permalink raw reply
* Re: [net-next PATCH V5] qdisc: bulk dequeue support for qdiscs with TCQ_F_ONETXQUEUE
From: Eric Dumazet @ 2014-09-30 11:48 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: netdev, David S. Miller, Tom Herbert, Hannes Frederic Sowa,
Florian Westphal, Daniel Borkmann, Jamal Hadi Salim,
Alexander Duyck, John Fastabend, Dave Taht, toke
In-Reply-To: <20140930085114.24043.81310.stgit@dragon>
On Tue, 2014-09-30 at 10:53 +0200, Jesper Dangaard Brouer wrote:
> Based on DaveM's recent API work on dev_hard_start_xmit(), that allows
> sending/processing an entire skb list.
>
> This patch implements qdisc bulk dequeue, by allowing multiple packets
> to be dequeued in dequeue_skb().
>
> The optimization principle for this is two fold, (1) to amortize
> locking cost and (2) avoid expensive tailptr update for notifying HW.
> (1) Several packets are dequeued while holding the qdisc root_lock,
> amortizing locking cost over several packet. The dequeued SKB list is
> processed under the TXQ lock in dev_hard_start_xmit(), thus also
> amortizing the cost of the TXQ lock.
> (2) Further more, dev_hard_start_xmit() will utilize the skb->xmit_more
> API to delay HW tailptr update, which also reduces the cost per
> packet.
>
> One restriction of the new API is that every SKB must belong to the
> same TXQ. This patch takes the easy way out, by restricting bulk
> dequeue to qdisc's with the TCQ_F_ONETXQUEUE flag, that specifies the
> qdisc only have attached a single TXQ.
>
> Some detail about the flow; dev_hard_start_xmit() will process the skb
> list, and transmit packets individually towards the driver (see
> xmit_one()). In case the driver stops midway in the list, the
> remaining skb list is returned by dev_hard_start_xmit(). In
> sch_direct_xmit() this returned list is requeued by dev_requeue_skb().
>
> To avoid overshooting the HW limits, which results in requeuing, the
> patch limits the amount of bytes dequeued, based on the drivers BQL
> limits. In-effect bulking will only happen for BQL enabled drivers.
> Besides the bytelimit from BQL, also limit bulking to maximum 7
> packets to avoid any issues with available descriptor in HW and
> any HoL issues measured at 100Mbit/s.
>
> For now, as a conservative approach, don't bulk dequeue GSO and
> segmented GSO packets, because testing showed requeuing occuring
> with segmented GSO packets.
>
> Jointed work with Hannes, Daniel and Florian.
>
> Testing:
> --------
> Demonstrating the performance improvement of qdisc dequeue bulking, is
> tricky because the effect only "kicks-in" once the qdisc system have a
> backlog. Thus, for a backlog to form, we need either 1) to exceed wirespeed
> of the link or 2) exceed the capability of the device driver.
>
> For practical use-cases, the measureable effect of this will be a
> reduction in CPU usage
>
> 01-TCP_STREAM:
> --------------
> Testing effect for TCP involves disabling TSO and GSO, because TCP
> already benefit from bulking, via TSO and especially for GSO segmented
> packets. This patch view TSO/GSO as a seperate kind of bulking, and
> avoid further bulking of these packet types.
>
> The measured perf diff benefit (at 10Gbit/s) for TCP_STREAM were 4.66%
> less CPU used on calls to _raw_spin_lock() (mostly from sch_direct_xmit).
>
> Tool mpstat, while stressing the system with netperf 24x TCP_STREAM, shows:
> * Disabled bulking: 8.30% soft 88.75% idle
> * Enabled bulking: 7.80% soft 89.36% idle
>
> 02-UDP_STREAM
> -------------
> The measured perf diff benefit for UDP_STREAM were 5.26% less CPU used
> on calls to _raw_spin_lock(). UDP_STREAM with packet size -m 1472 (to
> avoid sending UDP/IP fragments).
>
> 03-trafgen driver test
> ----------------------
> The performance of the 10Gbit/s ixgbe driver is limited due to
> updating the HW ring-queue tail-pointer on every packet. As
> previously demonstrated with pktgen.
>
> Using trafgen to send RAW frames from userspace (via AF_PACKET), and
> forcing it through qdisc path (with option --qdisc-path and -t0),
> sending with 12 CPUs.
>
> I can demonstrate this driver layer limitation:
> * 12.8 Mpps with no qdisc bulking
> * 14.8 Mpps with qdisc bulking (full 10G-wirespeed)
>
> 04-netperf-wrapper
> ------------------
> I've designed some tests for the tool "netperf-wrapper" that tries to
> measure the Head-of-Line (HoL) blocking effect.
>
> The bulking "budget" limit of 7 packets are choosen, based on testing
> with netperf-wrapper. With driver igb at 100Mbit/s and a budget of
> max 7 packets I cannot measure any added HoL latency. Increasing this
> value to 8 packets, I observed a fluctuating added 0.24ms delay
> corrosponding to 3000 bytes at 100Mbit/s.
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> Signed-off-by: Florian Westphal <fw@strlen.de>
>
> ---
> V5:
> - Changed "budget" limit to 7 packets (from 8)
> - Choosing to use skb->len, because BQL uses that
> - Added testing results to patch desc
>
> V4:
> - Patch rewritten in the Red Hat Neuchatel office jointed work with
> Hannes, Daniel and Florian.
> - Conservative approach of only using on BQL enabled drivers
> - No user tunable parameter, but limit bulking to 8 packets.
> - For now, avoid bulking GSO packets packets.
>
> V3:
> - Correct use of BQL
> - Some minor adjustments based on feedback.
> - Default setting only bulk dequeue 1 extra packet (2 packets).
>
> V2:
> - Restruct functions, split out functionality
> - Use BQL bytelimit to avoid overshooting driver limits
>
>
> include/net/sch_generic.h | 16 +++++++++++++++
> net/sched/sch_generic.c | 47 +++++++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 61 insertions(+), 2 deletions(-)
>
> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index e65b8e0..0654dfa 100644
> --- a/include/net/sch_generic.h
> +++ b/include/net/sch_generic.h
> @@ -6,6 +6,7 @@
> #include <linux/rcupdate.h>
> #include <linux/pkt_sched.h>
> #include <linux/pkt_cls.h>
> +#include <linux/dynamic_queue_limits.h>
> #include <net/gen_stats.h>
> #include <net/rtnetlink.h>
>
> @@ -111,6 +112,21 @@ static inline void qdisc_run_end(struct Qdisc *qdisc)
> qdisc->__state &= ~__QDISC___STATE_RUNNING;
> }
>
> +static inline bool qdisc_may_bulk(const struct Qdisc *qdisc)
> +{
> + return qdisc->flags & TCQ_F_ONETXQUEUE;
> +}
> +
> +static inline int qdisc_avail_bulklimit(const struct netdev_queue *txq)
> +{
> +#ifdef CONFIG_BQL
> + /* Non-BQL migrated drivers will return 0, too. */
> + return dql_avail(&txq->dql);
> +#else
> + return 0;
> +#endif
> +}
> +
> static inline bool qdisc_is_throttled(const struct Qdisc *qdisc)
> {
> return test_bit(__QDISC_STATE_THROTTLED, &qdisc->state) ? true : false;
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index 11b28f6..0fa8f0e 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -56,6 +56,42 @@ static inline int dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
> return 0;
> }
>
> +static struct sk_buff *try_bulk_dequeue_skb(struct Qdisc *q,
> + struct sk_buff *head_skb,
> + int bytelimit)
> +{
> + struct sk_buff *skb, *tail_skb = head_skb;
> + int budget = 7; /* For now, conservatively choosen limit */
> +
> + while (bytelimit > 0 && --budget > 0) {
> + /* For now, don't bulk dequeue GSO (or GSO segmented) pkts */
> + if (tail_skb->next || skb_is_gso(tail_skb))
> + break;
> +
> + skb = q->dequeue(q);
> + if (!skb)
> + break;
> +
> + bytelimit -= skb->len; /* covers GSO len */
> + skb = validate_xmit_skb(skb, qdisc_dev(q));
> + if (!skb)
> + break;
> +
> + /* "skb" can be a skb list after validate call above
> + * (GSO segmented), but it is okay to append it to
> + * current tail_skb->next, because next round will exit
> + * in-case "tail_skb->next" is a skb list.
> + */
> + tail_skb->next = skb;
> + tail_skb = skb;
> + }
> +
> + return head_skb;
> +}
> +
> +/* Note that dequeue_skb can possibly return a SKB list (via skb->next).
> + * A requeued skb (via q->gso_skb) can also be a SKB list.
> + */
> static inline struct sk_buff *dequeue_skb(struct Qdisc *q)
> {
> struct sk_buff *skb = q->gso_skb;
> @@ -70,10 +106,17 @@ static inline struct sk_buff *dequeue_skb(struct Qdisc *q)
> } else
> skb = NULL;
> } else {
> - if (!(q->flags & TCQ_F_ONETXQUEUE) || !netif_xmit_frozen_or_stopped(txq)) {
> + if (!(q->flags & TCQ_F_ONETXQUEUE) ||
> + !netif_xmit_frozen_or_stopped(txq)) {
> + int bytelimit = qdisc_avail_bulklimit(txq);
> +
> skb = q->dequeue(q);
> - if (skb)
> + if (skb) {
> + bytelimit -= skb->len;
> skb = validate_xmit_skb(skb, qdisc_dev(q));
> + }
> + if (skb && qdisc_may_bulk(q))
> + skb = try_bulk_dequeue_skb(q, skb, bytelimit);
> }
> }
Okay...
In my opinion, you guys missed some opportunities :
1) Ideally the algo should try to call validate_xmit_skb() outside of
the locks (qdisc lock or device lock), because eventually doing checksum
computation or full segmentation should allow other cpus doing enqueues.
2) TSO support. Have you ideas how to perform this ?
^ permalink raw reply
* Re: [PATCH] netfilter: fix nf_conn_nat->masq_index visibility
From: Pablo Neira Ayuso @ 2014-09-30 11:47 UTC (permalink / raw)
To: Arnd Bergmann
Cc: netdev, Arturo Borrero Gonzalez, davem, linux-kernel,
linux-arm-kernel, netfilter-devel, coreteam, Patrick McHardy,
Jozsef Kadlecsik
In-Reply-To: <2670668.vq09CjLWH3@wuerfel>
On Tue, Sep 30, 2014 at 01:27:50PM +0200, Arnd Bergmann wrote:
> A recent change introduced the NF_NAT_MASQUERADE_IPV4/6 symbols and now
> builds the masquerading code based on this symbol rather than the
> IP_NF_TARGET_MASQUERADE symbol, however the nf_nat.h header file
> still uses the old symbol, which leads to a build error when that
> is disabled:
>
> nf_nat_masquerade_ipv4.c: In function 'nf_nat_masquerade_ipv4':
> nf_nat_masquerade_ipv4.c:59:5: error: 'struct nf_conn_nat' has no member named 'masq_index'
>
> This changes the header file to do the respective change, and move
> to using the IS_ENABLED() macro in the process, to improve readability
> of the check
David pulled a patch to resolve this yesterday:
http://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/commit/?id=67981fefb20e717cea55b42f9081a833fa46b3be
Thanks anyway.
^ permalink raw reply
* RE: [PATCH net-next] r8169:add support for RTL8168EP
From: Hau @ 2014-09-30 11:37 UTC (permalink / raw)
To: Francois Romieu
Cc: netdev@vger.kernel.org, nic_swsd, linux-kernel@vger.kernel.org
In-Reply-To: <20140927203921.GB28553@electric-eye.fr.zoreil.com>
I will spilt this patch and submit again.
Thanks,
Hau
-----Original Message-----
From: Francois Romieu [mailto:romieu@fr.zoreil.com]
Sent: Sunday, September 28, 2014 4:39 AM
To: Hau
Cc: netdev@vger.kernel.org; nic_swsd; linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next] r8169:add support for RTL8168EP
Chun-Hao Lin <hau@realtek.com> :
> RTL8168EP is Realtek PCIe Gigabit Ethernet controller.
> It is a successor chip of RTL8168DP.
>
> This patch add support for this chip.
It does more than that.
Did Hayes review it ?
> Signed-off-by: Chun-Hao Lin <hau@realtek.com>
> ---
> drivers/net/ethernet/realtek/r8169.c | 611
> +++++++++++++++++++++++++++++------
> 1 file changed, 514 insertions(+), 97 deletions(-)
>
> diff --git a/drivers/net/ethernet/realtek/r8169.c
> b/drivers/net/ethernet/realtek/r8169.c
> index 1d81238..0ead9a7 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -155,6 +155,9 @@ enum mac_version {
> RTL_GIGA_MAC_VER_46,
> RTL_GIGA_MAC_VER_47,
> RTL_GIGA_MAC_VER_48,
> + RTL_GIGA_MAC_VER_49,
> + RTL_GIGA_MAC_VER_50,
> + RTL_GIGA_MAC_VER_51,
> RTL_GIGA_MAC_NONE = 0xff,
> };
>
> @@ -302,6 +305,15 @@ static const struct {
> [RTL_GIGA_MAC_VER_48] =
> _R("RTL8107e", RTL_TD_1, FIRMWARE_8107E_2,
> JUMBO_1K, false),
> + [RTL_GIGA_MAC_VER_49] =
> + _R("RTL8168ep/8111ep", RTL_TD_1, NULL,
> + JUMBO_9K, false),
> + [RTL_GIGA_MAC_VER_50] =
> + _R("RTL8168ep/8111ep", RTL_TD_1, NULL,
> + JUMBO_9K, false),
> + [RTL_GIGA_MAC_VER_51] =
> + _R("RTL8168ep/8111ep", RTL_TD_1, NULL,
> + JUMBO_9K, false),
> };
> #undef _R
>
> @@ -400,6 +412,10 @@ enum rtl_registers {
> FuncEvent = 0xf0,
> FuncEventMask = 0xf4,
> FuncPresetState = 0xf8,
> + IBCR0 = 0xf8,
> + IBCR2 = 0xf9,
> + IBIMR0 = 0xfa,
> + IBISR0 = 0xfb,
> FuncForceEvent = 0xfc,
> };
>
> @@ -467,6 +483,7 @@ enum rtl8168_registers {
> #define ERIAR_EXGMAC (0x00 << ERIAR_TYPE_SHIFT)
> #define ERIAR_MSIX (0x01 << ERIAR_TYPE_SHIFT)
> #define ERIAR_ASF (0x02 << ERIAR_TYPE_SHIFT)
> +#define ERIAR_OOB (0x02 << ERIAR_TYPE_SHIFT)
> #define ERIAR_MASK_SHIFT 12
> #define ERIAR_MASK_0001 (0x1 << ERIAR_MASK_SHIFT)
> #define ERIAR_MASK_0011 (0x3 << ERIAR_MASK_SHIFT)
> @@ -935,93 +952,10 @@ static const struct rtl_cond name = { \
> \
> static bool name ## _check(struct rtl8169_private *tp)
>
> -DECLARE_RTL_COND(rtl_ocpar_cond)
> -{
> - void __iomem *ioaddr = tp->mmio_addr;
> -
> - return RTL_R32(OCPAR) & OCPAR_FLAG;
> -}
Feel free to move this function around but please do it in a separate patch. You are adding extra noise and thus making this stuff harder to review than it should be.
> -
> -static u32 ocp_read(struct rtl8169_private *tp, u8 mask, u16 reg) -{
> - void __iomem *ioaddr = tp->mmio_addr;
> -
> - RTL_W32(OCPAR, ((u32)mask & 0x0f) << 12 | (reg & 0x0fff));
> -
> - return rtl_udelay_loop_wait_high(tp, &rtl_ocpar_cond, 100, 20) ?
> - RTL_R32(OCPDR) : ~0;
> -}
(this one is modified)
> -
> -static void ocp_write(struct rtl8169_private *tp, u8 mask, u16 reg,
> u32 data) -{
> - void __iomem *ioaddr = tp->mmio_addr;
> -
> - RTL_W32(OCPDR, data);
> - RTL_W32(OCPAR, OCPAR_FLAG | ((u32)mask & 0x0f) << 12 | (reg & 0x0fff));
> -
> - rtl_udelay_loop_wait_low(tp, &rtl_ocpar_cond, 100, 20);
> -}
(this one is modified)
> -
> -DECLARE_RTL_COND(rtl_eriar_cond)
> -{
> - void __iomem *ioaddr = tp->mmio_addr;
> -
> - return RTL_R32(ERIAR) & ERIAR_FLAG;
> -}
Feel free to move this function around but please do it in a separate patch.
> -
> -static void rtl8168_oob_notify(struct rtl8169_private *tp, u8 cmd) -{
> - void __iomem *ioaddr = tp->mmio_addr;
> -
> - RTL_W8(ERIDR, cmd);
> - RTL_W32(ERIAR, 0x800010e8);
> - msleep(2);
> -
> - if (!rtl_udelay_loop_wait_low(tp, &rtl_eriar_cond, 100, 5))
> - return;
> -
> - ocp_write(tp, 0x1, 0x30, 0x00000001);
> -}
This one is modified but it is also modified for the existing 8168DP.
Mantra: please do it in a separate patch.
> -
> #define OOB_CMD_RESET 0x00
> #define OOB_CMD_DRIVER_START 0x05
> #define OOB_CMD_DRIVER_STOP 0x06
>
> -static u16 rtl8168_get_ocp_reg(struct rtl8169_private *tp) -{
> - return (tp->mac_version == RTL_GIGA_MAC_VER_31) ? 0xb8 : 0x10;
> -}
> -
> -DECLARE_RTL_COND(rtl_ocp_read_cond)
> -{
> - u16 reg;
> -
> - reg = rtl8168_get_ocp_reg(tp);
> -
> - return ocp_read(tp, 0x0f, reg) & 0x00000800;
> -}
(this one is simply moved around)
> -
> -static void rtl8168_driver_start(struct rtl8169_private *tp) -{
> - rtl8168_oob_notify(tp, OOB_CMD_DRIVER_START);
> -
> - rtl_msleep_loop_wait_high(tp, &rtl_ocp_read_cond, 10, 10);
> -}
(this one is modified)
> -
> -static void rtl8168_driver_stop(struct rtl8169_private *tp) -{
> - rtl8168_oob_notify(tp, OOB_CMD_DRIVER_STOP);
> -
> - rtl_msleep_loop_wait_low(tp, &rtl_ocp_read_cond, 10, 10);
> -}
(this one is modified)
> -
> -static int r8168dp_check_dash(struct rtl8169_private *tp) -{
> - u16 reg = rtl8168_get_ocp_reg(tp);
> -
> - return (ocp_read(tp, 0x0f, reg) & 0x00008000) ? 1 : 0;
> -}
(this one is modified)
[...]
> @@ -1329,6 +1277,45 @@ static void rtl_w1w0_eri(struct rtl8169_private *tp, int addr, u32 mask, u32 p,
> rtl_eri_write(tp, addr, mask, (val & ~m) | p, type); }
>
> +static u32 ocp_read(struct rtl8169_private *tp, u8 mask, u16 reg) {
> + void __iomem *ioaddr = tp->mmio_addr;
> +
> + if (tp->mac_version == RTL_GIGA_MAC_VER_49 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_50 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_51) {
> + return rtl_eri_read(tp, reg, ERIAR_OOB);
> + } else if (tp->mac_version == RTL_GIGA_MAC_VER_27 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_28 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_31) {
> + RTL_W32(OCPAR, ((u32)mask & 0x0f) << 12 | (reg & 0x0fff));
> + return rtl_udelay_loop_wait_high(tp, &rtl_ocpar_cond, 100, 20)
> + ? RTL_R32(OCPDR) : ~0;
Please:
- use a local "bool rc" variable for rtl_udelay_loop_wait_high status
so that you can line things correctly.
- use a switch case
- croak in the default case
> + }
> +
> + return ~0;
> +}
> +
> +static void ocp_write(struct rtl8169_private *tp, u8 mask, u16 reg,
> +u32 data) {
> + void __iomem *ioaddr = tp->mmio_addr;
> +
> + if (tp->mac_version == RTL_GIGA_MAC_VER_49 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_50 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_51) {
> + rtl_eri_write(tp, reg, ((u32)mask & 0x0f) << ERIAR_MASK_SHIFT,
> + data, ERIAR_OOB);
It should line up after the opening parenthesis in "rtl_eri_write("
> + } else if (tp->mac_version == RTL_GIGA_MAC_VER_27 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_28 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_31) {
> + RTL_W32(OCPDR, data);
> + RTL_W32(OCPAR, OCPAR_FLAG | ((u32)mask & 0x0f) << 12 |
> + (reg & 0x0fff));
It should line up after the opening parenthesis in "RTL_W32("
Please:
- use a switch case
- croak in the default case
You may state in a comment that this function is only designed for 8168DP and derivatives (whence the reduced switch case).
[...]
> @@ -1361,6 +1348,103 @@ static u8 rtl8168d_efuse_read(struct
> rtl8169_private *tp, int reg_addr)
[...]
> +static void rtl8168_driver_start(struct rtl8169_private *tp) {
> + if (tp->mac_version == RTL_GIGA_MAC_VER_49 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_50 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_51) {
> + u32 tmp;
> +
> + ocp_write(tp, 0x01, 0x180, OOB_CMD_DRIVER_START);
> + tmp = ocp_read(tp, 0x01, 0x30);
> + tmp |= 0x01;
> + ocp_write(tp, 0x01, 0x30, tmp);
> + rtl_msleep_loop_wait_high(tp, &rtl_ep_ocp_read_cond, 10, 10);
> + } else if (tp->mac_version == RTL_GIGA_MAC_VER_27 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_28 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_31) {
> + rtl8168_oob_notify(tp, OOB_CMD_DRIVER_START);
> + rtl_msleep_loop_wait_high(tp, &rtl_ocp_read_cond, 10, 10);
switch case + croak on default please.
[snip]
> @@ -3724,6 +3819,143 @@ static void rtl8168h_2_hw_phy_config(struct rtl8169_private *tp)
> rtl_writephy(tp, 0x1f, 0x0000);
> }
>
> +static void rtl8168ep_1_hw_phy_config(struct rtl8169_private *tp) {
> + rtl_apply_firmware(tp);
?
You did not specify any firmware information in rtl_chip_infos.
It some firmware is supposed to help, please be sure to add the relevant #define FIRMWARE_8xyz as well.
[...]
> +static void rtl8168ep_2_hw_phy_config(struct rtl8169_private *tp) {
> + rtl_apply_firmware(tp);
See above.
[...]
> @@ -4265,7 +4511,7 @@ static void r810x_pll_power_up(struct rtl8169_private *tp)
> break;
> case RTL_GIGA_MAC_VER_47:
> case RTL_GIGA_MAC_VER_48:
> - RTL_W8(PMCH, RTL_R8(PMCH) | 0xC0);
> + RTL_W8(PMCH, RTL_R8(PMCH) | 0xc0);
> break;
It's welcome but it should be done in a different patch.
[...]
> @@ -4339,8 +4585,11 @@ static void r8168_pll_power_down(struct
> rtl8169_private *tp)
>
> if ((tp->mac_version == RTL_GIGA_MAC_VER_27 ||
> tp->mac_version == RTL_GIGA_MAC_VER_28 ||
> - tp->mac_version == RTL_GIGA_MAC_VER_31) &&
> - r8168dp_check_dash(tp)) {
> + tp->mac_version == RTL_GIGA_MAC_VER_31 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_49 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_50 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_51) &&
> + r8168_check_dash(tp)) {
> return;
Unrelated behavior change for RTL_GIGA_MAC_VER_2[78]. Different patch.
> }
>
> @@ -4369,12 +4618,16 @@ static void r8168_pll_power_down(struct rtl8169_private *tp)
> case RTL_GIGA_MAC_VER_33:
> case RTL_GIGA_MAC_VER_45:
> case RTL_GIGA_MAC_VER_46:
> + case RTL_GIGA_MAC_VER_50:
> + case RTL_GIGA_MAC_VER_51:
> RTL_W8(PMCH, RTL_R8(PMCH) & ~0x80);
> break;
> case RTL_GIGA_MAC_VER_40:
> case RTL_GIGA_MAC_VER_41:
> + case RTL_GIGA_MAC_VER_49:
> rtl_w1w0_eri(tp, 0x1a8, ERIAR_MASK_1111, 0x00000000,
> 0xfc000000, ERIAR_EXGMAC);
> + RTL_W8(PMCH, RTL_R8(PMCH) & ~0x80);
Unrelated behavior change for RTL_GIGA_MAC_VER_4[01] (8168g, huh ?).
-> Different patch.
[...]
> @@ -4395,10 +4648,14 @@ static void r8168_pll_power_up(struct rtl8169_private *tp)
> break;
> case RTL_GIGA_MAC_VER_45:
> case RTL_GIGA_MAC_VER_46:
> - RTL_W8(PMCH, RTL_R8(PMCH) | 0xC0);
> + case RTL_GIGA_MAC_VER_50:
> + case RTL_GIGA_MAC_VER_51:
> + RTL_W8(PMCH, RTL_R8(PMCH) | 0xc0);
See above.
> break;
> case RTL_GIGA_MAC_VER_40:
> case RTL_GIGA_MAC_VER_41:
> + case RTL_GIGA_MAC_VER_49:
> + RTL_W8(PMCH, RTL_R8(PMCH) | 0xc0);
> rtl_w1w0_eri(tp, 0x1a8, ERIAR_MASK_1111, 0xfc000000,
> 0x00000000, ERIAR_EXGMAC);
See above.
[...]
> @@ -7366,9 +7766,13 @@ static void rtl_remove_one(struct pci_dev *pdev)
> struct net_device *dev = pci_get_drvdata(pdev);
> struct rtl8169_private *tp = netdev_priv(dev);
>
> - if (tp->mac_version == RTL_GIGA_MAC_VER_27 ||
> - tp->mac_version == RTL_GIGA_MAC_VER_28 ||
> - tp->mac_version == RTL_GIGA_MAC_VER_31) {
> + if ((tp->mac_version == RTL_GIGA_MAC_VER_27 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_28 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_31 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_49 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_50 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_51) &&
> + r8168_check_dash(tp)) {
It does not line up correctly (one space excess).
Unrelated behavior change for RTL_GIGA_MAC_VER_{27, 28, 31}. Different patch.
> rtl8168_driver_stop(tp);
> }
>
[...]
> @@ -7703,11 +8113,14 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> if (tp->mac_version == RTL_GIGA_MAC_VER_45 ||
> tp->mac_version == RTL_GIGA_MAC_VER_46 ||
> tp->mac_version == RTL_GIGA_MAC_VER_47 ||
> - tp->mac_version == RTL_GIGA_MAC_VER_48) {
> + tp->mac_version == RTL_GIGA_MAC_VER_48 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_49 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_50 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_51) {
> u16 mac_addr[3];
>
> - *(u32 *)&mac_addr[0] = rtl_eri_read(tp, 0xE0, ERIAR_EXGMAC);
> - *(u16 *)&mac_addr[2] = rtl_eri_read(tp, 0xE4, ERIAR_EXGMAC);
> + *(u32 *)&mac_addr[0] = rtl_eri_read(tp, 0xe0, ERIAR_EXGMAC);
> + *(u16 *)&mac_addr[2] = rtl_eri_read(tp, 0xe4, ERIAR_EXGMAC);
It's welcome but it brings noise in the current patch. Different patch.
>
> if (is_valid_ether_addr((u8 *)mac_addr))
> rtl_rar_set(tp, (u8 *)mac_addr);
> @@ -7780,9 +8193,13 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> rtl_chip_infos[chipset].jumbo_tx_csum ? "ok" : "ko");
> }
>
> - if (tp->mac_version == RTL_GIGA_MAC_VER_27 ||
> - tp->mac_version == RTL_GIGA_MAC_VER_28 ||
> - tp->mac_version == RTL_GIGA_MAC_VER_31) {
> + if ((tp->mac_version == RTL_GIGA_MAC_VER_27 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_28 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_31 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_49 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_50 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_51) &&
> + r8168_check_dash(tp)) {
See above. Unrelated change, etc.
> rtl8168_driver_start(tp);
> }
--
Ueimor
------Please consider the environment before printing this e-mail.
^ permalink raw reply
* Re: [PATCH] tun: make sure interface usage can not overflow
From: Michael S. Tsirkin @ 2014-09-30 11:29 UTC (permalink / raw)
To: Kees Cook
Cc: LKML, David S. Miller, Jason Wang, Zhi Yong Wu, Tom Herbert,
Masatake YAMATO, Xi Wang, stephen hemminger, Network Development
In-Reply-To: <CAGXu5j+x5pg6-9y72o=mE9TiNEyFqTTE=o8ETh1Ao8R20KNRwA@mail.gmail.com>
On Mon, Sep 29, 2014 at 12:48:47PM -0700, Kees Cook wrote:
> On Mon, Sep 29, 2014 at 4:48 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Sun, Sep 28, 2014 at 04:27:53PM -0700, Kees Cook wrote:
> >> This makes the size argument a const, since it is always populated by
> >> the caller. Additionally double-checks to make sure the copy_from_user
> >> can never overflow, keeping CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy:
> >>
> >> In function 'copy_from_user',
> >> inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1871:7:
> >> ... copy_from_user() buffer size is not provably correct
> >>
> >> Signed-off-by: Kees Cook <keescook@chromium.org>
> >
> > What exactly is the issue here?
> > __tun_chr_ioctl is called with sizeof(struct compat_ifreq)
> > or sizeof (struct ifreq) as the last argument.
>
> Correct. There is no vulnerability here; I am attempting to both make
> the code more defensive to future changes, and to keep
> CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy.
>
> > So this looks like a false positive, but
> > CONFIG_DEBUG_STRICT_USER_COPY_CHECKS machinery is supposed
> > to avoid false positives.
>
> The support in GCC is currently a bit faulty, and it seems that it
> didn't notice the two callers were static values, so instead, adding
> an explicit test keeps it happy.
>
> > On which architecture is this?
>
> This is on x86, but with CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
> correctly enabled (gcc after 4.6 broke its ability to correctly
> optimize), which I've been playing with trying to get gcc working
> again. I sent the patch because it seems like it's a reasonable
> defensive change to make.
For the BUG_ON - I guess it's reasonable but we'll just break it again
by random code changes in the future. And when we do, I don't cherish
the thought of using trial and error to find a way to shut up the
warning again.
If gcc can't be fixed, something explicit like the uninitialized_var
is needed.
For const as an argument - is it also about gcc bugs?
We don't usually do this in kernel so if it's useful,
it has to be documented.
Finally, why are you switching the type from int to size_t?
Pls document the reason in the commit log.
> If you want to look more deeply, there's some background here:
> https://code.google.com/p/chromium/issues/detail?id=371036
>
> https://git.kernel.org/cgit/linux/kernel/git/kees/linux.git/commit/?h=gcc-bug&id=92dd7154932d8775a05dfd3de5564124c05a4150
>
> Thanks,
>
> -Kees
I think what you are trying to do is really useful, but
I doubt making random code changes that happen to make a
specific gcc version happy is maintainable.
HTH
Thanks!
> --
> Kees Cook
> Chrome OS Security
^ permalink raw reply
* [PATCH] netfilter: fix nf_conn_nat->masq_index visibility
From: Arnd Bergmann @ 2014-09-30 11:27 UTC (permalink / raw)
To: netdev
Cc: Arturo Borrero Gonzalez, Pablo Neira Ayuso, davem, linux-kernel,
linux-arm-kernel, netfilter-devel, coreteam, Patrick McHardy,
Jozsef Kadlecsik
A recent change introduced the NF_NAT_MASQUERADE_IPV4/6 symbols and now
builds the masquerading code based on this symbol rather than the
IP_NF_TARGET_MASQUERADE symbol, however the nf_nat.h header file
still uses the old symbol, which leads to a build error when that
is disabled:
nf_nat_masquerade_ipv4.c: In function 'nf_nat_masquerade_ipv4':
nf_nat_masquerade_ipv4.c:59:5: error: 'struct nf_conn_nat' has no member named 'masq_index'
This changes the header file to do the respective change, and move
to using the IS_ENABLED() macro in the process, to improve readability
of the check
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: 8dd33cc93ec92 ("netfilter: nf_nat: generalize IPv4 masquerading support for nf_tables")
---
found while doing ARM randconfig tests
diff --git a/include/net/netfilter/nf_nat.h b/include/net/netfilter/nf_nat.h
index a71dd333ac68..68f0953c86c3 100644
--- a/include/net/netfilter/nf_nat.h
+++ b/include/net/netfilter/nf_nat.h
@@ -32,10 +32,8 @@ struct nf_conn_nat {
struct hlist_node bysource;
struct nf_conn *ct;
union nf_conntrack_nat_help help;
-#if defined(CONFIG_IP_NF_TARGET_MASQUERADE) || \
- defined(CONFIG_IP_NF_TARGET_MASQUERADE_MODULE) || \
- defined(CONFIG_IP6_NF_TARGET_MASQUERADE) || \
- defined(CONFIG_IP6_NF_TARGET_MASQUERADE_MODULE)
+#if IS_ENABLED(CONFIG_NF_NAT_MASQUERADE_IPV4) || \
+ IS_ENABLED(CONFIG_NF_NAT_MASQUERADE_IPV6)
int masq_index;
#endif
};
@@ -56,7 +54,7 @@ int nf_nat_used_tuple(const struct nf_conntrack_tuple *tuple,
static inline struct nf_conn_nat *nfct_nat(const struct nf_conn *ct)
{
-#if defined(CONFIG_NF_NAT) || defined(CONFIG_NF_NAT_MODULE)
+#if IS_ENABLED(CONFIG_NF_NAT)
return nf_ct_ext_find(ct, NF_CT_EXT_NAT);
#else
return NULL;
@@ -68,8 +66,8 @@ static inline bool nf_nat_oif_changed(unsigned int hooknum,
struct nf_conn_nat *nat,
const struct net_device *out)
{
-#if IS_ENABLED(CONFIG_IP_NF_TARGET_MASQUERADE) || \
- IS_ENABLED(CONFIG_IP6_NF_TARGET_MASQUERADE)
+#if IS_ENABLED(CONFIG_NF_NAT_MASQUERADE_IPV4) || \
+ IS_ENABLED(CONFIG_NF_NAT_MASQUERADE_IPV6)
return nat->masq_index && hooknum == NF_INET_POST_ROUTING &&
CTINFO2DIR(ctinfo) == IP_CT_DIR_ORIGINAL &&
nat->masq_index != out->ifindex;
^ permalink raw reply related
* RE: [PATCH] tun: make sure interface usage can not overflow
From: David Laight @ 2014-09-30 11:18 UTC (permalink / raw)
To: 'Hannes Frederic Sowa'
Cc: Kees Cook, linux-kernel@vger.kernel.org, David S. Miller,
Jason Wang, Zhi Yong Wu, Michael S. Tsirkin, Tom Herbert,
Masatake YAMATO, Xi Wang, stephen hemminger,
netdev@vger.kernel.org
In-Reply-To: <1412075023.7155.21.camel@localhost>
From: Hannes Frederic
> On Di, 2014-09-30 at 08:20 +0000, David Laight wrote:
> > From: Hannes Frederic
> > > On Mo, 2014-09-29 at 12:41 -0700, Kees Cook wrote:
> > > > On Mon, Sep 29, 2014 at 4:04 AM, David Laight <David.Laight@aculab.com> wrote:
> > > > > From: Kees Cook
> > > > >> This makes the size argument a const, since it is always populated by
> > > > >> the caller.
> > > > >
> > > > > There is almost no point making parameters 'const.
> > > > > ('const foo *' makes sense).
> > > > >
> > > > >> Additionally double-checks to make sure the copy_from_user
> > > > >> can never overflow, keeping CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy:
> > > > >>
> > > > >> In function 'copy_from_user',
> > > > >> inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1871:7:
> > > > >> ... copy_from_user() buffer size is not provably correct
> > > > >
> > > > > If 'ifreq_len' could be too big then you want to error the ioctl, not panic.
> > > > > If it can't be too big you don't need the check.
> > > >
> > > > The ifreq_len comes from the callers, and is the output of "sizeof"
> > > > which is const. Changing the function parameter to "const" means any
> > > > changes made in the future where the incoming value isn't const, the
> > > > compiler will throw a warning.
> > >
> > > Hmmm, I think you want something like BUILD_BUG_ON(!
> > > __builtin_constant_p(var)). const in function argument only ensures that
> > > the value cannot be modified in the function.
> >
> > You'd have to do something in the header file - nothing in the function
> > body can do that check.
>
> Sure, it should work. You only need to make sure that gcc inlines the
> function, so the value is constant (it is not enough that gcc knows the
> value range, one specific constant is needed). So the simplest fix for
> this is to specify __tun_chr_ioctl as __always_inline. ;)
You are joking aren't you???
Look at the code.
I'd suggest fixing whatever implements CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
to be less picky.
David
^ permalink raw reply
* [PATCH iproute2] iplink: do not require assigning negative ifindex at link creation
From: Atzm Watanabe @ 2014-09-30 11:10 UTC (permalink / raw)
To: netdev; +Cc: Cong Wang, Stephen Hemminger
Since commit 3c682146aeff, iplink requires assigning negative
ifindex (-1) to the kernel when creating interface without
specifying index.
Cc: Cong Wang <xiyou.wangcong@gmail.com>
Signed-off-by: Atzm Watanabe <atzm@stratosphere.co.jp>
---
ip/iplink.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/ip/iplink.c b/ip/iplink.c
index cb9c870..de6b2a9 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -689,7 +689,10 @@ static int iplink_modify(int cmd, unsigned int flags, int argc, char **argv)
addattr_l(&req.n, sizeof(req), IFLA_LINK, &ifindex, 4);
}
- req.i.ifi_index = index;
+ if (index <= 0)
+ req.i.ifi_index = 0;
+ else
+ req.i.ifi_index = index;
}
if (name) {
--
1.8.1.5
^ permalink raw reply related
* Re: [net-next PATCH V5] qdisc: bulk dequeue support for qdiscs with TCQ_F_ONETXQUEUE
From: Jamal Hadi Salim @ 2014-09-30 11:07 UTC (permalink / raw)
To: Jesper Dangaard Brouer, netdev, David S. Miller, Tom Herbert,
Eric Dumazet, Hannes Frederic Sowa, Florian Westphal,
Daniel Borkmann
Cc: Alexander Duyck, John Fastabend, Dave Taht, toke
In-Reply-To: <20140930085114.24043.81310.stgit@dragon>
On 09/30/14 04:53, Jesper Dangaard Brouer wrote:
[..]
> To avoid overshooting the HW limits, which results in requeuing, the
> patch limits the amount of bytes dequeued, based on the drivers BQL
> limits. In-effect bulking will only happen for BQL enabled drivers.
> Besides the bytelimit from BQL, also limit bulking to maximum 7
> packets to avoid any issues with available descriptor in HW and
> any HoL issues measured at 100Mbit/s.
>
[..]
>
> The measured perf diff benefit (at 10Gbit/s) for TCP_STREAM were 4.66%
> less CPU used on calls to _raw_spin_lock() (mostly from sch_direct_xmit).
>
> Tool mpstat, while stressing the system with netperf 24x TCP_STREAM, shows:
> * Disabled bulking: 8.30% soft 88.75% idle
> * Enabled bulking: 7.80% soft 89.36% idle
>
I know you have put a lot of hard work here and i hate to do
this to you, but:
The base test case is to surely *not to allow* the bulk code to be
executed at all. i.e when you say "disabled bulking" it
should mean not calling qdisc_may_bulk or qdisc_avail_bulklimit()
because that code was not there initially. My gut feeling is you
will find that your numbers for "Disabled bulking" to be a lot lower
than you show.
This is because no congestion likely happens at 24x TCP_STREAM
running at 10G with a modern day cpu and therefore you will end up
consuming more cpu.
Note, there are benefits as you have shown - but i would not
consider those to be standard use cases (actully one which would
have shown clear win is the VM thing Rusty was after). For this reason
my view is that i should be able to disable via ifdef bulking (yes, I
know DaveM hates ifdefs ;->).
cheers,
jamal
^ permalink raw reply
* Re: [PATCH] tun: make sure interface usage can not overflow
From: Hannes Frederic Sowa @ 2014-09-30 11:03 UTC (permalink / raw)
To: David Laight
Cc: Kees Cook, linux-kernel@vger.kernel.org, David S. Miller,
Jason Wang, Zhi Yong Wu, Michael S. Tsirkin, Tom Herbert,
Masatake YAMATO, Xi Wang, stephen hemminger,
netdev@vger.kernel.org
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6D174A0C5C@AcuExch.aculab.com>
On Di, 2014-09-30 at 08:20 +0000, David Laight wrote:
> From: Hannes Frederic
> > On Mo, 2014-09-29 at 12:41 -0700, Kees Cook wrote:
> > > On Mon, Sep 29, 2014 at 4:04 AM, David Laight <David.Laight@aculab.com> wrote:
> > > > From: Kees Cook
> > > >> This makes the size argument a const, since it is always populated by
> > > >> the caller.
> > > >
> > > > There is almost no point making parameters 'const.
> > > > ('const foo *' makes sense).
> > > >
> > > >> Additionally double-checks to make sure the copy_from_user
> > > >> can never overflow, keeping CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy:
> > > >>
> > > >> In function 'copy_from_user',
> > > >> inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1871:7:
> > > >> ... copy_from_user() buffer size is not provably correct
> > > >
> > > > If 'ifreq_len' could be too big then you want to error the ioctl, not panic.
> > > > If it can't be too big you don't need the check.
> > >
> > > The ifreq_len comes from the callers, and is the output of "sizeof"
> > > which is const. Changing the function parameter to "const" means any
> > > changes made in the future where the incoming value isn't const, the
> > > compiler will throw a warning.
> >
> > Hmmm, I think you want something like BUILD_BUG_ON(!
> > __builtin_constant_p(var)). const in function argument only ensures that
> > the value cannot be modified in the function.
>
> You'd have to do something in the header file - nothing in the function
> body can do that check.
Sure, it should work. You only need to make sure that gcc inlines the
function, so the value is constant (it is not enough that gcc knows the
value range, one specific constant is needed). So the simplest fix for
this is to specify __tun_chr_ioctl as __always_inline. ;)
Bye,
Hannes
^ permalink raw reply
* Re: [PATCH 2/7] netfilter: Convert print_tuple functions to return void
From: Petr Mladek @ 2014-09-30 10:22 UTC (permalink / raw)
To: Joe Perches
Cc: Steven Rostedt, Al Viro, Andrew Morton, Linus Torvalds,
Pablo Neira Ayuso, Patrick McHardy, Jozsef Kadlecsik,
David S. Miller, netfilter-devel, coreteam, netdev, linux-kernel
In-Reply-To: <f2e8cf8df433a197daa62cbaf124c900c708edc7.1412031505.git.joe@perches.com>
On Mon 29-09-14 16:08:22, Joe Perches wrote:
> Since adding a new function to seq_file (seq_is_full)
> there isn't any value for functions called from seq_show to
> return anything. Remove the int returns of the various
> print_tuple/<foo>_print_tuple functions.
>
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
> include/net/netfilter/nf_conntrack_core.h | 2 +-
> include/net/netfilter/nf_conntrack_l3proto.h | 4 ++--
> include/net/netfilter/nf_conntrack_l4proto.h | 4 ++--
> net/netfilter/nf_conntrack_l3proto_generic.c | 5 ++---
> net/netfilter/nf_conntrack_proto_dccp.c | 10 +++++-----
> net/netfilter/nf_conntrack_proto_generic.c | 5 ++---
> net/netfilter/nf_conntrack_proto_gre.c | 10 +++++-----
> net/netfilter/nf_conntrack_proto_sctp.c | 10 +++++-----
> net/netfilter/nf_conntrack_proto_tcp.c | 10 +++++-----
> net/netfilter/nf_conntrack_proto_udp.c | 10 +++++-----
> net/netfilter/nf_conntrack_proto_udplite.c | 10 +++++-----
> net/netfilter/nf_conntrack_standalone.c | 15 +++++++--------
> 12 files changed, 46 insertions(+), 49 deletions(-)
>
> diff --git a/include/net/netfilter/nf_conntrack_core.h b/include/net/netfilter/nf_conntrack_core.h
> index cc0c188..f2f0fa3 100644
> --- a/include/net/netfilter/nf_conntrack_core.h
> +++ b/include/net/netfilter/nf_conntrack_core.h
> @@ -72,7 +72,7 @@ static inline int nf_conntrack_confirm(struct sk_buff *skb)
> return ret;
> }
>
> -int
> +void
> print_tuple(struct seq_file *s, const struct nf_conntrack_tuple *tuple,
> const struct nf_conntrack_l3proto *l3proto,
> const struct nf_conntrack_l4proto *proto);
> diff --git a/include/net/netfilter/nf_conntrack_l3proto.h b/include/net/netfilter/nf_conntrack_l3proto.h
> index adc1fa3..cdc920b 100644
> --- a/include/net/netfilter/nf_conntrack_l3proto.h
> +++ b/include/net/netfilter/nf_conntrack_l3proto.h
> @@ -38,8 +38,8 @@ struct nf_conntrack_l3proto {
> const struct nf_conntrack_tuple *orig);
>
> /* Print out the per-protocol part of the tuple. */
> - int (*print_tuple)(struct seq_file *s,
> - const struct nf_conntrack_tuple *);
> + void (*print_tuple)(struct seq_file *s,
> + const struct nf_conntrack_tuple *);
>
> /*
> * Called before tracking.
> diff --git a/include/net/netfilter/nf_conntrack_l4proto.h b/include/net/netfilter/nf_conntrack_l4proto.h
> index 4c8d573..fead8ee 100644
> --- a/include/net/netfilter/nf_conntrack_l4proto.h
> +++ b/include/net/netfilter/nf_conntrack_l4proto.h
> @@ -56,8 +56,8 @@ struct nf_conntrack_l4proto {
> u_int8_t pf, unsigned int hooknum);
>
> /* Print out the per-protocol part of the tuple. Return like seq_* */
> - int (*print_tuple)(struct seq_file *s,
> - const struct nf_conntrack_tuple *);
> + void (*print_tuple)(struct seq_file *s,
> + const struct nf_conntrack_tuple *);
>
> /* Print out the private part of the conntrack. */
> int (*print_conntrack)(struct seq_file *s, struct nf_conn *);
> diff --git a/net/netfilter/nf_conntrack_l3proto_generic.c b/net/netfilter/nf_conntrack_l3proto_generic.c
> index e7eb807..cf9ace7 100644
> --- a/net/netfilter/nf_conntrack_l3proto_generic.c
> +++ b/net/netfilter/nf_conntrack_l3proto_generic.c
> @@ -49,10 +49,9 @@ static bool generic_invert_tuple(struct nf_conntrack_tuple *tuple,
> return true;
> }
>
> -static int generic_print_tuple(struct seq_file *s,
> - const struct nf_conntrack_tuple *tuple)
> +static void generic_print_tuple(struct seq_file *s,
> + const struct nf_conntrack_tuple *tuple)
> {
> - return 0;
> }
>
> static int generic_get_l4proto(const struct sk_buff *skb, unsigned int nhoff,
> diff --git a/net/netfilter/nf_conntrack_proto_dccp.c b/net/netfilter/nf_conntrack_proto_dccp.c
> index cb372f9..40d96f9 100644
> --- a/net/netfilter/nf_conntrack_proto_dccp.c
> +++ b/net/netfilter/nf_conntrack_proto_dccp.c
> @@ -618,12 +618,12 @@ out_invalid:
> return -NF_ACCEPT;
> }
>
> -static int dccp_print_tuple(struct seq_file *s,
> - const struct nf_conntrack_tuple *tuple)
> +static void dccp_print_tuple(struct seq_file *s,
> + const struct nf_conntrack_tuple *tuple)
> {
> - return seq_printf(s, "sport=%hu dport=%hu ",
> - ntohs(tuple->src.u.dccp.port),
> - ntohs(tuple->dst.u.dccp.port));
> + seq_printf(s, "sport=%hu dport=%hu ",
> + ntohs(tuple->src.u.dccp.port),
> + ntohs(tuple->dst.u.dccp.port));
> }
>
> static int dccp_print_conntrack(struct seq_file *s, struct nf_conn *ct)
> diff --git a/net/netfilter/nf_conntrack_proto_generic.c b/net/netfilter/nf_conntrack_proto_generic.c
> index d25f293..0a3ded1 100644
> --- a/net/netfilter/nf_conntrack_proto_generic.c
> +++ b/net/netfilter/nf_conntrack_proto_generic.c
> @@ -39,10 +39,9 @@ static bool generic_invert_tuple(struct nf_conntrack_tuple *tuple,
> }
>
> /* Print out the per-protocol part of the tuple. */
> -static int generic_print_tuple(struct seq_file *s,
> - const struct nf_conntrack_tuple *tuple)
> +static void generic_print_tuple(struct seq_file *s,
> + const struct nf_conntrack_tuple *tuple)
> {
> - return 0;
> }
>
> static unsigned int *generic_get_timeouts(struct net *net)
> diff --git a/net/netfilter/nf_conntrack_proto_gre.c b/net/netfilter/nf_conntrack_proto_gre.c
> index d566573..cd85336 100644
> --- a/net/netfilter/nf_conntrack_proto_gre.c
> +++ b/net/netfilter/nf_conntrack_proto_gre.c
> @@ -226,12 +226,12 @@ static bool gre_pkt_to_tuple(const struct sk_buff *skb, unsigned int dataoff,
> }
>
> /* print gre part of tuple */
> -static int gre_print_tuple(struct seq_file *s,
> - const struct nf_conntrack_tuple *tuple)
> +static void gre_print_tuple(struct seq_file *s,
> + const struct nf_conntrack_tuple *tuple)
> {
> - return seq_printf(s, "srckey=0x%x dstkey=0x%x ",
> - ntohs(tuple->src.u.gre.key),
> - ntohs(tuple->dst.u.gre.key));
> + seq_printf(s, "srckey=0x%x dstkey=0x%x ",
> + ntohs(tuple->src.u.gre.key),
> + ntohs(tuple->dst.u.gre.key));
> }
>
> /* print private data for conntrack */
> diff --git a/net/netfilter/nf_conntrack_proto_sctp.c b/net/netfilter/nf_conntrack_proto_sctp.c
> index 1314d33..1b1d0e9 100644
> --- a/net/netfilter/nf_conntrack_proto_sctp.c
> +++ b/net/netfilter/nf_conntrack_proto_sctp.c
> @@ -166,12 +166,12 @@ static bool sctp_invert_tuple(struct nf_conntrack_tuple *tuple,
> }
>
> /* Print out the per-protocol part of the tuple. */
> -static int sctp_print_tuple(struct seq_file *s,
> - const struct nf_conntrack_tuple *tuple)
> +static void sctp_print_tuple(struct seq_file *s,
> + const struct nf_conntrack_tuple *tuple)
> {
> - return seq_printf(s, "sport=%hu dport=%hu ",
> - ntohs(tuple->src.u.sctp.port),
> - ntohs(tuple->dst.u.sctp.port));
> + seq_printf(s, "sport=%hu dport=%hu ",
> + ntohs(tuple->src.u.sctp.port),
> + ntohs(tuple->dst.u.sctp.port));
> }
>
> /* Print out the private part of the conntrack. */
> diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
> index 44d1ea3..c2693e78 100644
> --- a/net/netfilter/nf_conntrack_proto_tcp.c
> +++ b/net/netfilter/nf_conntrack_proto_tcp.c
> @@ -302,12 +302,12 @@ static bool tcp_invert_tuple(struct nf_conntrack_tuple *tuple,
> }
>
> /* Print out the per-protocol part of the tuple. */
> -static int tcp_print_tuple(struct seq_file *s,
> - const struct nf_conntrack_tuple *tuple)
> +static void tcp_print_tuple(struct seq_file *s,
> + const struct nf_conntrack_tuple *tuple)
> {
> - return seq_printf(s, "sport=%hu dport=%hu ",
> - ntohs(tuple->src.u.tcp.port),
> - ntohs(tuple->dst.u.tcp.port));
> + seq_printf(s, "sport=%hu dport=%hu ",
> + ntohs(tuple->src.u.tcp.port),
> + ntohs(tuple->dst.u.tcp.port));
> }
>
> /* Print out the private part of the conntrack. */
> diff --git a/net/netfilter/nf_conntrack_proto_udp.c b/net/netfilter/nf_conntrack_proto_udp.c
> index 9d7721c..6957281 100644
> --- a/net/netfilter/nf_conntrack_proto_udp.c
> +++ b/net/netfilter/nf_conntrack_proto_udp.c
> @@ -63,12 +63,12 @@ static bool udp_invert_tuple(struct nf_conntrack_tuple *tuple,
> }
>
> /* Print out the per-protocol part of the tuple. */
> -static int udp_print_tuple(struct seq_file *s,
> - const struct nf_conntrack_tuple *tuple)
> +static void udp_print_tuple(struct seq_file *s,
> + const struct nf_conntrack_tuple *tuple)
> {
> - return seq_printf(s, "sport=%hu dport=%hu ",
> - ntohs(tuple->src.u.udp.port),
> - ntohs(tuple->dst.u.udp.port));
> + seq_printf(s, "sport=%hu dport=%hu ",
> + ntohs(tuple->src.u.udp.port),
> + ntohs(tuple->dst.u.udp.port));
> }
>
> static unsigned int *udp_get_timeouts(struct net *net)
> diff --git a/net/netfilter/nf_conntrack_proto_udplite.c b/net/netfilter/nf_conntrack_proto_udplite.c
> index 2750e6c..c5903d1 100644
> --- a/net/netfilter/nf_conntrack_proto_udplite.c
> +++ b/net/netfilter/nf_conntrack_proto_udplite.c
> @@ -71,12 +71,12 @@ static bool udplite_invert_tuple(struct nf_conntrack_tuple *tuple,
> }
>
> /* Print out the per-protocol part of the tuple. */
> -static int udplite_print_tuple(struct seq_file *s,
> - const struct nf_conntrack_tuple *tuple)
> +static void udplite_print_tuple(struct seq_file *s,
> + const struct nf_conntrack_tuple *tuple)
> {
> - return seq_printf(s, "sport=%hu dport=%hu ",
> - ntohs(tuple->src.u.udp.port),
> - ntohs(tuple->dst.u.udp.port));
> + seq_printf(s, "sport=%hu dport=%hu ",
> + ntohs(tuple->src.u.udp.port),
> + ntohs(tuple->dst.u.udp.port));
> }
>
> static unsigned int *udplite_get_timeouts(struct net *net)
> diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
> index cf65a1e..3c2ce5c 100644
> --- a/net/netfilter/nf_conntrack_standalone.c
> +++ b/net/netfilter/nf_conntrack_standalone.c
> @@ -36,12 +36,13 @@
> MODULE_LICENSE("GPL");
>
> #ifdef CONFIG_NF_CONNTRACK_PROCFS
> -int
> +void
> print_tuple(struct seq_file *s, const struct nf_conntrack_tuple *tuple,
> const struct nf_conntrack_l3proto *l3proto,
> const struct nf_conntrack_l4proto *l4proto)
> {
> - return l3proto->print_tuple(s, tuple) || l4proto->print_tuple(s, tuple);
> + l3proto->print_tuple(s, tuple);
> + l4proto->print_tuple(s, tuple);
> }
> EXPORT_SYMBOL_GPL(print_tuple);
>
> @@ -202,9 +203,8 @@ static int ct_seq_show(struct seq_file *s, void *v)
> if (l4proto->print_conntrack && l4proto->print_conntrack(s, ct))
> goto release;
>
> - if (print_tuple(s, &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple,
> - l3proto, l4proto))
> - goto release;
> + print_tuple(s, &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple,
> + l3proto, l4proto);
To be precise, we should add:
if (seq_overflow(s))
goto release;
> if (seq_print_acct(s, ct, IP_CT_DIR_ORIGINAL))
> goto release;
> @@ -213,9 +213,8 @@ static int ct_seq_show(struct seq_file *s, void *v)
> if (seq_printf(s, "[UNREPLIED] "))
> goto release;
>
> - if (print_tuple(s, &ct->tuplehash[IP_CT_DIR_REPLY].tuple,
> - l3proto, l4proto))
> - goto release;
> + print_tuple(s, &ct->tuplehash[IP_CT_DIR_REPLY].tuple,
> + l3proto, l4proto);
same here
> if (seq_print_acct(s, ct, IP_CT_DIR_REPLY))
> goto release;
> --
> 1.8.1.2.459.gbcd45b4.dirty
>
Best Regards,
Petr
^ permalink raw reply
* Re: [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()
From: Chen Gang @ 2014-09-30 10:10 UTC (permalink / raw)
To: David Vrabel, konrad.wilk, ian.campbell, wei.liu2,
boris.ostrovsky, bhelgaas, jgross, yongjun_wei, mukesh.rathor
Cc: xen-devel, netdev@vger.kernel.org, linux-pci, linux-scsi,
linux-kernel@vger.kernel.org
In-Reply-To: <542A7E0E.9050702@citrix.com>
On 9/30/14 17:55, David Vrabel wrote:
> On 26/09/14 17:36, Chen Gang wrote:
>> When xenbus_switch_state() fails, it will call xenbus_switch_fatal()
>> internally, so need not return any status value, then use 'void' instead
>> of 'int' for xenbus_switch_state() and __xenbus_switch_state().
>>
>> Also need be sure that all callers which check the return value must let
>> 'err' be 0.
>>
>> And also need change the related comments for xenbus_switch_state().
>
> Since this patch does not fix a bug and there is no unanimous agreement
> on the API change I'm not going to apply it (nor the previous version).
>
OK, at least for me, it is no problems.
But I still recommend to improve it in the future, it is not a good idea
to let all related things remain in current condition (for me, at lease
need some related code comments for it).
Thanks.
--
Chen Gang
Open, share, and attitude like air, water, and life which God blessed
^ permalink raw reply
* Re: BCM4313 & brcmsmac & 3.12: only semi-working?
From: Arend van Spriel @ 2014-09-30 10:06 UTC (permalink / raw)
To: Maximilian Engelhardt
Cc: Michael Tokarev, Seth Forshee, brcm80211-dev-list, linux-wireless,
netdev
In-Reply-To: <2326963.udog5tteBt@eisbaer>
On 09/29/14 21:40, Maximilian Engelhardt wrote:
> On Monday 29 September 2014 15:44:03 Arend van Spriel wrote:
>> On 09/26/14 17:20, Michael Tokarev wrote:
>>> I can send it your way, -- guess it will be quite a bit costly,
>>> but I don't have any use for it anyway (short of throwing it
>>> away), and since I already spent significantly more money due
>>> to all this (whole thinkpad plus ssds and several wifi adaptors),
>>> this additional cost is just a small noize. But since that's
>>> 2nd card in a row, maybe there's something else in there, the
>>> prob is not in the card?
>>
>> Could be. Maybe some BIOS issue. Can you make some hi-res pictures of
>> the card and email them to me? If it is identical to what I already have
>> over here there is not much sense in sending it.
>>
>> Regards,
>> Arend
>
> Hi Arend,
>
> I just saw this thread on linux-wireless and wanted to answer as it might be
> of interest to you.
>
> I also own a BCM4313 wireless network card. About a year ago I reported some
> problems with reception strength which were then fixed after some time,
> debugging and testing passed. At that time I also did some throughput testing,
> but only had a 802.11g access-point to test. The results were not ideal, but
> also not too bad. So at that time I thought the issues were all more or less
> fixed and mostly fine. I also don't use wireless very much, so as long as things
> do work somehow acceptable I probably don't notice any problems immediately.
> So it comes that only about some month ago I noticed that the throughput I
> measured with my 11g access-point (about half the rate than with an atheros
> card on the same ap) is about the same on a 11n access-point where it should
> be much faster. I didn't experience any stalls, but that may also be that I
> didn't use the card enough to really notice them.
>
> I always wanted to report a bug because of the low throughout, but never got
> to it because of lack of time. I didn't want to provide a report saying just
> it doesn't work or it's slow without any data about it and a description how
> to reproduce it, as I think without this information a bug report is mostly
> useless.
>
> I also had a look at the kernel changelog of the brcmsm driver and notices
> there was little to no activity lately. Because of this I also wasn't sure if
> there is still someone interesting in fixing bugs for this device.
>
> As I was annoyed by the bad support for this card I decided it would be more
> easy and much less time consuming to simply buy another card than trying to
> get this fixed. So I bought a BCM43228 card, because it also supports 5 GHz.
> Only after it arrived I noticed that it was only supported by the 3.17+ kernel
> (not so much of a problem) and that it only seems to work in 802.11g mode
> (only slow speeds and no 5 GHz). At least I could use it in full 11g speeds,
> so it was a improvement.
>
> So I still don't have a card that does simply work. As I hope the missing
> support for my BCM43228 will hopefully be added some time in the future it
> probably would still be worth fixing the BCM4313 card as other users will also
> benefit from it.
>
> A friend of mine also has the same laptop than me and the same (or at least
> very similar) wireless card. He has told me he has also problem with stalls
> like Michael reported (if I remember the history of the thread correctly).
>
> So I'm not really sure where I should go from here. I can try to provide some
> debugging information as time permits, but I don't know how much time I will
> have for this in the future. Of course ideally I want to use the BCM43228 card
> with full support, as it can work on 5 GHz.
>
> Currently the BCM43228 card is plugged into my laptop, but I want to avoid
> swapping the cards more that very few times because the antenna connectors are
> only designed for very few (un)plug cycles.
>
> If it's of any information, my card is labeled BCM-BCM94313HMGB on the
> sticker, the laptop where it was originally is a ThinkPad Edge E135.
Thanks for taking time to chime in. This chipset is a pain in the....
The label info does help. You have a 4313 with internal PA for which
support was added later. The card that Michael has seems to have an
external PA. The initial iPA support patch broke things for everyone
with external PA so it was reverted. In the second round it was better,
but it seems Michael still had issues. As he mentioned BT issues and his
card shares the external PA between WLAN and BT I believe that there is
a BT-coex issue.
What is causing your 4313 to seemingly do 11g rates is hard to tell
without any debug info. I have that card over here, but in my cabled
setup it is doing 72Mbps, ie. 11n rate. I can run a rate-vs-range test
to see if there is an issue.
Thanks again and
Regards,
Arend
^ 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