* Re: [PATCH] ipvs: Remove unused variable ret from sync_thread_master()
From: Oleg Nesterov @ 2013-11-12 16:21 UTC (permalink / raw)
To: Peter Zijlstra, Tejun Heo
Cc: David Laight, Geert Uytterhoeven, Ingo Molnar, netdev,
linux-kernel
In-Reply-To: <20131112145243.GU5056@laptop.programming.kicks-ass.net>
On 11/12, Peter Zijlstra wrote:
>
> On Tue, Nov 12, 2013 at 02:21:39PM -0000, David Laight wrote:
> > Shame there isn't a process flag to indicate that the process
> > will sleep uninterruptibly and that it doesn't matter.
> > So don't count to the load average and don't emit a warning
> > if it has been sleeping for a long time.
>
> A process flag wouldn't work, because the task could block waiting for
> actual work to complete in other sleeps.
>
> However, we could do something like the below; which would allow us
> writing things like:
>
> (void)___wait_event(*sk_sleep(sk),
> sock_writeable(sk) || kthread_should_stop(),
> TASK_UNINTERRUPTIBLE | TASK_IDLE, 0, 0,
> schedule());
>
> Marking the one wait-for-more-work as TASK_IDLE such that it doesn't
> contribute to the load avg.
Agreed, I thought about additional bit too.
> static const char * const task_state_array[] = {
> - "R (running)", /* 0 */
> - "S (sleeping)", /* 1 */
> - "D (disk sleep)", /* 2 */
> - "T (stopped)", /* 4 */
> - "t (tracing stop)", /* 8 */
> - "Z (zombie)", /* 16 */
> - "X (dead)", /* 32 */
> - "x (dead)", /* 64 */
> - "K (wakekill)", /* 128 */
> - "W (waking)", /* 256 */
> - "P (parked)", /* 512 */
> + "R (running)", /* 0 */
> + "S (sleeping)", /* 1 */
> + "D (disk sleep)", /* 2 */
> + "T (stopped)", /* 4 */
> + "t (tracing stop)", /* 8 */
> + "Z (zombie)", /* 16 */
> + "X (dead)", /* 32 */
> + "x (dead)", /* 64 */
> + "K (wakekill)", /* 128 */
> + "W (waking)", /* 256 */
> + "P (parked)", /* 512 */
> + "I (idle)", /* 1024 */
> };
but I am not sure about what /proc/ should report in this case...
> #define task_contributes_to_load(task) \
> ((task->state & TASK_UNINTERRUPTIBLE) != 0 && \
> - (task->flags & PF_FROZEN) == 0)
> + (task->flags & PF_FROZEN) == 0 && \
> + (task->state & TASK_IDLE) == 0)
perhaps
(task->state & (TASK_UNINTERRUPTIBLE | TASK_IDLE)) == TASK_UNINTERRUPTIBLE
can save an insn.
I am also wondering if it makes any sense to turn PF_FROZEN into
TASK_FROZEN, something like (incomplete, probably racy) patch below.
Note that it actually adds the new state, not the the qualifier.
Oleg.
--- x/include/linux/freezer.h
+++ x/include/linux/freezer.h
@@ -23,7 +23,7 @@ extern unsigned int freeze_timeout_msecs
*/
static inline bool frozen(struct task_struct *p)
{
- return p->flags & PF_FROZEN;
+ return p->state & TASK_FROZEN;
}
extern bool freezing_slow_path(struct task_struct *p);
--- x/kernel/freezer.c
+++ x/kernel/freezer.c
@@ -57,16 +57,13 @@ bool __refrigerator(bool check_kthr_stop
pr_debug("%s entered refrigerator\n", current->comm);
for (;;) {
- set_current_state(TASK_UNINTERRUPTIBLE);
-
spin_lock_irq(&freezer_lock);
- current->flags |= PF_FROZEN;
- if (!freezing(current) ||
- (check_kthr_stop && kthread_should_stop()))
- current->flags &= ~PF_FROZEN;
+ if (freezing(current) &&
+ !(check_kthr_stop && kthread_should_stop()))
+ set_current_state(TASK_FROZEN);
spin_unlock_irq(&freezer_lock);
- if (!(current->flags & PF_FROZEN))
+ if (!(current->state & TASK_FROZEN))
break;
was_frozen = true;
schedule();
@@ -148,8 +145,7 @@ void __thaw_task(struct task_struct *p)
* refrigerator.
*/
spin_lock_irqsave(&freezer_lock, flags);
- if (frozen(p))
- wake_up_process(p);
+ try_to_wake_up(p, TASK_FROZEN, 0);
spin_unlock_irqrestore(&freezer_lock, flags);
}
^ permalink raw reply
* Re: [RFC] Revert "sierra_net: keep status interrupt URB active"
From: Dan Williams @ 2013-11-12 16:25 UTC (permalink / raw)
To: Bjørn Mork
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, linux-usb-u79uwXL29TY76Z2rM5mHXA,
John Henderson
In-Reply-To: <1383946173.29096.12.camel-wKZy7rqYPVb5EHUCmHmTqw@public.gmane.org>
On Fri, 2013-11-08 at 15:29 -0600, Dan Williams wrote:
> On Fri, 2013-11-08 at 21:44 +0100, Bjørn Mork wrote:
> > Dan Williams <dcbw-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> writes:
> > > On Mon, 2013-11-04 at 14:27 -0600, Dan Williams wrote:
> > >> On Fri, 2013-11-01 at 13:53 +0100, Bjørn Mork wrote:
> > >> > This reverts commit 7b0c5f21f348a66de495868b8df0284e8dfd6bbf.
> > >> >
> > >> > It's not easy to create a driver for all the various firmware
> > >> > bugs out there.
> > >> >
> > >> > This change caused regressions for a number of devices, which
> > >> > started to fail link detection and therefore became completely
> > >> > non-functional. The exact reason is yet unknown, it looks like
> > >> > the affected firmwares might actually need all or some of the
> > >> > additional SYNC messages the patch got rid of.
> > >> >
> > >> > Reverting is not optimal, as it will re-introduce the original
> > >> > problem, but it is currently the only alternative known to fix
> > >> > this issue.
> > >>
> > >> Instead, how does the following patch work for you?
> > >
> > > Bjorn, did you have a chance to try this patch out on your devices?
> >
> > The only DirectIP device I have is the MC7710, which never had any of
> > the firmware issues you are trying to fix. I only tried to forward Johns
> > issue.
> >
> > When this patch worked for John, then I am pretty confident that you
> > have solved the problem here.
>
> Well, "solved", since I still have no idea why the original patch would
> cause the device behavior based on what I know and have read about the
> expected firmware/host handshaking sequence. But the patch there
> appears to fix the problem *and* not blindly send tons of SYNCs forever.
>
> So I'll go ahead and submit a proper version of it.
Actually, is "[PATCH] usbnet: fix status interrupt urb handling" the
real fix for this problem?
John, any chance you could revert my RFC patch and try Felix's patch in
that mail?
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] ipvs: Remove unused variable ret from sync_thread_master()
From: Oleg Nesterov @ 2013-11-12 16:26 UTC (permalink / raw)
To: Peter Zijlstra
Cc: David Laight, Geert Uytterhoeven, Ingo Molnar, netdev,
linux-kernel
In-Reply-To: <20131112143152.GS5056@laptop.programming.kicks-ass.net>
On 11/12, Peter Zijlstra wrote:
>
> On Tue, Nov 12, 2013 at 02:21:39PM -0000, David Laight wrote:
> >
> > /* Tell scheduler we are going to sleep... */
> > if (signal_pending(current))
> > /* We don't want waking immediately (again) */
> > sleep_state = TASK_UNINTERRUPTIBLE;
> > else
> > sleep_state = TASK_INTERRUPTIBLE;
> > set_current_state(sleep_state);
>
> If this is for kernel threads, I think you can wipe the pending state;
Yes, unless this kthread does allow_signal() signal_pending() can't be
true.
Oleg.
^ permalink raw reply
* Re: [RFC PATCHv2 3/4] of: provide a binding for fixed link PHYs
From: Mark Rutland @ 2013-11-12 16:29 UTC (permalink / raw)
To: Grant Likely, Florian Fainelli
Cc: Thomas Petazzoni, David S. Miller, netdev@vger.kernel.org,
devicetree@vger.kernel.org, Lior Amsalem, Sascha Hauer,
Christian Gmeiner, Ezequiel Garcia, Gregory Clement,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20131112123746.EAEF0C42283@trevor.secretlab.ca>
Hi Florian, Grant,
On Tue, Nov 12, 2013 at 12:37:46PM +0000, Grant Likely wrote:
> On Fri, 25 Oct 2013 05:40:57 +0100, Florian Fainelli <florian@openwrt.org> wrote:
> > Le mercredi 18 septembre 2013, 18:11:12 Thomas Petazzoni a écrit :
> > > Dear Grant Likely,
> > >
> > > On Tue, 17 Sep 2013 23:29:23 -0500, Grant Likely wrote:
> > > > I understand what you're trying to do here, but it causes a troublesome
> > > > leakage of implementation detail into the binding, making the whole
> > > > thing look very odd. This binding tries to make a fixed link look
> > > > exactly like a real PHY even to the point of including a phandle to the
> > > > phy. But having a phandle to a node which is *always* a direct child of
> > > > the MAC node is redundant and a rather looney. Yes, doing it that way
> > > > makes it easy for of_phy_find_device() to be transparent for fixed link,
> > > > but that should *not* drive bindings, especially when that makes the
> > > > binding really rather weird.
> > > >
> > > > Second, this new binding doesn't provide anything over and above the
> > > > existing fixed-link binding. It may not be pretty, but it is
> > > > estabilshed.
> > >
> > > Have you followed the past discussions about this patch set? Basically
> > > the *only* feedback I received on RFCv1 is that the fixed-link property
> > > sucks, and everybody (including the known Device Tree binding
> > > maintainer Mark Rutland) suggested to not use the fixed-link mechanism.
> > > See http://article.gmane.org/gmane.linux.network/276932, where Mark
> > > said:
> > >
> > > ""
> > > I'm not sure grouping these values together is the best way of handling
> > > this. It's rather opaque, and inflexible for future extension.
> > > ""
> > >
> > > So, please DT maintainers, tell me what you want. I honestly don't care
> > > whether fixed-link or a separate node is chosen. However, I do care
> > > about being dragged around between two solutions just because the
> > > former DT maintainer and the new DT maintainers do not agree.
>
> I've been sleepy on this issue, which limits how much I can push. I'll
> say one more thing on the issue (below) and then leave the decision up
> to Mark. I trust him and he knows what he is doing.
>
> > Since I would like to move forward so I can one day use that binding in a
> > real-life product, I am going to go for Mark's side which happens to be how I
> > want the binding to look like.
> >
> > Do we all agree that the new binding is just way better than the old one? In
> > light of the recent unstable DT ABI discussion, we can still continue parsing
> > the old one for the sake of compatibility.
>
> Regardless of what you want it to look like, does the old binding work
> for your purposes? If yes then use it. The only valid reason for
> creating a new binding is if the old one doesn't work for a specific
> (not theoretical) use case.
I think the issue here was that I am not versed in the history of all of
the existing bindings. While I'm not keen on the existing fixed-link
property and I think it should be done differently were it being created
from scratch today, as Grant has pointed out we're already supporting it
today, and adding a new binding is going to make the code handling it
more complex.
If fixed-link works for your use case today, then use fixed-link.
If we have a valid reason to create a new binding, we should. At the
moment I think the only egregious portion of the binding is the globally
unique fake PHY id, and if that causes issues we should be able to
assign IDs within Linux ignoring the values in the DT, or reorganise
things such that the arbitrary ID doesn't matter.
If there are configurations we need to support that the fixed-link
property cannot encode, then I think we should go ahead with the binding
style that you are proposing today. However, we don't need to go with it
right away, and we can continue to support fixed-link regardless.
I apologise for the lack of consistency here, and I'm sorry that I've
delayed this series for so long.
Thanks,
Mark.
^ permalink raw reply
* RE: [PATCH] usb: xhci: Link TRB must not occur with a USB payload burst.
From: David Laight @ 2013-11-12 16:50 UTC (permalink / raw)
To: Alan Stern
Cc: Sarah Sharp, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <Pine.LNX.4.44L0.1311121051090.1200-100000-IYeN2dnnYyZXsRXLowluHWD2FQJk+8+b@public.gmane.org>
> > If all the fragments are larger than the MBP (assume 16k) then
> > that would be relatively easy. However that is very dependant
> > on the source of the data. It might be true for disk data, but
> > is unlikely to be true for ethernet data.
>
> I don't quite understand your point. Are you saying that if all the
> TRBs are very short, you might need more than 64 TRBs to reach a 16-KB
> boundary?
Since you don't really want to do all the work twice, the sensible
way is to add each input fragment to the ring one a time.
If you need to cross a link TRB and the last MBP boundary is within
the previous data TRB then you can split the previous data TRB at the
MBP boundary and continue.
If the previous TRB is short then you'd need to go back through the
earlier TRB until you found the one that contains a TRB boundary,
split it, and write a link TRB in the following slot.
If you are within the first MBP then you'd need to replace the first
TRB of the message with a link TRB.
And yes, if the data is really fragmented you might need a lot of
TRB for even 1k of data.
> > For bulk data the link TRB can be forced at a packet boundary
> > by splitting the TD up - the receiving end won't know the difference.
>
> That won't work. What happens if you split a TD up into two pieces and
> the first piece receives a short packet? The host controller will
> automatically move to the start of the second piece. That's not what
> we want.
You can split a bulk TD on a 1k boundary and the target won't know the
difference.
> > There is no necessity for taking an interrupt from every link segment.
>
> Yes, there is. The HCD needs to know when the dequeue pointer has
> moved beyond the end of the ring segment, so that it can start reusing
> the TRB slots in that segment.
You already know that because of the interrupts for the data packets
themselves.
> > I would change the code to use a single segment (for coding simplicity)
> > and queue bulk URB when there isn't enough ring space.
> > URB with too many fragments could either be rejected, sent in sections,
> > or partially linearised (and probably still sent in sections).
>
> Rejecting an URB is not feasible. Splitting it up into multiple TDs is
> not acceptable, as explained above. Sending it in sections (i.e.,
> queueing only some of the TRBs at any time) would work, provided you
> got at least two interrupts every time the queue wrapped around (which
> suggests you might want at least two ring segments).
Rejecting badly fragmented URB is almost certainly ok. You really don't
want the hardware overhead of processing a TRB every few bytes.
This would be especially bad on iommu systems.
Before the ring expansion code was added there was an implicit
limit of (probably) 125 fragments for a URB. Exceeding this limit
wasn't the reason for adding the ring expansion code.
And, as I've pointed out, both bulk and isoc URB can be split unless
they are using too many fragments for a short piece of data.
The current code refuses to write into a TRB segment until it is
empty - I think that restriction is only there so that it can
add another segment when the space runs out.
David
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* oom-kill && frozen()
From: Oleg Nesterov @ 2013-11-12 16:56 UTC (permalink / raw)
To: Peter Zijlstra, Tejun Heo, David Rientjes
Cc: David Laight, Geert Uytterhoeven, Ingo Molnar, netdev,
linux-kernel
In-Reply-To: <20131112162136.GA29065@redhat.com>
On 11/12, Oleg Nesterov wrote:
>
> I am also wondering if it makes any sense to turn PF_FROZEN into
> TASK_FROZEN, something like (incomplete, probably racy) patch below.
> Note that it actually adds the new state, not the the qualifier.
As for the current usage of PF_FROZEN... David, it seems that
oom_scan_process_thread()->__thaw_task() is dead? Probably this
was fine before, when __thaw_task() cleared the "need to freeze"
condition, iirc it was PF_FROZEN.
But today __thaw_task() can't help, no? the task will simply
schedule() in D state again.
Oleg.
^ permalink raw reply
* Re: [PATCH] ipvs: Remove unused variable ret from sync_thread_master()
From: Peter Zijlstra @ 2013-11-12 17:00 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Tejun Heo, David Laight, Geert Uytterhoeven, Ingo Molnar, netdev,
linux-kernel
In-Reply-To: <20131112162136.GA29065@redhat.com>
On Tue, Nov 12, 2013 at 05:21:36PM +0100, Oleg Nesterov wrote:
> On 11/12, Peter Zijlstra wrote:
> >
> > On Tue, Nov 12, 2013 at 02:21:39PM -0000, David Laight wrote:
> > > Shame there isn't a process flag to indicate that the process
> > > will sleep uninterruptibly and that it doesn't matter.
> > > So don't count to the load average and don't emit a warning
> > > if it has been sleeping for a long time.
> >
> > A process flag wouldn't work, because the task could block waiting for
> > actual work to complete in other sleeps.
> >
> > However, we could do something like the below; which would allow us
> > writing things like:
> >
> > (void)___wait_event(*sk_sleep(sk),
> > sock_writeable(sk) || kthread_should_stop(),
> > TASK_UNINTERRUPTIBLE | TASK_IDLE, 0, 0,
> > schedule());
> >
> > Marking the one wait-for-more-work as TASK_IDLE such that it doesn't
> > contribute to the load avg.
>
> Agreed, I thought about additional bit too.
>
> > static const char * const task_state_array[] = {
> > - "R (running)", /* 0 */
> > - "S (sleeping)", /* 1 */
> > - "D (disk sleep)", /* 2 */
> > - "T (stopped)", /* 4 */
> > - "t (tracing stop)", /* 8 */
> > - "Z (zombie)", /* 16 */
> > - "X (dead)", /* 32 */
> > - "x (dead)", /* 64 */
> > - "K (wakekill)", /* 128 */
> > - "W (waking)", /* 256 */
> > - "P (parked)", /* 512 */
> > + "R (running)", /* 0 */
> > + "S (sleeping)", /* 1 */
> > + "D (disk sleep)", /* 2 */
> > + "T (stopped)", /* 4 */
> > + "t (tracing stop)", /* 8 */
> > + "Z (zombie)", /* 16 */
> > + "X (dead)", /* 32 */
> > + "x (dead)", /* 64 */
> > + "K (wakekill)", /* 128 */
> > + "W (waking)", /* 256 */
> > + "P (parked)", /* 512 */
> > + "I (idle)", /* 1024 */
> > };
>
> but I am not sure about what /proc/ should report in this case...
We have to put in something...
BUILD_BUG_ON(1 + ilog2(TASK_STATE_MAX) != ARRAY_SIZE(task_state_array));
However, since we always set it together with TASK_UNINTERUPTIBLE
userspace shouldn't actually ever see the I thing.
> > #define task_contributes_to_load(task) \
> > ((task->state & TASK_UNINTERRUPTIBLE) != 0 && \
> > - (task->flags & PF_FROZEN) == 0)
> > + (task->flags & PF_FROZEN) == 0 && \
> > + (task->state & TASK_IDLE) == 0)
>
> perhaps
>
> (task->state & (TASK_UNINTERRUPTIBLE | TASK_IDLE)) == TASK_UNINTERRUPTIBLE
>
> can save an insn.
Fair enough.
> I am also wondering if it makes any sense to turn PF_FROZEN into
> TASK_FROZEN, something like (incomplete, probably racy) patch below.
> Note that it actually adds the new state, not the the qualifier.
>
> --- x/include/linux/freezer.h
> +++ x/include/linux/freezer.h
> @@ -23,7 +23,7 @@ extern unsigned int freeze_timeout_msecs
> */
> static inline bool frozen(struct task_struct *p)
> {
> - return p->flags & PF_FROZEN;
> + return p->state & TASK_FROZEN;
do we want == there? Does it make sense to allow it be set with other
state flags?
> }
>
> extern bool freezing_slow_path(struct task_struct *p);
> --- x/kernel/freezer.c
> +++ x/kernel/freezer.c
> @@ -57,16 +57,13 @@ bool __refrigerator(bool check_kthr_stop
> pr_debug("%s entered refrigerator\n", current->comm);
>
> for (;;) {
> - set_current_state(TASK_UNINTERRUPTIBLE);
> -
> spin_lock_irq(&freezer_lock);
> - current->flags |= PF_FROZEN;
> - if (!freezing(current) ||
> - (check_kthr_stop && kthread_should_stop()))
> - current->flags &= ~PF_FROZEN;
> + if (freezing(current) &&
> + !(check_kthr_stop && kthread_should_stop()))
> + set_current_state(TASK_FROZEN);
> spin_unlock_irq(&freezer_lock);
>
> - if (!(current->flags & PF_FROZEN))
> + if (!(current->state & TASK_FROZEN))
> break;
> was_frozen = true;
> schedule();
> @@ -148,8 +145,7 @@ void __thaw_task(struct task_struct *p)
> * refrigerator.
> */
> spin_lock_irqsave(&freezer_lock, flags);
> - if (frozen(p))
> - wake_up_process(p);
> + try_to_wake_up(p, TASK_FROZEN, 0);
> spin_unlock_irqrestore(&freezer_lock, flags);
> }
Should work I suppose... I'm not entirely sure why that's a PF to begin
with.
^ permalink raw reply
* [PATCH] rtlwifi: Remove unused calls to rtl_is_special_data()
From: Larry Finger @ 2013-11-12 17:02 UTC (permalink / raw)
To: linville; +Cc: linux-wireless, Larry Finger, netdev, mark.cave-ayland
When routine rtl_is_special_data() is called with false as its last argument,
and the returned value is not tested, the call is essentially an extended
no-op. Accordingly, these calls may be removed.
Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
Reported-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
drivers/net/wireless/rtlwifi/pci.c | 2 --
drivers/net/wireless/rtlwifi/usb.c | 4 ----
2 files changed, 6 deletions(-)
diff --git a/drivers/net/wireless/rtlwifi/pci.c b/drivers/net/wireless/rtlwifi/pci.c
index 0f49444..8707d1a 100644
--- a/drivers/net/wireless/rtlwifi/pci.c
+++ b/drivers/net/wireless/rtlwifi/pci.c
@@ -688,8 +688,6 @@ static void _rtl_receive_one(struct ieee80211_hw *hw, struct sk_buff *skb,
rtlpriv->stats.rxbytesunicast += skb->len;
}
- rtl_is_special_data(hw, skb, false);
-
if (ieee80211_is_data(fc)) {
rtlpriv->cfg->ops->led_control(hw, LED_CTL_RX);
diff --git a/drivers/net/wireless/rtlwifi/usb.c b/drivers/net/wireless/rtlwifi/usb.c
index 6e2b5c5..ec385e4 100644
--- a/drivers/net/wireless/rtlwifi/usb.c
+++ b/drivers/net/wireless/rtlwifi/usb.c
@@ -475,8 +475,6 @@ static void _rtl_usb_rx_process_agg(struct ieee80211_hw *hw,
rtlpriv->stats.rxbytesunicast += skb->len;
}
- rtl_is_special_data(hw, skb, false);
-
if (ieee80211_is_data(fc)) {
rtlpriv->cfg->ops->led_control(hw, LED_CTL_RX);
@@ -517,8 +515,6 @@ static void _rtl_usb_rx_process_noagg(struct ieee80211_hw *hw,
rtlpriv->stats.rxbytesunicast += skb->len;
}
- rtl_is_special_data(hw, skb, false);
-
if (ieee80211_is_data(fc)) {
rtlpriv->cfg->ops->led_control(hw, LED_CTL_RX);
--
1.8.4
^ permalink raw reply related
* Re: [Fwd: Re: [PATCH v2 2/2] x86: add prefetching to do_csum]
From: Neil Horman @ 2013-11-12 17:12 UTC (permalink / raw)
To: Joe Perches
Cc: netdev, Dave Jones, linux-kernel, sebastien.dugue,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, Eric Dumazet
In-Reply-To: <1384220542.4771.23.camel@joe-AO722>
On Mon, Nov 11, 2013 at 05:42:22PM -0800, Joe Perches wrote:
> Hi again Neil.
>
> Forwarding on to netdev with a concern as to how often
> do_csum is used via csum_partial for very short headers
> and what impact any prefetch would have there.
>
> Also, what changed in your test environment?
>
> Why are the new values 5+% higher cycles/byte than the
> previous values?
>
> And here is the new table reformatted:
>
> len set iterations Readahead cachelines vs cycles/byte
> 1 2 3 4 6 10 20
> 1500B 64MB 1000000 1.4342 1.4300 1.4350 1.4350 1.4396 1.4315 1.4555
> 1500B 128MB 1000000 1.4312 1.4346 1.4271 1.4284 1.4376 1.4318 1.4431
> 1500B 256MB 1000000 1.4309 1.4254 1.4316 1.4308 1.4418 1.4304 1.4367
> 1500B 512MB 1000000 1.4534 1.4516 1.4523 1.4563 1.4554 1.4644 1.4590
> 9000B 64MB 1000000 0.8921 0.8924 0.8932 0.8949 0.8952 0.8939 0.8985
> 9000B 128MB 1000000 0.8841 0.8856 0.8845 0.8854 0.8861 0.8879 0.8861
> 9000B 256MB 1000000 0.8806 0.8821 0.8813 0.8833 0.8814 0.8827 0.8895
> 9000B 512MB 1000000 0.8838 0.8852 0.8841 0.8865 0.8846 0.8901 0.8865
> 64KB 64MB 1000000 0.8132 0.8136 0.8132 0.8150 0.8147 0.8149 0.8147
> 64KB 128MB 1000000 0.8013 0.8014 0.8013 0.8020 0.8041 0.8015 0.8033
> 64KB 256MB 1000000 0.7956 0.7959 0.7956 0.7976 0.7981 0.7967 0.7973
> 64KB 512MB 1000000 0.7934 0.7932 0.7937 0.7951 0.7954 0.7943 0.7948
>
There we go, thats better:
len set iterations Readahead cachelines vs cycles/byte
1 2 3 4 5 10 20
1500B 64MB 1000000 1.3638 1.3288 1.3464 1.3505 1.3586 1.3527 1.3408
1500B 128MB 1000000 1.3394 1.3357 1.3625 1.3456 1.3536 1.3400 1.3410
1500B 256MB 1000000 1.3773 1.3362 1.3419 1.3548 1.3543 1.3442 1.4163
1500B 512MB 1000000 1.3442 1.3390 1.3434 1.3505 1.3767 1.3513 1.3820
9000B 64MB 1000000 0.8505 0.8492 0.8521 0.8593 0.8566 0.8577 0.8547
9000B 128MB 1000000 0.8507 0.8507 0.8523 0.8627 0.8593 0.8670 0.8570
9000B 256MB 1000000 0.8516 0.8515 0.8568 0.8546 0.8549 0.8609 0.8596
9000B 512MB 1000000 0.8517 0.8526 0.8552 0.8675 0.8547 0.8526 0.8621
64KB 64MB 1000000 0.7679 0.7689 0.7688 0.7716 0.7714 0.7722 0.7716
64KB 128MB 1000000 0.7683 0.7687 0.7710 0.7690 0.7717 0.7694 0.7703
64KB 256MB 1000000 0.7680 0.7703 0.7688 0.7689 0.7726 0.7717 0.7713
64KB 512MB 1000000 0.7692 0.7690 0.7701 0.7705 0.7698 0.7693 0.7735
So, the numbers are correct now that I returned my hardware to its previous
interrupt affinity state, but the trend seems to be the same (namely that there
isn't a clear one). We seem to find peak performance around a readahead of 2
cachelines, but its very small (about 3%), and its inconsistent (larger set
sizes fall to either side of that stride). So I don't see it as a clear win. I
still think we should probably scrap the readahead for now, just take the perf
bits, and revisit this when we can use the vector instructions or the
independent carry chain instructions to improve this more consistently.
Thoughts
Neil
^ permalink raw reply
* Re: [PATCH] usb: xhci: Link TRB must not occur with a USB payload burst.
From: Sarah Sharp @ 2013-11-12 17:15 UTC (permalink / raw)
To: Alan Stern
Cc: David Laight, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <Pine.LNX.4.44L0.1311111525480.17105-100000-pYrvlCTfrz9XsRXLowluHWD2FQJk+8+b@public.gmane.org>
On Mon, Nov 11, 2013 at 03:34:53PM -0500, Alan Stern wrote:
> On Mon, 11 Nov 2013, David Laight wrote:
>
> > > Suppose, for example, the MBP is 1024. If you have a TD with length
> > > 1500, and if it had only one fragment, the last (and only) fragment's
> > > length would not less than the MBP and it would not be an exact
> > > multiple of the MBP.
> >
> > That doesn't matter - eg example 2 in figure 25
>
> You're right. I do wish the spec had been written more clearly.
>
> > Reading it all again makes me think that a LINK trb is only
> > allowed on the burst boundary (which might be 16k bytes).
> > The only real way to implement that is to ensure that TD never
> > contain LINK TRB.
>
> That's one way to do it. Or you could allow a Link TRB at an
> intermediate MBP boundary.
I like this idea instead. The xHCI driver should be modified to be able
to handle link TRBs in the middle of the segments (the cancellation code
would have to be touched as well). We would keep a running count
of the number of bytes left in a TD fragment, as we fill in the TRBs.
If we find the TD fragment would span a link TRB, we backtrack to the
end of the last TD fragment, put in a link TRB, and then continue on the
next segment.
> It comes down to a question of how often you want the controller to
> issue an interrupt. If a ring segment is 4 KB (one page), then it can
> hold 256 TRBs. With scatter-gather transfers, each SG element
> typically refers to something like a 2-page buffer (depends on how
> fragmented the memory is). Therefore a ring segment will describe
> somewhere around 512 pages of data, i.e., something like 2 MB. Since
> SuperSpeed is 500 MB/s, you'd end up getting in the vicinity of 250
> interrupts every second just because of ring segment crossings.
The driver is currently defined to have 64 TRBs per ring segment. But
that doesn't matter; we don't get an interrupt when a ring segment is
crossed. Instead we set the interrupt-on-completion flag on the last
TRB of the TD, not on any earlier fragment or link TRB.
> Using larger ring segments would help.
Ring segments have to be physically contiguous, so I'm not sure if we
want to ask for segments that are bigger than a page. I've already got
a report from someone else about the ring expansion getting out of
control, so I would like to figure that out before we talk about using
even bigger segments.
Finally, it's interesting to note that the USB mass storage driver is
using scatter gather lists just fine without the driver following the TD
fragment rules. Or at least no one has reported any issues. I wonder
why it works?
Sarah Sharp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] usb: xhci: Link TRB must not occur with a USB payload burst.
From: Sarah Sharp @ 2013-11-12 17:15 UTC (permalink / raw)
To: David Laight
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, linux-usb-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <AE90C24D6B3A694183C094C60CF0A2F6026B73F1-CgBM+Bx2aUAnGFn1LkZF6NBPR1lH4CV8@public.gmane.org>
On Fri, Nov 08, 2013 at 11:07:41AM -0000, David Laight wrote:
> > While this change improves things a lot (it runs for 20 minutes rather than
> > a few seconds), there is still something else wrong.
>
> Almost certainly caused by an unrelated local change.
Ok, good, I was concerned there was another issue we were missing.
Sarah Sharp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* RE: [PATCH] usb: xhci: Link TRB must not occur with a USB payload burst.
From: David Laight @ 2013-11-12 17:28 UTC (permalink / raw)
To: Sarah Sharp
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, linux-usb-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20131112171549.GB4925@xanatos>
> On Fri, Nov 08, 2013 at 11:07:41AM -0000, David Laight wrote:
> > > While this change improves things a lot (it runs for 20 minutes rather than
> > > a few seconds), there is still something else wrong.
> >
> > Almost certainly caused by an unrelated local change.
>
> Ok, good, I was concerned there was another issue we were missing.
My modified code wasn't setting the td_size properly.
That is fixed and verified in the later patch I sent.
David
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [Fwd: Re: [PATCH v2 2/2] x86: add prefetching to do_csum]
From: Joe Perches @ 2013-11-12 17:33 UTC (permalink / raw)
To: Neil Horman
Cc: netdev, Dave Jones, linux-kernel, sebastien.dugue,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, Eric Dumazet
In-Reply-To: <20131112171239.GC19780@hmsreliant.think-freely.org>
On Tue, 2013-11-12 at 12:12 -0500, Neil Horman wrote:
> On Mon, Nov 11, 2013 at 05:42:22PM -0800, Joe Perches wrote:
> > Hi again Neil.
> >
> > Forwarding on to netdev with a concern as to how often
> > do_csum is used via csum_partial for very short headers
> > and what impact any prefetch would have there.
> >
> > Also, what changed in your test environment?
> >
> > Why are the new values 5+% higher cycles/byte than the
> > previous values?
> >
> > And here is the new table reformatted:
> >
> > len set iterations Readahead cachelines vs cycles/byte
> > 1 2 3 4 6 10 20
> > 1500B 64MB 1000000 1.4342 1.4300 1.4350 1.4350 1.4396 1.4315 1.4555
> > 1500B 128MB 1000000 1.4312 1.4346 1.4271 1.4284 1.4376 1.4318 1.4431
> > 1500B 256MB 1000000 1.4309 1.4254 1.4316 1.4308 1.4418 1.4304 1.4367
> > 1500B 512MB 1000000 1.4534 1.4516 1.4523 1.4563 1.4554 1.4644 1.4590
> > 9000B 64MB 1000000 0.8921 0.8924 0.8932 0.8949 0.8952 0.8939 0.8985
> > 9000B 128MB 1000000 0.8841 0.8856 0.8845 0.8854 0.8861 0.8879 0.8861
> > 9000B 256MB 1000000 0.8806 0.8821 0.8813 0.8833 0.8814 0.8827 0.8895
> > 9000B 512MB 1000000 0.8838 0.8852 0.8841 0.8865 0.8846 0.8901 0.8865
> > 64KB 64MB 1000000 0.8132 0.8136 0.8132 0.8150 0.8147 0.8149 0.8147
> > 64KB 128MB 1000000 0.8013 0.8014 0.8013 0.8020 0.8041 0.8015 0.8033
> > 64KB 256MB 1000000 0.7956 0.7959 0.7956 0.7976 0.7981 0.7967 0.7973
> > 64KB 512MB 1000000 0.7934 0.7932 0.7937 0.7951 0.7954 0.7943 0.7948
> >
>
>
> There we go, thats better:
> len set iterations Readahead cachelines vs cycles/byte
> 1 2 3 4 5 10 20
> 1500B 64MB 1000000 1.3638 1.3288 1.3464 1.3505 1.3586 1.3527 1.3408
> 1500B 128MB 1000000 1.3394 1.3357 1.3625 1.3456 1.3536 1.3400 1.3410
> 1500B 256MB 1000000 1.3773 1.3362 1.3419 1.3548 1.3543 1.3442 1.4163
> 1500B 512MB 1000000 1.3442 1.3390 1.3434 1.3505 1.3767 1.3513 1.3820
> 9000B 64MB 1000000 0.8505 0.8492 0.8521 0.8593 0.8566 0.8577 0.8547
> 9000B 128MB 1000000 0.8507 0.8507 0.8523 0.8627 0.8593 0.8670 0.8570
> 9000B 256MB 1000000 0.8516 0.8515 0.8568 0.8546 0.8549 0.8609 0.8596
> 9000B 512MB 1000000 0.8517 0.8526 0.8552 0.8675 0.8547 0.8526 0.8621
> 64KB 64MB 1000000 0.7679 0.7689 0.7688 0.7716 0.7714 0.7722 0.7716
> 64KB 128MB 1000000 0.7683 0.7687 0.7710 0.7690 0.7717 0.7694 0.7703
> 64KB 256MB 1000000 0.7680 0.7703 0.7688 0.7689 0.7726 0.7717 0.7713
> 64KB 512MB 1000000 0.7692 0.7690 0.7701 0.7705 0.7698 0.7693 0.7735
>
>
> So, the numbers are correct now that I returned my hardware to its previous
> interrupt affinity state, but the trend seems to be the same (namely that there
> isn't a clear one). We seem to find peak performance around a readahead of 2
> cachelines, but its very small (about 3%), and its inconsistent (larger set
> sizes fall to either side of that stride). So I don't see it as a clear win. I
> still think we should probably scrap the readahead for now, just take the perf
> bits, and revisit this when we can use the vector instructions or the
> independent carry chain instructions to improve this more consistently.
>
> Thoughts
Perhaps a single prefetch, not of the first addr but of
the addr after PREFETCH_STRIDE would work best but only
if length is > PREFETCH_STRIDE.
I'd try:
if (len > PREFETCH_STRIDE)
prefetch(buf + PREFETCH_STRIDE);
while (count64) {
etc...
}
I still don't know how much that impacts very short lengths.
Can you please add a 20 byte length to your tests?
^ permalink raw reply
* RE: [PATCH] usb: xhci: Link TRB must not occur with a USB payload burst.
From: David Laight @ 2013-11-12 17:33 UTC (permalink / raw)
To: Sarah Sharp, Alan Stern
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, linux-usb-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20131112171438.GA4925@xanatos>
> > That's one way to do it. Or you could allow a Link TRB at an
> > intermediate MBP boundary.
>
> I like this idea instead. The xHCI driver should be modified to be able
> to handle link TRBs in the middle of the segments (the cancellation code
> would have to be touched as well). We would keep a running count
> of the number of bytes left in a TD fragment, as we fill in the TRBs.
> If we find the TD fragment would span a link TRB, we backtrack to the
> end of the last TD fragment, put in a link TRB, and then continue on the
> next segment.
I'd do that as a later change.
It needs a fair amount of other stuff fixing first and the current
code is rather broken - and needs a fix for stable.
> > It comes down to a question of how often you want the controller to
> > issue an interrupt. If a ring segment is 4 KB (one page), then it can
> > hold 256 TRBs. With scatter-gather transfers, each SG element
> > typically refers to something like a 2-page buffer (depends on how
> > fragmented the memory is). Therefore a ring segment will describe
> > somewhere around 512 pages of data, i.e., something like 2 MB. Since
> > SuperSpeed is 500 MB/s, you'd end up getting in the vicinity of 250
> > interrupts every second just because of ring segment crossings.
>
> The driver is currently defined to have 64 TRBs per ring segment. But
> that doesn't matter; we don't get an interrupt when a ring segment is
> crossed. Instead we set the interrupt-on-completion flag on the last
> TRB of the TD, not on any earlier fragment or link TRB.
>
> > Using larger ring segments would help.
>
> Ring segments have to be physically contiguous, so I'm not sure if we
> want to ask for segments that are bigger than a page. I've already got
> a report from someone else about the ring expansion getting out of
> control, so I would like to figure that out before we talk about using
> even bigger segments.
I'd vote for a single segment containing either 128 or 256 TRBs.
That is less than a single page.
> Finally, it's interesting to note that the USB mass storage driver is
> using scatter gather lists just fine without the driver following the TD
> fragment rules. Or at least no one has reported any issues. I wonder
> why it works?
I suspect that breaking the rules just generates two TD.
The mass storage driver is probably almost always presenting
buffer fragments that are page sized and page aligned.
In which case the split is invisible at the target.
David
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [RFC] Revert "sierra_net: keep status interrupt URB active"
From: Dan Williams @ 2013-11-12 17:37 UTC (permalink / raw)
To: Bjørn Mork
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, linux-usb-u79uwXL29TY76Z2rM5mHXA,
John Henderson
In-Reply-To: <1384273510.14773.0.camel-wKZy7rqYPVb5EHUCmHmTqw@public.gmane.org>
On Tue, 2013-11-12 at 10:25 -0600, Dan Williams wrote:
> On Fri, 2013-11-08 at 15:29 -0600, Dan Williams wrote:
> > On Fri, 2013-11-08 at 21:44 +0100, Bjørn Mork wrote:
> > > Dan Williams <dcbw-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> writes:
> > > > On Mon, 2013-11-04 at 14:27 -0600, Dan Williams wrote:
> > > >> On Fri, 2013-11-01 at 13:53 +0100, Bjørn Mork wrote:
> > > >> > This reverts commit 7b0c5f21f348a66de495868b8df0284e8dfd6bbf.
> > > >> >
> > > >> > It's not easy to create a driver for all the various firmware
> > > >> > bugs out there.
> > > >> >
> > > >> > This change caused regressions for a number of devices, which
> > > >> > started to fail link detection and therefore became completely
> > > >> > non-functional. The exact reason is yet unknown, it looks like
> > > >> > the affected firmwares might actually need all or some of the
> > > >> > additional SYNC messages the patch got rid of.
> > > >> >
> > > >> > Reverting is not optimal, as it will re-introduce the original
> > > >> > problem, but it is currently the only alternative known to fix
> > > >> > this issue.
> > > >>
> > > >> Instead, how does the following patch work for you?
> > > >
> > > > Bjorn, did you have a chance to try this patch out on your devices?
> > >
> > > The only DirectIP device I have is the MC7710, which never had any of
> > > the firmware issues you are trying to fix. I only tried to forward Johns
> > > issue.
> > >
> > > When this patch worked for John, then I am pretty confident that you
> > > have solved the problem here.
> >
> > Well, "solved", since I still have no idea why the original patch would
> > cause the device behavior based on what I know and have read about the
> > expected firmware/host handshaking sequence. But the patch there
> > appears to fix the problem *and* not blindly send tons of SYNCs forever.
> >
> > So I'll go ahead and submit a proper version of it.
>
> Actually, is "[PATCH] usbnet: fix status interrupt urb handling" the
> real fix for this problem?
>
> John, any chance you could revert my RFC patch and try Felix's patch in
> that mail?
It fixes the problem for me without requiring the revert or my RFC
patch....
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] usbnet: fix status interrupt urb handling
From: Dan Williams @ 2013-11-12 17:37 UTC (permalink / raw)
To: Felix Fietkau
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, linux-usb-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1384270481-30972-1-git-send-email-nbd-p3rKhJxN3npAfugRpC6u6w@public.gmane.org>
On Tue, 2013-11-12 at 16:34 +0100, Felix Fietkau wrote:
> Since commit 7b0c5f21f348a66de495868b8df0284e8dfd6bbf
> "sierra_net: keep status interrupt URB active", sierra_net triggers
> status interrupt polling before the net_device is opened (in order to
> properly receive the sync message response).
>
> To be able to receive further interrupts, the interrupt urb needs to be
> re-submitted, so this patch removes the bogus check for netif_running().
>
> Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Signed-off-by: Felix Fietkau <nbd-p3rKhJxN3npAfugRpC6u6w@public.gmane.org>
Tested-by: Dan Williams <dcbw-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
> drivers/net/usb/usbnet.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index 90a429b..8494bb5 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -204,9 +204,6 @@ static void intr_complete (struct urb *urb)
> break;
> }
>
> - if (!netif_running (dev->net))
> - return;
> -
> status = usb_submit_urb (urb, GFP_ATOMIC);
> if (status != 0)
> netif_err(dev, timer, dev->net,
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [RFC PATCHv2 3/4] of: provide a binding for fixed link PHYs
From: Florian Fainelli @ 2013-11-12 17:40 UTC (permalink / raw)
To: Mark Rutland
Cc: Grant Likely, Thomas Petazzoni, Lior Amsalem,
devicetree@vger.kernel.org, netdev@vger.kernel.org, Sascha Hauer,
Christian Gmeiner, Ezequiel Garcia, Gregory Clement,
David S. Miller, linux-arm-kernel@lists.infradead.org
In-Reply-To: <20131112162914.GE4237@e106331-lin.cambridge.arm.com>
2013/11/12 Mark Rutland <mark.rutland@arm.com>:
> Hi Florian, Grant,
>
> On Tue, Nov 12, 2013 at 12:37:46PM +0000, Grant Likely wrote:
>> On Fri, 25 Oct 2013 05:40:57 +0100, Florian Fainelli <florian@openwrt.org> wrote:
>> > Le mercredi 18 septembre 2013, 18:11:12 Thomas Petazzoni a écrit :
>> > > Dear Grant Likely,
>> > >
>> > > On Tue, 17 Sep 2013 23:29:23 -0500, Grant Likely wrote:
>> > > > I understand what you're trying to do here, but it causes a troublesome
>> > > > leakage of implementation detail into the binding, making the whole
>> > > > thing look very odd. This binding tries to make a fixed link look
>> > > > exactly like a real PHY even to the point of including a phandle to the
>> > > > phy. But having a phandle to a node which is *always* a direct child of
>> > > > the MAC node is redundant and a rather looney. Yes, doing it that way
>> > > > makes it easy for of_phy_find_device() to be transparent for fixed link,
>> > > > but that should *not* drive bindings, especially when that makes the
>> > > > binding really rather weird.
>> > > >
>> > > > Second, this new binding doesn't provide anything over and above the
>> > > > existing fixed-link binding. It may not be pretty, but it is
>> > > > estabilshed.
>> > >
>> > > Have you followed the past discussions about this patch set? Basically
>> > > the *only* feedback I received on RFCv1 is that the fixed-link property
>> > > sucks, and everybody (including the known Device Tree binding
>> > > maintainer Mark Rutland) suggested to not use the fixed-link mechanism.
>> > > See http://article.gmane.org/gmane.linux.network/276932, where Mark
>> > > said:
>> > >
>> > > ""
>> > > I'm not sure grouping these values together is the best way of handling
>> > > this. It's rather opaque, and inflexible for future extension.
>> > > ""
>> > >
>> > > So, please DT maintainers, tell me what you want. I honestly don't care
>> > > whether fixed-link or a separate node is chosen. However, I do care
>> > > about being dragged around between two solutions just because the
>> > > former DT maintainer and the new DT maintainers do not agree.
>>
>> I've been sleepy on this issue, which limits how much I can push. I'll
>> say one more thing on the issue (below) and then leave the decision up
>> to Mark. I trust him and he knows what he is doing.
>>
>> > Since I would like to move forward so I can one day use that binding in a
>> > real-life product, I am going to go for Mark's side which happens to be how I
>> > want the binding to look like.
>> >
>> > Do we all agree that the new binding is just way better than the old one? In
>> > light of the recent unstable DT ABI discussion, we can still continue parsing
>> > the old one for the sake of compatibility.
>>
>> Regardless of what you want it to look like, does the old binding work
>> for your purposes? If yes then use it. The only valid reason for
>> creating a new binding is if the old one doesn't work for a specific
>> (not theoretical) use case.
>
> I think the issue here was that I am not versed in the history of all of
> the existing bindings. While I'm not keen on the existing fixed-link
> property and I think it should be done differently were it being created
> from scratch today, as Grant has pointed out we're already supporting it
> today, and adding a new binding is going to make the code handling it
> more complex.
>
> If fixed-link works for your use case today, then use fixed-link.
Like I said in an earlier response to Thomas yesterday, it does not
cover everything, most importantly, the existing "fixed-link" property
does not allow the representation of direct MAC to MAC connections
where you still need to specify the MII connection type for it to work
properly. This is something that can be easily provided by the new
fixed PHY binding, but less easily by the existing "fixed-link"
property. We cannot extend "fixed-link" to contain a 6th integer
because "phy-mode" or "phy-connection-type" is a string. My options
here are pretty simple:
- use the existing "fixed-link" property even though it is not nice,
it does the job
- add a supplemental "connection-type" property and parse it in my
driver, which is where it is going to be used
- go on with the new proposed Device Tree binding
>
> If we have a valid reason to create a new binding, we should. At the
> moment I think the only egregious portion of the binding is the globally
> unique fake PHY id, and if that causes issues we should be able to
> assign IDs within Linux ignoring the values in the DT, or reorganise
> things such that the arbitrary ID doesn't matter.
I think we all agreed that the PHY ID (the term should be clarified as
it tends to either mean PHY address or PHY OUI) had to be gone, and
this is what Thomas did in this serie. Whichever binding we choose, it
should be fairly easy to keep the "PHY ID" integer in the "fixed-link"
property but make the code ignore it (with possibly a big fat warning
for a transition period).
>
> If there are configurations we need to support that the fixed-link
> property cannot encode, then I think we should go ahead with the binding
> style that you are proposing today. However, we don't need to go with it
> right away, and we can continue to support fixed-link regardless.
Well, yes, the lack of "connection-type" representation is a blocker
for some hardware configurations where two Ethernet MACs are connected
one to each other. Whether this deserves a new incarnation of the
"fixed PHY" Device Tree binding is left for discussion.
>
> I apologise for the lack of consistency here, and I'm sorry that I've
> delayed this series for so long.
--
Florian
^ permalink raw reply
* Re: [PATCH net 2/2] macvtap: limit head length of skb allocated
From: Greg Rose @ 2013-11-12 17:45 UTC (permalink / raw)
To: Jason Wang; +Cc: davem, netdev, linux-kernel, mst, stefanha
In-Reply-To: <1384250577-20330-2-git-send-email-jasowang@redhat.com>
On Tue, 12 Nov 2013 18:02:57 +0800
Jason Wang <jasowang@redhat.com> wrote:
> We currently use hdr_len as a hint of head length which is advertised
> by guest. But when guest advertise a very big value, it can lead to
> an 64K+ allocating of kmalloc() which has a very high possibility of
> failure when host memory is fragmented or under heavy stress. The
> huge hdr_len also reduce the effect of zerocopy or even disable if a
> gso skb is linearized in guest.
>
> To solves those issues, this patch introduces an upper limit
> (PAGE_SIZE) of the head, which guarantees an order 0 allocation each
> time.
>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> The patch was needed for stable.
> ---
> drivers/net/macvtap.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> index 9dccb1e..7ee6f9d 100644
> --- a/drivers/net/macvtap.c
> +++ b/drivers/net/macvtap.c
> @@ -523,6 +523,11 @@ static inline struct sk_buff
> *macvtap_alloc_skb(struct sock *sk, size_t prepad, int noblock, int
> *err) {
> struct sk_buff *skb;
> + int good_linear = SKB_MAX_HEAD(prepad);
> +
> + /* Don't use huge linear part */
> + if (linear > good_linear)
> + linear = good_linear;
>
> /* Under a page? Don't bother with paged skb. */
> if (prepad + len < PAGE_SIZE || !linear)
I see no problem with this or the tuntap patch except that in both
cases kernel coding style would prefer that you align the local
variable declarations in a reverse pyramid, longest at the beginning,
shortest at the end.
- Greg
^ permalink raw reply
* Re: [PATCH] ipvs: Remove unused variable ret from sync_thread_master()
From: Oleg Nesterov @ 2013-11-12 18:04 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Tejun Heo, David Laight, Geert Uytterhoeven, Ingo Molnar, netdev,
linux-kernel
In-Reply-To: <20131112170043.GB16796@laptop.programming.kicks-ass.net>
On 11/12, Peter Zijlstra wrote:
>
> On Tue, Nov 12, 2013 at 05:21:36PM +0100, Oleg Nesterov wrote:
> > On 11/12, Peter Zijlstra wrote:
> > >
> > > static const char * const task_state_array[] = {
> > > ...
> > > + "I (idle)", /* 1024 */
> > > };
> >
> > but I am not sure about what /proc/ should report in this case...
>
> We have to put in something...
>
> BUILD_BUG_ON(1 + ilog2(TASK_STATE_MAX) != ARRAY_SIZE(task_state_array));
>
> However, since we always set it together with TASK_UNINTERUPTIBLE
> userspace shouldn't actually ever see the I thing.
OOPS. I didn't know that get_task_state() does &= TASK_REPORT. So it
can never report anything > EXIT_DEAD.
Perhaps we should change BUILD_BUG_ON() and shrink task_state_array?
--- x/include/linux/sched.h
+++ x/include/linux/sched.h
@@ -163,7 +163,7 @@ extern char ___assert_task_state[1 - 2*!
/* get_task_state() */
#define TASK_REPORT (TASK_RUNNING | TASK_INTERRUPTIBLE | \
TASK_UNINTERRUPTIBLE | __TASK_STOPPED | \
- __TASK_TRACED)
+ __TASK_TRACED | EXIT_ZOMBIE | EXIT_DEAD)
#define task_is_traced(task) ((task->state & __TASK_TRACED) != 0)
#define task_is_stopped(task) ((task->state & __TASK_STOPPED) != 0)
--- x/fs/proc/array.c
+++ x/fs/proc/array.c
@@ -148,16 +148,12 @@ static const char * const task_state_arr
static inline const char *get_task_state(struct task_struct *tsk)
{
- unsigned int state = (tsk->state & TASK_REPORT) | tsk->exit_state;
+ unsigned int state = (tsk->state | tsk->exit_state) & TASK_REPORT;
const char * const *p = &task_state_array[0];
- BUILD_BUG_ON(1 + ilog2(TASK_STATE_MAX) != ARRAY_SIZE(task_state_array));
+ BUILD_BUG_ON(1 + ilog2(TASK_REPORT) != ARRAY_SIZE(task_state_array));
- while (state) {
- p++;
- state >>= 1;
- }
- return *p;
+ return task_state_array[fls(state)];
}
static inline void task_state(struct seq_file *m, struct pid_namespace *ns,
> > I am also wondering if it makes any sense to turn PF_FROZEN into
> > TASK_FROZEN, something like (incomplete, probably racy) patch below.
> > Note that it actually adds the new state, not the the qualifier.
> >
> > --- x/include/linux/freezer.h
> > +++ x/include/linux/freezer.h
> > @@ -23,7 +23,7 @@ extern unsigned int freeze_timeout_msecs
> > */
> > static inline bool frozen(struct task_struct *p)
> > {
> > - return p->flags & PF_FROZEN;
> > + return p->state & TASK_FROZEN;
>
> do we want == there?
Not really, but if we add TASK_FROZEN then we should probably
add task_is_frozen() just for consistency (frozen() should use
this helper) and all task_is_* helpers do "&".
And,
> Does it make sense to allow it be set with other
> state flags?
Not sure, but _perhaps_ TASK_FROZEN | TASK_KILLABLE can be used
too, say, we can add CGROUP_FROZEN_KILLABLE. Probably makes no
sense, I dunno.
> > --- x/kernel/freezer.c
> > +++ x/kernel/freezer.c
> > @@ -57,16 +57,13 @@ bool __refrigerator(bool check_kthr_stop
> > pr_debug("%s entered refrigerator\n", current->comm);
> >
> > for (;;) {
> > - set_current_state(TASK_UNINTERRUPTIBLE);
> > -
> > spin_lock_irq(&freezer_lock);
> > - current->flags |= PF_FROZEN;
> > - if (!freezing(current) ||
> > - (check_kthr_stop && kthread_should_stop()))
> > - current->flags &= ~PF_FROZEN;
> > + if (freezing(current) &&
> > + !(check_kthr_stop && kthread_should_stop()))
> > + set_current_state(TASK_FROZEN);
> > spin_unlock_irq(&freezer_lock);
> >
> > - if (!(current->flags & PF_FROZEN))
> > + if (!(current->state & TASK_FROZEN))
> > break;
> > was_frozen = true;
> > schedule();
> > @@ -148,8 +145,7 @@ void __thaw_task(struct task_struct *p)
> > * refrigerator.
> > */
> > spin_lock_irqsave(&freezer_lock, flags);
> > - if (frozen(p))
> > - wake_up_process(p);
> > + try_to_wake_up(p, TASK_FROZEN, 0);
> > spin_unlock_irqrestore(&freezer_lock, flags);
> > }
>
> Should work I suppose...
And perhaps we can simplify this a bit. Not sure we can kill freezer_lock
altogether, but at least __thaw_task() can avoid it.
But I am still not sure the new TASK_ state really makes sense, even if
it looks a bit more clean to me.
OTOH, personally I think that /proc/pid/status should report "F (frozen)"
if frozen(), but this doesn't need TASK_FROZEN of course.
> I'm not entirely sure why that's a PF to begin
> with.
Oh, it had more PF_'s until tj improved (and cleanuped) this code ;)
Oleg.
^ permalink raw reply
* Bug#511720: [PATCH iproute2] ss: avoid passing negative numbers to malloc
From: Andreas Henriksson @ 2013-11-12 18:52 UTC (permalink / raw)
To: stephen; +Cc: netdev
Example:
$ ss state established \( sport = :4060 or sport = :4061 or sport = :4062 or sport = :4063 or sport = :4064 or sport = :4065 or sport = :4066 or sport = :4067 \) > /dev/null
Aborted
In the example above ssfilter_bytecompile(...) will return (int)136.
char l1 = 136; means -120 which will result in a negative number
being passed to malloc at misc/ss.c:913.
Simply declare l1 and l2 as intergers to avoid the char overflow.
This is one of the issues originally reported in http://bugs.debian.org/511720
Reported-by: Andreas Schuldei <andreas@debian.org>
Signed-off-by: Andreas Henriksson <andreas@fatal.se>
---
misc/ss.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/misc/ss.c b/misc/ss.c
index c0369f1..db3a3a4 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -907,7 +907,8 @@ static int ssfilter_bytecompile(struct ssfilter *f, char **bytecode)
}
case SSF_OR:
{
- char *a1, *a2, *a, l1, l2;
+ char *a1, *a2, *a;
+ int l1, l2;
l1 = ssfilter_bytecompile(f->pred, &a1);
l2 = ssfilter_bytecompile(f->post, &a2);
if (!(a = malloc(l1+l2+4))) abort();
--
1.8.4.3
^ permalink raw reply related
* RE: [PATCH] usb: xhci: Link TRB must not occur with a USB payload burst.
From: Alan Stern @ 2013-11-12 19:00 UTC (permalink / raw)
To: David Laight; +Cc: Sarah Sharp, netdev, linux-usb
In-Reply-To: <AE90C24D6B3A694183C094C60CF0A2F6026B7407@saturn3.aculab.com>
On Tue, 12 Nov 2013, David Laight wrote:
> > > If all the fragments are larger than the MBP (assume 16k) then
> > > that would be relatively easy. However that is very dependant
> > > on the source of the data. It might be true for disk data, but
> > > is unlikely to be true for ethernet data.
> >
> > I don't quite understand your point. Are you saying that if all the
> > TRBs are very short, you might need more than 64 TRBs to reach a 16-KB
> > boundary?
>
> Since you don't really want to do all the work twice, the sensible
> way is to add each input fragment to the ring one a time.
> If you need to cross a link TRB and the last MBP boundary is within
> the previous data TRB then you can split the previous data TRB at the
> MBP boundary and continue.
> If the previous TRB is short then you'd need to go back through the
> earlier TRB until you found the one that contains a TRB boundary,
> split it, and write a link TRB in the following slot.
Right. You could avoid doing a lot of work twice by using two passes.
In the first pass, keep track of the number of bytes allotted to each
TRB and the MBP boundaries, without doing anything else. That way,
you'll know where to put a Link TRB without any need for backtracking.
Then in the second pass, fill in the actual contents of the TRBs.
> If you are within the first MBP then you'd need to replace the first
> TRB of the message with a link TRB.
Unless the first TRB of the message is the first TRB of the ring
segment. Then you're in trouble... Hopefully, real URBs will never be
that badly fragmented.
> And yes, if the data is really fragmented you might need a lot of
> TRB for even 1k of data.
Until somebody needs more than 16 TRBs for each KB of data, we should
be okay.
> > > For bulk data the link TRB can be forced at a packet boundary
> > > by splitting the TD up - the receiving end won't know the difference.
> >
> > That won't work. What happens if you split a TD up into two pieces and
> > the first piece receives a short packet? The host controller will
> > automatically move to the start of the second piece. That's not what
> > we want.
>
> You can split a bulk TD on a 1k boundary and the target won't know the
> difference.
The target won't know the difference, but the host will. Here's an
example: Suppose the driver submits two URBs, each for a data-in
transfer of 32 KB. You split each URB up into two 16-KB TDs; let's
call them A, B, C, and D (where A and B make up the first URB, and C
and D make up the second URB).
Now suppose the device terminates the first transfer after only 7200
bytes, with a short packet. It then sends a complete 32 KB of data for
the second transfer. What will happen in this case?
7200 bytes is somewhere in the middle of TD A. That TD will terminate
early. The host controller will then proceed to TD B. As a result,
the next 32 KB of data will be stored in TD B and C -- not in TD C and
D as it should be.
This example shows why you must not split IN URBs into multiple TDs.
> > > There is no necessity for taking an interrupt from every link segment.
> >
> > Yes, there is. The HCD needs to know when the dequeue pointer has
> > moved beyond the end of the ring segment, so that it can start reusing
> > the TRB slots in that segment.
>
> You already know that because of the interrupts for the data packets
> themselves.
I don't know what you mean by this. The host controller does not
generate an interrupt for each data packet. In generates interrupts
only for TRBs with the IOC bit set (which is generally the last TRB in
each TD).
Suppose you have only two ring segments, and a driver submits an URB
which is so fragmented that it requires more TRBs than you have room
for in those two segments. When do you want the interrupts to arrive?
Answer: At each segment crossing.
> > > I would change the code to use a single segment (for coding simplicity)
> > > and queue bulk URB when there isn't enough ring space.
> > > URB with too many fragments could either be rejected, sent in sections,
> > > or partially linearised (and probably still sent in sections).
> >
> > Rejecting an URB is not feasible. Splitting it up into multiple TDs is
> > not acceptable, as explained above. Sending it in sections (i.e.,
> > queueing only some of the TRBs at any time) would work, provided you
> > got at least two interrupts every time the queue wrapped around (which
> > suggests you might want at least two ring segments).
>
> Rejecting badly fragmented URB is almost certainly ok. You really don't
> want the hardware overhead of processing a TRB every few bytes.
> This would be especially bad on iommu systems.
I disagree. Sure, it would be bad. But if you can handle it, you
should.
> Before the ring expansion code was added there was an implicit
> limit of (probably) 125 fragments for a URB. Exceeding this limit
> wasn't the reason for adding the ring expansion code.
>
> And, as I've pointed out, both bulk and isoc URB can be split unless
> they are using too many fragments for a short piece of data.
>
> The current code refuses to write into a TRB segment until it is
> empty - I think that restriction is only there so that it can
> add another segment when the space runs out.
That's right. The idea was my suggestion.
Alan Stern
^ permalink raw reply
* [PATCH] net/7990: Make lance_private.name const
From: Geert Uytterhoeven @ 2013-11-12 19:14 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, linux-m68k, Geert Uytterhoeven
This allows to drop a few casts.
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
drivers/net/ethernet/amd/7990.h | 2 +-
drivers/net/ethernet/amd/hplance.c | 2 +-
drivers/net/ethernet/amd/mvme147.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/amd/7990.h b/drivers/net/ethernet/amd/7990.h
index 0a5837b96421..60c60926afda 100644
--- a/drivers/net/ethernet/amd/7990.h
+++ b/drivers/net/ethernet/amd/7990.h
@@ -100,7 +100,7 @@ struct lance_init_block {
*/
struct lance_private
{
- char *name;
+ const char *name;
unsigned long base;
volatile struct lance_init_block *init_block; /* CPU address of RAM */
volatile struct lance_init_block *lance_init_block; /* LANCE address of RAM */
diff --git a/drivers/net/ethernet/amd/hplance.c b/drivers/net/ethernet/amd/hplance.c
index 0c61fd50d882..a9046cf3a564 100644
--- a/drivers/net/ethernet/amd/hplance.c
+++ b/drivers/net/ethernet/amd/hplance.c
@@ -149,7 +149,7 @@ static void hplance_init(struct net_device *dev, struct dio_dev *d)
}
lp = netdev_priv(dev);
- lp->lance.name = (char*)d->name; /* discards const, shut up gcc */
+ lp->lance.name = d->name;
lp->lance.base = va;
lp->lance.init_block = (struct lance_init_block *)(va + HPLANCE_MEMOFF); /* CPU addr */
lp->lance.lance_init_block = NULL; /* LANCE addr of same RAM */
diff --git a/drivers/net/ethernet/amd/mvme147.c b/drivers/net/ethernet/amd/mvme147.c
index e108e911da05..b5d76b45176e 100644
--- a/drivers/net/ethernet/amd/mvme147.c
+++ b/drivers/net/ethernet/amd/mvme147.c
@@ -120,7 +120,7 @@ struct net_device * __init mvme147lance_probe(int unit)
return ERR_PTR(-ENOMEM);
}
- lp->lance.name = (char*)name; /* discards const, shut up gcc */
+ lp->lance.name = name;
lp->lance.base = dev->base_addr;
lp->lance.init_block = (struct lance_init_block *)(lp->ram); /* CPU addr */
lp->lance.lance_init_block = (struct lance_init_block *)(lp->ram); /* LANCE addr of same RAM */
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH] usb: xhci: Link TRB must not occur with a USB payload burst.
From: Alan Stern @ 2013-11-12 19:22 UTC (permalink / raw)
To: Sarah Sharp; +Cc: David Laight, netdev, linux-usb
In-Reply-To: <20131112171438.GA4925@xanatos>
On Tue, 12 Nov 2013, Sarah Sharp wrote:
> > It comes down to a question of how often you want the controller to
> > issue an interrupt. If a ring segment is 4 KB (one page), then it can
> > hold 256 TRBs. With scatter-gather transfers, each SG element
> > typically refers to something like a 2-page buffer (depends on how
> > fragmented the memory is). Therefore a ring segment will describe
> > somewhere around 512 pages of data, i.e., something like 2 MB. Since
> > SuperSpeed is 500 MB/s, you'd end up getting in the vicinity of 250
> > interrupts every second just because of ring segment crossings.
>
> The driver is currently defined to have 64 TRBs per ring segment. But
A TRB is 16 bytes, right? So a page can hold 256 TRBs. Why use only
64 per segment?
> that doesn't matter; we don't get an interrupt when a ring segment is
> crossed. Instead we set the interrupt-on-completion flag on the last
> TRB of the TD, not on any earlier fragment or link TRB.
That's because you don't worry about handling URBs which require too
many TRBs (i.e., more than are available). You just add more ring
segments. Instead, you could re-use segments on the fly.
For example, suppose you have only two ring segments and you get an URB
which requires enough TRBs to fill up four segments. You could fill in
the first two segments worth, and get an interrupt when the controller
traverses the Link TRB between them. At that point you store the third
set of TRBs in the first segment, which is now vacant. Similarly, when
the second Link TRB is traversed, you fill in the fourth set of TRBs.
> > Using larger ring segments would help.
>
> Ring segments have to be physically contiguous, so I'm not sure if we
> want to ask for segments that are bigger than a page. I've already got
> a report from someone else about the ring expansion getting out of
> control, so I would like to figure that out before we talk about using
> even bigger segments.
Maybe you can get away with fewer segments, if they are bigger.
> Finally, it's interesting to note that the USB mass storage driver is
> using scatter gather lists just fine without the driver following the TD
> fragment rules. Or at least no one has reported any issues. I wonder
> why it works?
I'd guess this is because the hardware is actually a lot more flexible
than the "No Link TRBs in the middle of a TD fragment" rule.
The whole idea of TD fragments makes no sense to begin with. What
point is there in grouping packets into MaxBurst-sized collections?
The hardware does not have to finish one burst before beginning the
next one. For example, suppose the MaxBurst size is 8. The host
starts by bursting packets 1-8. When it receives the ACK for packet 4,
the host could then burst packets 9-12. It doesn't have to wait for
the ACK to packet 8. (Unless I have misunderstood the USB-3 spec.)
If the host does this, the burst boundaries won't occur on MBP
boundaries, and hence won't occur on TD fragment boundaries. The
fragment boundaries will be essentially meaningless.
Alan Stern
^ permalink raw reply
* Re: [PATCH net] netlink: fix netlink_ack with large messages
From: David Miller @ 2013-11-12 19:35 UTC (permalink / raw)
To: jbenc; +Cc: pablo, jhs, tgraf, netdev
In-Reply-To: <20131112162907.50f0bafa@griffin>
From: Jiri Benc <jbenc@redhat.com>
Date: Tue, 12 Nov 2013 16:29:07 +0100
> On Sat, 9 Nov 2013 19:03:53 +0100, Pablo Neira Ayuso wrote:
>> On Sat, Nov 09, 2013 at 08:43:51AM -0500, Jamal Hadi Salim wrote:
>> > for errors, we need to give the user something back. This has been the
>> > behavior for 80 years now. Giving them a HUGE message
>> > back is rediculuos(tm). Ive had enough of SCTP doing that.
>> > We need to cap it - sort of what ICMP does.
>> > ICMP caps at 64B; something like 128B is reasonable.
>>
>> Personally, I have only used the sequence number to correlate the
>> original request with the ack reply, so I agree in that trimming it to
>> some reasonable amount of bytes like ICMP is the way to go. I prefer
>> if we select a large enough amount of bytes to avoid breaking backward
>> compatibility, eg. 128KB, since I'm not sure what kind of handling
>> others may have done of this.
>
> Do you think capping at NLMSG_GOODSIZE would be too low? The allocation
> won't fit into one page with NLMSG_GOODSIZE but I doubt we can go lower
> than that. Alternatively, we can do some math to fully use the two
> pages, like
> NLMSG_GOODSIZE + min(PAGE_SIZE, 8192UL) - NLMSG_HDRLEN - NLMSG_ALIGN(sizeof(struct nlmsgerr))
> (which I'm not sure is worth it).
I don't think this is the way to go.
I think since existing apps expect the whole message, we have to
provide it.
We should add a new socket option so that applications can ask that
messages not be quoted in ACKs, as I've stated a few times already in
this thread.
^ permalink raw reply
* Re: [Fwd: Re: [PATCH v2 2/2] x86: add prefetching to do_csum]
From: Neil Horman @ 2013-11-12 19:50 UTC (permalink / raw)
To: Joe Perches
Cc: netdev, Dave Jones, linux-kernel, sebastien.dugue,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, Eric Dumazet
In-Reply-To: <1384277615.3665.10.camel@joe-AO722>
On Tue, Nov 12, 2013 at 09:33:35AM -0800, Joe Perches wrote:
> On Tue, 2013-11-12 at 12:12 -0500, Neil Horman wrote:
> > On Mon, Nov 11, 2013 at 05:42:22PM -0800, Joe Perches wrote:
> > > Hi again Neil.
> > >
> > > Forwarding on to netdev with a concern as to how often
> > > do_csum is used via csum_partial for very short headers
> > > and what impact any prefetch would have there.
> > >
> > > Also, what changed in your test environment?
> > >
> > > Why are the new values 5+% higher cycles/byte than the
> > > previous values?
> > >
> > > And here is the new table reformatted:
> > >
> > > len set iterations Readahead cachelines vs cycles/byte
> > > 1 2 3 4 6 10 20
> > > 1500B 64MB 1000000 1.4342 1.4300 1.4350 1.4350 1.4396 1.4315 1.4555
> > > 1500B 128MB 1000000 1.4312 1.4346 1.4271 1.4284 1.4376 1.4318 1.4431
> > > 1500B 256MB 1000000 1.4309 1.4254 1.4316 1.4308 1.4418 1.4304 1.4367
> > > 1500B 512MB 1000000 1.4534 1.4516 1.4523 1.4563 1.4554 1.4644 1.4590
> > > 9000B 64MB 1000000 0.8921 0.8924 0.8932 0.8949 0.8952 0.8939 0.8985
> > > 9000B 128MB 1000000 0.8841 0.8856 0.8845 0.8854 0.8861 0.8879 0.8861
> > > 9000B 256MB 1000000 0.8806 0.8821 0.8813 0.8833 0.8814 0.8827 0.8895
> > > 9000B 512MB 1000000 0.8838 0.8852 0.8841 0.8865 0.8846 0.8901 0.8865
> > > 64KB 64MB 1000000 0.8132 0.8136 0.8132 0.8150 0.8147 0.8149 0.8147
> > > 64KB 128MB 1000000 0.8013 0.8014 0.8013 0.8020 0.8041 0.8015 0.8033
> > > 64KB 256MB 1000000 0.7956 0.7959 0.7956 0.7976 0.7981 0.7967 0.7973
> > > 64KB 512MB 1000000 0.7934 0.7932 0.7937 0.7951 0.7954 0.7943 0.7948
> > >
> >
> >
> > There we go, thats better:
> > len set iterations Readahead cachelines vs cycles/byte
> > 1 2 3 4 5 10 20
> > 1500B 64MB 1000000 1.3638 1.3288 1.3464 1.3505 1.3586 1.3527 1.3408
> > 1500B 128MB 1000000 1.3394 1.3357 1.3625 1.3456 1.3536 1.3400 1.3410
> > 1500B 256MB 1000000 1.3773 1.3362 1.3419 1.3548 1.3543 1.3442 1.4163
> > 1500B 512MB 1000000 1.3442 1.3390 1.3434 1.3505 1.3767 1.3513 1.3820
> > 9000B 64MB 1000000 0.8505 0.8492 0.8521 0.8593 0.8566 0.8577 0.8547
> > 9000B 128MB 1000000 0.8507 0.8507 0.8523 0.8627 0.8593 0.8670 0.8570
> > 9000B 256MB 1000000 0.8516 0.8515 0.8568 0.8546 0.8549 0.8609 0.8596
> > 9000B 512MB 1000000 0.8517 0.8526 0.8552 0.8675 0.8547 0.8526 0.8621
> > 64KB 64MB 1000000 0.7679 0.7689 0.7688 0.7716 0.7714 0.7722 0.7716
> > 64KB 128MB 1000000 0.7683 0.7687 0.7710 0.7690 0.7717 0.7694 0.7703
> > 64KB 256MB 1000000 0.7680 0.7703 0.7688 0.7689 0.7726 0.7717 0.7713
> > 64KB 512MB 1000000 0.7692 0.7690 0.7701 0.7705 0.7698 0.7693 0.7735
> >
> >
> > So, the numbers are correct now that I returned my hardware to its previous
> > interrupt affinity state, but the trend seems to be the same (namely that there
> > isn't a clear one). We seem to find peak performance around a readahead of 2
> > cachelines, but its very small (about 3%), and its inconsistent (larger set
> > sizes fall to either side of that stride). So I don't see it as a clear win. I
> > still think we should probably scrap the readahead for now, just take the perf
> > bits, and revisit this when we can use the vector instructions or the
> > independent carry chain instructions to improve this more consistently.
> >
> > Thoughts
>
> Perhaps a single prefetch, not of the first addr but of
> the addr after PREFETCH_STRIDE would work best but only
> if length is > PREFETCH_STRIDE.
>
> I'd try:
>
> if (len > PREFETCH_STRIDE)
> prefetch(buf + PREFETCH_STRIDE);
> while (count64) {
> etc...
> }
>
> I still don't know how much that impacts very short lengths.
>
> Can you please add a 20 byte length to your tests?
>
>
Sure, I modified the code so that we only prefetched 2 cache lines ahead, but
only if the overall length of the input buffer is more than 2 cache lines.
Below are the results (all counts are the average of 1000000 iterations of the
csum operation, as previous tests were, I just omitted that column).
len set cycles/byte cycles/byte improvement
no prefetch prefetch
===========================================================
20B 64MB 45.014989 44.402432 1.3%
20B 128MB 44.900317 46.146447 -2.7%
20B 256MB 45.303223 48.193623 -6.3%
20B 512MB 45.615301 44.486872 2.2%
1500B 64MB 1.364365 1.332285 1.9%
1500B 128MB 1.373945 1.335907 1.4%
1500B 256MB 1.356971 1.339084 1.2%
1500B 512MB 1.351091 1.341431 0.7%
9000B 64MB 0.850966 0.851077 -0.1%
9000B 128MB 0.851013 0.850366 0.1%
9000B 256MB 0.854212 0.851033 0.3%
9000B 512MB 0.857346 0.851744 0.7%
64KB 64MB 0.768224 0.768450 ~0%
64KB 128MB 0.768706 0.768884 ~0%
64KB 256MB 0.768459 0.768445 ~0%
64KB 512MB 0.768808 0.769404 -0.1%
The 20 byte results seem to have a few outliers. I'm guessing the improvement
came from good fortune in that the random selection happened to hit on the same
range of numbers a few times over, so we hit already cached data. I would
expect them to run more slowly (as the 2 and 3 rows illustrate), since 20B is
less than the 128 bytes in 2 cachelines on my test system, and so all were doing
is adding in an additional comparison and jump per iteration. Our sweet spot is
the 1500 byte range, giving us a small performance boost, but that quickly gets
lost in the noise as the buffer size grows beyond that.
I'm still left thinking we should just abandon the prefetch at this point and
keep the perf code until we have new instructions to help us with this further,
unless you see something I dont.
Neil
^ 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