* [PATCH BUGFIX 1/6] pkt_sched: properly cap timestamps in charge_actual_service
From: Paolo valente @ 2013-03-05 18:04 UTC (permalink / raw)
To: Jamal Hadi Salim, David S. Miller, shemminger
Cc: netdev, linux-kernel, fchecconi, rizzo, Paolo Valente
In-Reply-To: <1362506702-4985-1-git-send-email-paolo.valente@unimore.it>
QFQ+ schedules the active aggregates in a group using a bucket list
(one list per group). The bucket in which each aggregate is inserted
depends on the aggregate's timestamps, and the number
of buckets in a group is enough to accomodate the possible (range of)
values of the timestamps of all the aggregates in the group. For this
property to hold, timestamps must however be computed correctly. One
necessary condition for computing timestamps correctly is that the
number of bits dequeued for each aggregate, while the aggregate is in
service, does not exceed the maximum budget budgetmax assigned to the
aggregate.
For each aggregate, budgetmax is proportional to the number of classes
in the aggregate. If the number of classes of the aggregate is
decreased through qfq_change_class(), then budgetmax is decreased
automatically as well. Problems may occur if the aggregate is in
service when budgetmax is decreased, because the current remaining
budget of the aggregate and/or the service already received by the
aggregate may happen to be larger than the new value of budgetmax. In
this case, when the aggregate is eventually deselected and its
timestamps are updated, the aggregate may happen to have received an
amount of service larger than budgetmax. This may cause the aggregate
to be assigned a higher virtual finish time than the maximum
acceptable value for the last bucket in the bucket list of the group.
This fix introduces a cap that addresses this issue.
Signed-off-by: Paolo Valente <paolo.valente@unimore.it>
Reviewed-by: Fabio Checconi <fchecconi@gmail.com>
---
net/sched/sch_qfq.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/net/sched/sch_qfq.c b/net/sched/sch_qfq.c
index 6ed3765..0f6e2db 100644
--- a/net/sched/sch_qfq.c
+++ b/net/sched/sch_qfq.c
@@ -299,6 +299,10 @@ static void qfq_update_agg(struct qfq_sched *q, struct qfq_aggregate *agg,
new_num_classes == q->max_agg_classes - 1) /* agg no more full */
hlist_add_head(&agg->nonfull_next, &q->nonfull_aggs);
+ /* The next assignment may let
+ * agg->initial_budget > agg->budgetmax
+ * hold, we will take it into account in charge_actual_service().
+ */
agg->budgetmax = new_num_classes * agg->lmax;
new_agg_weight = agg->class_weight * new_num_classes;
agg->inv_w = ONE_FP/new_agg_weight;
@@ -990,8 +994,13 @@ static inline struct sk_buff *qfq_peek_skb(struct qfq_aggregate *agg,
/* Update F according to the actual service received by the aggregate. */
static inline void charge_actual_service(struct qfq_aggregate *agg)
{
- /* compute the service received by the aggregate */
- u32 service_received = agg->initial_budget - agg->budget;
+ /* Compute the service received by the aggregate, taking into
+ * account that, after decreasing the number of classes in
+ * agg, it may happen that
+ * agg->initial_budget - agg->budget > agg->bugdetmax
+ */
+ u32 service_received = min(agg->budgetmax,
+ agg->initial_budget - agg->budget);
agg->F = agg->S + (u64)service_received * agg->inv_w;
}
--
1.7.9.5
^ permalink raw reply related
* [PATCH BUGFIX 0/6] pkt_sched: fix little service anomalies and possible crashes of qfq+
From: Paolo valente @ 2013-03-05 18:04 UTC (permalink / raw)
To: Jamal Hadi Salim, David S. Miller, shemminger
Cc: netdev, linux-kernel, fchecconi, rizzo, Paolo Valente
In-Reply-To: <20130226.173700.882998504986475149.davem@davemloft.net>
Il 26/02/2013 23:37, David Miller ha scritto:
> From: Paolo valente <paolo.valente@unimore.it>
> Date: Tue, 26 Feb 2013 18:02:46 +0100
>
>> The portions of the code interested by each fix are small and do not
>> overlap with each other, so I decided to provide just one patch
>> (I hope that this was the right choice).
>
> Please split this up into 6 patches, each with an appropriately
> verbose analysis and explanation of each bug being fixed, thanks.
>
>
Split, and inserted a detailed description of both the problem and the fix
in each patch.
Paolo valente (6):
pkt_sched: properly cap timestamps in charge_actual_service
pkt_sched: fix the update of eligible-group sets
pkt_sched: serve activated aggregates immediately if the scheduler is
empty
pkt_sched: prevent budget from wrapping around after a dequeue
pkt_sched: do not allow virtual time to jump if an aggregate is in
service
pkt_sched: remove a useless invocation of qfq_update_eligible
net/sched/sch_qfq.c | 66 +++++++++++++++++++++++++++++++++++----------------
1 file changed, 45 insertions(+), 21 deletions(-)
--
1.7.9.5
^ permalink raw reply
* Re: [PATCH V2] lglock: add read-preference local-global rwlock
From: Srivatsa S. Bhat @ 2013-03-05 17:55 UTC (permalink / raw)
To: Lai Jiangshan
Cc: Michel Lespinasse, Oleg Nesterov, Lai Jiangshan, linux-doc,
peterz, fweisbec, linux-kernel, namhyung, mingo, linux-arch,
linux, xiaoguangrong, wangyun, paulmck, nikunj, linux-pm, rusty,
rostedt, rjw, vincent.guittot, tglx, linux-arm-kernel, netdev,
sbw, tj, akpm, linuxppc-dev
In-Reply-To: <5136123C.3040508@cn.fujitsu.com>
On 03/05/2013 09:11 PM, Lai Jiangshan wrote:
> On 03/03/13 01:11, Srivatsa S. Bhat wrote:
>> On 03/02/2013 06:44 PM, Lai Jiangshan wrote:
>>> From 345a7a75c314ff567be48983e0892bc69c4452e7 Mon Sep 17 00:00:00 2001
>>> From: Lai Jiangshan <laijs@cn.fujitsu.com>
>>> Date: Sat, 2 Mar 2013 20:33:14 +0800
>>> Subject: [PATCH] lglock: add read-preference local-global rwlock
>>>
>>> Current lglock is not read-preference, so it can't be used on some cases
>>> which read-preference rwlock can do. Example, get_cpu_online_atomic().
>>>
>> [...]
>>> diff --git a/kernel/lglock.c b/kernel/lglock.c
>>> index 6535a66..52e9b2c 100644
>>> --- a/kernel/lglock.c
>>> +++ b/kernel/lglock.c
>>> @@ -87,3 +87,71 @@ void lg_global_unlock(struct lglock *lg)
>>> preempt_enable();
>>> }
>>> EXPORT_SYMBOL(lg_global_unlock);
>>> +
>>> +#define FALLBACK_BASE (1UL << 30)
>>> +
>>> +void lg_rwlock_local_read_lock(struct lgrwlock *lgrw)
>>> +{
>>> + struct lglock *lg = &lgrw->lglock;
>>> +
>>> + preempt_disable();
>>> + if (likely(!__this_cpu_read(*lgrw->reader_refcnt))) {
>>> + rwlock_acquire_read(&lg->lock_dep_map, 0, 0, _RET_IP_);
>>> + if (unlikely(!arch_spin_trylock(this_cpu_ptr(lg->lock)))) {
>>> + read_lock(&lgrw->fallback_rwlock);
>>> + __this_cpu_write(*lgrw->reader_refcnt, FALLBACK_BASE);
>>> + return;
>>> + }
>>> + }
>>> +
>>> + __this_cpu_inc(*lgrw->reader_refcnt);
>>> +}
>>> +EXPORT_SYMBOL(lg_rwlock_local_read_lock);
>>> +
>>> +void lg_rwlock_local_read_unlock(struct lgrwlock *lgrw)
>>> +{
>>> + switch (__this_cpu_read(*lgrw->reader_refcnt)) {
>>> + case 1:
>>> + __this_cpu_write(*lgrw->reader_refcnt, 0);
>>> + lg_local_unlock(&lgrw->lglock);
>>> + return;
>>
>> This should be a break, instead of a return, right?
>> Otherwise, there will be a preempt imbalance...
>
>
> "lockdep" and "preempt" are handled in lg_local_unlock(&lgrw->lglock);
>
Ah, ok.. I had missed that.
Regards,
Srivatsa S. Bhat
^ permalink raw reply
* Re: [PATCH 4/7] usbnet: cdc_mbim: don't recover device if suspend fails in system sleep
From: Bjørn Mork @ 2013-03-05 17:35 UTC (permalink / raw)
To: Alan Stern
Cc: Ming Lei, David S. Miller, Greg Kroah-Hartman, Jiri Kosina,
Oliver Neukum, netdev, linux-usb, linux-input
In-Reply-To: <Pine.LNX.4.44L0.1303051147300.1308-100000@iolanthe.rowland.org>
Alan Stern <stern@rowland.harvard.edu> writes:
> On Tue, 5 Mar 2013, Bjørn Mork wrote:
>> Ming Lei <ming.lei@canonical.com> writes:
>>
>> > Yes, USB core will flush any outstanding URBs, but the driver still need
>> > to deal with suspend failure carefully, for example, suppose usb_resume()
>> > is called in suspend failure path, and the submitted URBs are killed
>> > by USB core later. But after the device is wakeup, and the resume() will
>> > do nothing since the suspend count is leaked. So it is what the patches
>> > are fixing, and it is better to not depend on the default flushing URBs of
>> > USB core.
>>
>> I am starting to wonder why the USB core has combined system suspend and
>> runtime suspend if we are going to end up with every driver testing
>> PMSG_IS_AUTO(message) and selecting a completely different code path.
>
> Mainly for historical reasons. System suspend existed long before
> runtime suspend did. When runtime suspend was added, it piggybacked
> off the existing code. Furthermore, originally there was no
> requirement that system suspend always succeed; that was added later.
>
> Also, the code paths are not completely different. They differ mainly
> in their error handling. But when you think about it, how serious an
> error can you encounter when you try to _stop_ using a device?
Thanks for explaining.
>> You are right that we will end up with problems if usbnet_resume is
>> called for a device usbnet hasn't suspended. But I'd still claim that
>> is a bug in the USB core, which is the one that decided to ignore the
>> suspend error and still call resume.
>>
>> I guess proper error handling here require the USB core to see the
>> interface driver as dead if it fails to suspend on system suspend, and
>> do forced rebinding on resume.
>
> You are welcome to submit a patch to do this. It shouldn't be hard; we
> already have a flag indicating that an interface needs to be unbound at
> reprobed at resume time. You can update the kerneldoc in addition; as
> you noticed, it currently does not describe the actual code completely
> accurately.
I guess I saw that coming :) Will take a shot at it when time permits.
Bjørn
^ permalink raw reply
* Re: [RFC PATCH 3/5] ixgbe: Add support for ndo_ll_poll
From: Eric Dumazet @ 2013-03-05 17:36 UTC (permalink / raw)
To: Ben Hutchings
Cc: Eliezer Tamir, Willem de Bruijn, e1000-devel, netdev,
Jesse Brandeburg, linux-kernel, Andi Kleen, HPA, Eliezer Tamir,
Dave Miller
In-Reply-To: <1362504520.2791.46.camel@bwh-desktop.uk.solarflarecom.com>
On Tue, 2013-03-05 at 17:28 +0000, Ben Hutchings wrote:
> On Tue, 2013-03-05 at 17:26 +0000, Ben Hutchings wrote:
> > On Wed, 2013-02-27 at 09:56 -0800, Eliezer Tamir wrote:
> > > Add the ixgbe driver code implementing ndo_ll_poll.
> > > It should be easy for other drivers to do something similar
> > > in order to enable support for CONFIG_INET_LL_RX_POLL
> >
> > Yes... in fact I wonder whether the lock and state couldn't be added to
> > napi_struct instead of being driver-specific.
> [...]
>
> That would potentially allow GRO to detect that it's being called in
> low-latency context...
Not sure we have to add another test in fast path.
Users wanting polling support should disable GRO (ethtool -K eth0 gro
off)
Doing this dynamically would be nice of course.
------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_feb
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
^ permalink raw reply
* Re: [RFC PATCH 3/5] ixgbe: Add support for ndo_ll_poll
From: Ben Hutchings @ 2013-03-05 17:28 UTC (permalink / raw)
To: Eliezer Tamir
Cc: Willem de Bruijn, e1000-devel, netdev, Jesse Brandeburg,
linux-kernel, Andi Kleen, HPA, Eliezer Tamir, Dave Miller
In-Reply-To: <1362504410.2791.44.camel@bwh-desktop.uk.solarflarecom.com>
On Tue, 2013-03-05 at 17:26 +0000, Ben Hutchings wrote:
> On Wed, 2013-02-27 at 09:56 -0800, Eliezer Tamir wrote:
> > Add the ixgbe driver code implementing ndo_ll_poll.
> > It should be easy for other drivers to do something similar
> > in order to enable support for CONFIG_INET_LL_RX_POLL
>
> Yes... in fact I wonder whether the lock and state couldn't be added to
> napi_struct instead of being driver-specific.
[...]
That would potentially allow GRO to detect that it's being called in
low-latency context...
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_feb
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
^ permalink raw reply
* Re: [RFC PATCH 3/5] ixgbe: Add support for ndo_ll_poll
From: Ben Hutchings @ 2013-03-05 17:26 UTC (permalink / raw)
To: Eliezer Tamir
Cc: Willem de Bruijn, e1000-devel, netdev, Jesse Brandeburg,
linux-kernel, Andi Kleen, HPA, Eliezer Tamir, Dave Miller
In-Reply-To: <20130227175606.10611.55498.stgit@gitlad.jf.intel.com>
On Wed, 2013-02-27 at 09:56 -0800, Eliezer Tamir wrote:
> Add the ixgbe driver code implementing ndo_ll_poll.
> It should be easy for other drivers to do something similar
> in order to enable support for CONFIG_INET_LL_RX_POLL
Yes... in fact I wonder whether the lock and state couldn't be added to
napi_struct instead of being driver-specific.
[...]
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
[...]
> static void ixgbe_napi_disable_all(struct ixgbe_adapter *adapter)
> {
> int q_idx;
>
> - for (q_idx = 0; q_idx < adapter->num_q_vectors; q_idx++)
> + local_bh_disable(); /* for ixgbe_qv_lock_napi() */
> + for (q_idx = 0; q_idx < adapter->num_q_vectors; q_idx++) {
> napi_disable(&adapter->q_vector[q_idx]->napi);
> + while (!ixgbe_qv_lock_napi(adapter->q_vector[q_idx])) {
> + pr_info("QV %d locked\n", q_idx);
> + msleep(1);
> + }
> + }
> + local_bh_enable();
> }
Sleeping with preemption disabled?
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_feb
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
^ permalink raw reply
* Re: [RFC PATCH 1/5] net: implement support for low latency socket polling
From: Eliezer Tamir @ 2013-03-05 17:15 UTC (permalink / raw)
To: Ben Hutchings
Cc: Eliezer Tamir, linux-kernel, netdev, Dave Miller,
Jesse Brandeburg, e1000-devel, Willem de Bruijn, Andi Kleen, HPA,
Eliezer Tamir
In-Reply-To: <1362501781.2791.19.camel@bwh-desktop.uk.solarflarecom.com>
On 05/03/2013 18:43, Ben Hutchings wrote:
> On Wed, 2013-02-27 at 09:55 -0800, Eliezer Tamir wrote:
>
> Should the units really be cycles or, say, microseconds? I assume that
> a sysctl setter can do a conversion to cycles so that there's no need to
> multiply every time the value is used. (If the CPU doesn't have
> constant_tsc or equivalent then this conversion doesn't quite work, but
> then low-latency tunng usually includes disabling frequency scaling.)
We are not very sensitive to this setting, anything on the order of your
half round time trip plus a few standard deviations works well.
We are busy waiting, so setting a higher value does not change the
results much.
It does make sense to have this in ms, and it might not matter if the
dynamic cycles mess with the value too much.
BTW on my machines enabling frequency scaling improves performance in
many cases.
> Also, this should be a per-device (or even per-NAPI-context?) setting.
Again, I would expect this to depend more on your workload than on the
NIC, so I would keep this global.
User knobs should be as simple as possible.
>>+int sysctl_net_ll_poll __read_mostly = 150000;
> Nicely tuned for your specific test system, no doubt. :-)
why don't you try this on your NIC and see ;-)
Thanks for the input,
Eliezer
^ permalink raw reply
* [PATCH] net: reduce net_rx_action() latency to 2 HZ
From: Eric Dumazet @ 2013-03-05 17:15 UTC (permalink / raw)
To: David Miller; +Cc: netdev
From: Eric Dumazet <edumazt@google.com>
We should use time_after_eq() to get maximum latency of two ticks,
instead of three.
Bug added in commit 24f8b2385 (net: increase receive packet quantum)
Signed-off-by: Eric Dumazet <edumazt@google.com>
---
net/core/dev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index 18d8b5a..461b315 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4107,7 +4107,7 @@ static void net_rx_action(struct softirq_action *h)
* Allow this to run for 2 jiffies since which will allow
* an average latency of 1.5/HZ.
*/
- if (unlikely(budget <= 0 || time_after(jiffies, time_limit)))
+ if (unlikely(budget <= 0 || time_after_eq(jiffies, time_limit)))
goto softnet_break;
local_irq_enable();
^ permalink raw reply related
* Re: [RFC PATCH 2/5] tcp: add TCP support for low latency receive poll.
From: Ben Hutchings @ 2013-03-05 17:13 UTC (permalink / raw)
To: Eliezer Tamir
Cc: Willem de Bruijn, e1000-devel, netdev, Jesse Brandeburg,
linux-kernel, Andi Kleen, HPA, Eliezer Tamir, Dave Miller
In-Reply-To: <20130227175601.10611.99536.stgit@gitlad.jf.intel.com>
On Wed, 2013-02-27 at 09:56 -0800, Eliezer Tamir wrote:
> an example of how one could add support for ndo_ll_poll to TCP.
[...]
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -279,6 +279,7 @@
>
> #include <asm/uaccess.h>
> #include <asm/ioctls.h>
> +#include <net/ll_poll.h>
>
> int sysctl_tcp_fin_timeout __read_mostly = TCP_FIN_TIMEOUT;
>
> @@ -1475,6 +1476,17 @@ int tcp_read_sock(struct sock *sk, read_descriptor_t *desc,
>
> if (sk->sk_state == TCP_LISTEN)
> return -ENOTCONN;
> +
> +#ifdef CONFIG_INET_LL_TCP_POLL
> +/* TODO: what do we do if the state changes after sk_poll_ll()? */
Maybe this bit should be a separate function that the callers can use
(or not) before calling tcp_read_sock(). Then they must take care of
re-locking and revalidating if it decides to poll.
Ben.
> + if (sk_valid_ll(sk) && skb_queue_empty(&sk->sk_receive_queue)
> + && (sk->sk_state == TCP_ESTABLISHED)) {
> +
> + release_sock(sk);
> + sk_poll_ll(sk);
> + lock_sock(sk);
> + }
> +#endif
> while ((skb = tcp_recv_skb(sk, seq, &offset)) != NULL) {
> if (offset < skb->len) {
> int used;
[...]
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_feb
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
^ permalink raw reply
* Re: [PATCH] net/rds: using strlcpy instead of strncpy
From: Ben Hutchings @ 2013-03-05 17:00 UTC (permalink / raw)
To: Chen Gang
Cc: David Laight, venkat.x.venkatsubra, David Miller, rds-devel,
netdev
In-Reply-To: <51356FED.4070909@asianux.com>
On Tue, 2013-03-05 at 12:09 +0800, Chen Gang wrote:
> 于 2013年03月05日 11:37, Ben Hutchings 写道:
> >> I think what I have done is just like your choice "2."
> >> > for me, I think they are equal:
> >> >
> >> > - strncpy(ctr.name, names[i], sizeof(ctr.name) - 1);
> >> > + strlcpy(ctr.name, names[i], sizeof(ctr.name));
> >> >
> >> >
> >> > strncpy(ctr.name, names[i], sizeof(ctr.name) - 1);
> >> > + ctr.name[sizeof(ctr.name) - 1] = '\0';
> > They are not. strncpy() pads with zeroes to the end of the given buffer
> > whereas strlcpy() adds only a single zero byte (and truncates if
> > necessary to fit the zero byte).
>
> ok, thank you. they are really not the same (originally, I did not
> notice it)
>
> could you supply the reason:
> why need we zero all of ctr.name ?
> (for me, I think, keeping ctr.name just a zero-based string is ok)
This function calls rds_copy_info() to copy the whole of ctr into
userland.
If ctr is not completely initialised, then the values of the
uninitialised bytes are left over from the local variables of an earlier
system call. If an attacker knows enough about the stack layout (easy
if this is a distribution kernel), they can make a series of system
calls that leak information about heap-allocated objects. That can help
them to exploit other kernel bugs for privilege escalation. So we
should initialise every bit of memory that is going to be copied to
userland.
(In fact, in general it's not even enough to initialise all fields of
the structure, because there may be padding bytes between them. In this
case we know there isn't, because it's declared as packed.)
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH 4/7] usbnet: cdc_mbim: don't recover device if suspend fails in system sleep
From: Alan Stern @ 2013-03-05 16:54 UTC (permalink / raw)
To: Bjørn Mork
Cc: Ming Lei, David S. Miller, Greg Kroah-Hartman, Jiri Kosina,
Oliver Neukum, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <87wqtlommw.fsf-lbf33ChDnrE/G1V5fR+Y7Q@public.gmane.org>
On Tue, 5 Mar 2013, Bjørn Mork wrote:
> Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> writes:
>
> > Yes, USB core will flush any outstanding URBs, but the driver still need
> > to deal with suspend failure carefully, for example, suppose usb_resume()
> > is called in suspend failure path, and the submitted URBs are killed
> > by USB core later. But after the device is wakeup, and the resume() will
> > do nothing since the suspend count is leaked. So it is what the patches
> > are fixing, and it is better to not depend on the default flushing URBs of
> > USB core.
>
> I am starting to wonder why the USB core has combined system suspend and
> runtime suspend if we are going to end up with every driver testing
> PMSG_IS_AUTO(message) and selecting a completely different code path.
Mainly for historical reasons. System suspend existed long before
runtime suspend did. When runtime suspend was added, it piggybacked
off the existing code. Furthermore, originally there was no
requirement that system suspend always succeed; that was added later.
Also, the code paths are not completely different. They differ mainly
in their error handling. But when you think about it, how serious an
error can you encounter when you try to _stop_ using a device?
> You are right that we will end up with problems if usbnet_resume is
> called for a device usbnet hasn't suspended. But I'd still claim that
> is a bug in the USB core, which is the one that decided to ignore the
> suspend error and still call resume.
>
> I guess proper error handling here require the USB core to see the
> interface driver as dead if it fails to suspend on system suspend, and
> do forced rebinding on resume.
You are welcome to submit a patch to do this. It shouldn't be hard; we
already have a flag indicating that an interface needs to be unbound at
reprobed at resume time. You can update the kerneldoc in addition; as
you noticed, it currently does not describe the actual code completely
accurately.
Alan Stern
--
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 PATCH 1/5] net: implement support for low latency socket polling
From: Ben Hutchings @ 2013-03-05 16:43 UTC (permalink / raw)
To: Eliezer Tamir
Cc: Willem de Bruijn, e1000-devel, netdev, Jesse Brandeburg,
linux-kernel, Andi Kleen, HPA, Eliezer Tamir, Dave Miller
In-Reply-To: <20130227175555.10611.42794.stgit@gitlad.jf.intel.com>
On Wed, 2013-02-27 at 09:55 -0800, Eliezer Tamir wrote:
> Adds a new ndo_ll_poll method and the code that supports and uses it.
> This method can be used by low latency applications to busy poll ethernet
> device queues directly from the socket code. The ip_low_latency_poll sysctl
> entry controls how many cycles to poll. Set to zero to disable.
[...]
> --- /dev/null
> +++ b/include/net/ll_poll.h
> @@ -0,0 +1,71 @@
> +/*
> + * low latency device queue flush
> + */
> +
> +#ifndef _LINUX_NET_LL_POLL_H
> +#define _LINUX_NET_LL_POLL_H
> +#ifdef CONFIG_INET_LL_RX_POLL
> +#include <linux/netdevice.h>
> +struct napi_struct;
> +extern int sysctl_net_ll_poll __read_mostly;
> +
> +/* return values from ndo_ll_poll */
> +#define LL_FLUSH_DONE 0
> +#define LL_FLUSH_FAILED 1
> +#define LL_FLUSH_BUSY 2
> +
> +static inline int sk_valid_ll(struct sock *sk)
bool
> +{
> + return sysctl_net_ll_poll && sk->dev_ref &&
> + !need_resched() && !signal_pending(current);
> +}
> +
> +/*
> + * TODO: how do we know that we have a working get_cycles?
> + * do we limit this by a configure dependacy?
In general it appears to require a run-time check. You might need to
augment <asm/timex.h>.
> + * TODO: this is not safe when the device can be removed,
> + * but simple refcounting may prevent removal indefinatly
> + */
> +static inline int sk_poll_ll(struct sock *sk)
> +{
> + struct napi_struct *napi = sk->dev_ref;
> + const struct net_device_ops *ops;
> + unsigned long end_time = sysctl_net_ll_poll + get_cycles();
ACCESS_ONCE(sysctl_net_ll_poll)
> + if (!napi->dev || !napi->dev->netdev_ops ||
> + !napi->dev->netdev_ops->ndo_ll_poll)
> + return false;
> +
> + local_bh_disable();
> +
> + ops = napi->dev->netdev_ops;
> + while (skb_queue_empty(&sk->sk_receive_queue) &&
> + !time_after((unsigned long)get_cycles(), end_time))
cycles_t may be narrower than unsigned long, in which case time_after()
will not compare correctly. I think you need to open-code the
equivalent of time_after() but using cycles_t.
> + if (ops->ndo_ll_poll(napi) == LL_FLUSH_FAILED)
> + break; /* premanent failure */
> +
> + local_bh_enable();
> +
> + return !skb_queue_empty(&sk->sk_receive_queue);
> +}
> +
> +static inline void skb_mark_ll(struct napi_struct *napi, struct sk_buff *skb)
> +{
> + skb->dev_ref = napi;
> +}
Slightly odd - I would expect skb to be the first parameter.
[...]
> --- a/net/ipv4/Kconfig
> +++ b/net/ipv4/Kconfig
> @@ -402,6 +402,18 @@ config INET_LRO
>
> If unsure, say Y.
>
> +config INET_LL_RX_POLL
> + bool "Low Latency Receive Poll"
> + default n
> + ---help---
> + Support Low Latency Receive Queue Poll.
> + (For network card drivers which support this option.)
> + When waiting for data in read or poll call directly into the the device driver
> + to flush packets which may be pending on the device queues into the stack.
> +
> +
> + If unsure, say N.
Of course, all distributions will be expected to enable this. So I'm
not sure what the point of the compile-time option is. You might as
well enable it at compile-time but leave the default set to 0.
> config INET_DIAG
> tristate "INET: socket monitoring interface"
> default y
> diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
> index 960fd29..0c060c6 100644
> --- a/net/ipv4/sysctl_net_ipv4.c
> +++ b/net/ipv4/sysctl_net_ipv4.c
> @@ -25,6 +25,7 @@
> #include <net/inet_frag.h>
> #include <net/ping.h>
> #include <net/tcp_memcontrol.h>
> +#include <net/ll_poll.h>
>
> static int zero;
> static int one = 1;
> @@ -326,6 +327,15 @@ static struct ctl_table ipv4_table[] = {
> .mode = 0644,
> .proc_handler = proc_dointvec
> },
> +#ifdef CONFIG_INET_LL_RX_POLL
> + {
> + .procname = "ip_low_latency_poll",
> + .data = &sysctl_net_ll_poll,
> + .maxlen = sizeof(int),
> + .mode = 0644,
> + .proc_handler = proc_dointvec
> + },
> +#endif
This would need to be added to Documentation/networking/ip-sysctl.txt.
Should the units really be cycles or, say, microseconds? I assume that
a sysctl setter can do a conversion to cycles so that there's no need to
multiply every time the value is used. (If the CPU doesn't have
constant_tsc or equivalent then this conversion doesn't quite work, but
then low-latency tunng usually includes disabling frequency scaling.)
Also, this should be a per-device (or even per-NAPI-context?) setting.
> {
> .procname = "tcp_syn_retries",
> .data = &sysctl_tcp_syn_retries,
> diff --git a/net/socket.c b/net/socket.c
> index ee0d029..86da082 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -105,6 +105,12 @@
> #include <linux/sockios.h>
> #include <linux/atalk.h>
>
> +#ifdef CONFIG_INET_LL_RX_POLL
> +#include <net/ll_poll.h>
> +int sysctl_net_ll_poll __read_mostly = 150000;
Nicely tuned for your specific test system, no doubt. :-)
> +EXPORT_SYMBOL_GPL(sysctl_net_ll_poll);
> +#endif
[...]
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_feb
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
^ permalink raw reply
* Re: [PATCH V2] lglock: add read-preference local-global rwlock
From: Oleg Nesterov @ 2013-03-05 16:41 UTC (permalink / raw)
To: Lai Jiangshan
Cc: Michel Lespinasse, Srivatsa S. Bhat, Lai Jiangshan, linux-doc,
peterz, fweisbec, linux-kernel, namhyung, mingo, linux-arch,
linux, xiaoguangrong, wangyun, paulmck, nikunj, linux-pm, rusty,
rostedt, rjw, vincent.guittot, tglx, linux-arm-kernel, netdev,
sbw, tj, akpm, linuxppc-dev
In-Reply-To: <51360ED1.3030104@cn.fujitsu.com>
On 03/05, Lai Jiangshan wrote:
>
> On 03/03/13 01:20, Oleg Nesterov wrote:
> > On 03/02, Lai Jiangshan wrote:
> >>
> >> +void lg_rwlock_local_read_unlock(struct lgrwlock *lgrw)
> >> +{
> >> + switch (__this_cpu_read(*lgrw->reader_refcnt)) {
> >> + case 1:
> >> + __this_cpu_write(*lgrw->reader_refcnt, 0);
> >> + lg_local_unlock(&lgrw->lglock);
> >> + return;
> >> + case FALLBACK_BASE:
> >> + __this_cpu_write(*lgrw->reader_refcnt, 0);
> >> + read_unlock(&lgrw->fallback_rwlock);
> >> + rwlock_release(&lg->lock_dep_map, 1, _RET_IP_);
> >
> > I guess "case 1:" should do rwlock_release() too.
>
> Already do it in "lg_local_unlock(&lgrw->lglock);" before it returns.
> (I like reuse old code)
Yes, I was wrong thanks. Another case when I didn't notice that you
re-use the regular lg_ code...
> > We need rwlock_acquire_read() even in the fast-path, and this acquire_read
> > should be paired with rwlock_acquire() in _write_lock(), but it does
> > spin_acquire(lg->lock_dep_map). Yes, currently this is the same (afaics)
> > but perhaps fallback_rwlock->dep_map would be more clean.
>
> I can't tell which one is better. I try to use fallback_rwlock->dep_map later.
I am not sure which one should be better too, please check.
Again, I forgot that _write_lock/unlock use lg_global_*() code.
Oleg.
^ permalink raw reply
* Re: Fw: [Bug 54861] New: r6040 net-driver on new kernels not work(3.X.X)
From: Florian Fainelli @ 2013-03-05 16:35 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20130305083032.3a43c194@nehalam.linuxnetplumber.net>
On 03/05/2013 05:30 PM, Stephen Hemminger wrote:
>
>
> Begin forwarded message:
Thanks Stephen, I am following up with the reporter on the bugtracker
and will submit fixes.
--
Florian
^ permalink raw reply
* Re: [PATCH] lglock: add read-preference local-global rwlock
From: Oleg Nesterov @ 2013-03-05 16:35 UTC (permalink / raw)
To: Lai Jiangshan
Cc: Lai Jiangshan, linux-doc, peterz, fweisbec, Michel Lespinasse,
mingo, linux-arch, linux, xiaoguangrong, wangyun, paulmck, nikunj,
linux-pm, rusty, rostedt, rjw, namhyung, tglx, linux-arm-kernel,
netdev, linux-kernel, vincent.guittot, sbw, Srivatsa S. Bhat, tj,
akpm, linuxppc-dev
In-Reply-To: <51361540.3060603@cn.fujitsu.com>
On 03/05, Lai Jiangshan wrote:
>
> On 03/03/13 01:06, Oleg Nesterov wrote:
> > On 03/02, Michel Lespinasse wrote:
> >>
> >> My version would be slower if it needs to take the
> >> slow path in a reentrant way, but I'm not sure it matters either :)
> >
> > I'd say, this doesn't matter at all, simply because this can only happen
> > if we race with the active writer.
>
> It can also happen when interrupted. (still very rarely)
>
> arch_spin_trylock()
> ------->interrupted,
> __this_cpu_read() returns 0.
> arch_spin_trylock() fails
> slowpath, any nested will be slowpath too.
> ...
> ..._read_unlock()
> <-------interrupt
> __this_cpu_inc()
> ....
Yes sure. Or it can take the local lock after we already take the global
fallback_lock.
But the same can happen with FALLBACK_BASE, just because we need to take
a lock (local or global) first, then increment the counter.
> (I worries to much. I tend to remove FALLBACK_BASE now, we should
> add it only after we proved we needed it, this part is not proved)
Agreed, great ;)
Oleg.
^ permalink raw reply
* Re: [PATCH] lglock: add read-preference local-global rwlock
From: Michel Lespinasse @ 2013-03-05 16:32 UTC (permalink / raw)
To: Lai Jiangshan
Cc: Oleg Nesterov, Srivatsa S. Bhat, Lai Jiangshan, linux-doc, peterz,
fweisbec, linux-kernel, namhyung, mingo, linux-arch, linux,
xiaoguangrong, wangyun, paulmck, nikunj, linux-pm, rusty, rostedt,
rjw, vincent.guittot, tglx, linux-arm-kernel, netdev, sbw, tj,
akpm, linuxppc-dev
In-Reply-To: <51361540.3060603@cn.fujitsu.com>
On Tue, Mar 5, 2013 at 7:54 AM, Lai Jiangshan <laijs@cn.fujitsu.com> wrote:
> On 03/03/13 01:06, Oleg Nesterov wrote:
>> On 03/02, Michel Lespinasse wrote:
>>>
>>> My version would be slower if it needs to take the
>>> slow path in a reentrant way, but I'm not sure it matters either :)
>>
>> I'd say, this doesn't matter at all, simply because this can only happen
>> if we race with the active writer.
>>
>
> It can also happen when interrupted. (still very rarely)
>
> arch_spin_trylock()
> ------->interrupted,
> __this_cpu_read() returns 0.
> arch_spin_trylock() fails
> slowpath, any nested will be slowpath too.
> ...
> ..._read_unlock()
> <-------interrupt
> __this_cpu_inc()
> ....
Yes (and I think this is actually the most likely way for it to happen).
We do need this to work correctly, but I don't expect we need it to be fast.
(could be wrong, this is only my intuition)
--
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.
^ permalink raw reply
* Fw: [Bug 54861] New: r6040 net-driver on new kernels not work(3.X.X)
From: Stephen Hemminger @ 2013-03-05 16:30 UTC (permalink / raw)
To: Florian Fainelli; +Cc: netdev
Begin forwarded message:
Date: Tue, 5 Mar 2013 08:02:27 -0800
From: "bugzilla-daemon@bugzilla.kernel.org" <bugzilla-daemon@bugzilla.kernel.org>
To: "stephen@networkplumber.org" <stephen@networkplumber.org>
Subject: [Bug 54861] New: r6040 net-driver on new kernels not work(3.X.X)
https://bugzilla.kernel.org/show_bug.cgi?id=54861
Summary: r6040 net-driver on new kernels not work(3.X.X)
Product: Networking
Version: 2.5
Kernel Version: 3.6.8 (3.7.8), possible all new versions
Platform: All
OS/Version: Linux
Tree: Mainline
Status: NEW
Severity: normal
Priority: P1
Component: IPV4
AssignedTo: shemminger@linux-foundation.org
ReportedBy: sunheretic13@gmail.com
Regression: No
Created an attachment (id=94581)
--> (https://bugzilla.kernel.org/attachment.cgi?id=94581)
kernel config files (2.6.37.2 , 3.6.8, 3.7.8)
Hello.
I have a problem.
I have a motherboard with processor RDC8610. As PHY-chip used tlk100 (Texas
Instruments). tlk100 uses "Generic PHY-driver" from kernel.
When I compile a kernel 2.6.37.2 works fine.
When I compile a kernel 3.6.8 or 3.7.8 I have problems in the network.
The built-in network card (r6040) does not work pings.
ifconfig shows no errors, but the "RX bytes"> 0 while the "TX bytes" = 0! It
seems that the transmitter is not initialized or not working.
Rarely get messages on the console:
eth1: link UP
eth1: link DOWN
As I see it, the value "TX bytes" from ifconfig increased (but not
significantly)
Network cable is connected and not damaged.
I can see that from the kernel version 2.6.37.2 to 3.6.8, the NIC driver r6040
upgraded from 0.26 (30May2010) to 0.28 (07Oct2011).
I attach the configuration files of the kernel.
What is it? Bug in the new driver r6040?
Or am I doing wrong?
--
Configure bugmail: https://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
^ permalink raw reply
* Re: [PATCH v6 04/46] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks
From: Lai Jiangshan @ 2013-03-05 16:25 UTC (permalink / raw)
To: Srivatsa S. Bhat
Cc: Lai Jiangshan, Michel Lespinasse, linux-doc, peterz, fweisbec,
linux-kernel, namhyung, mingo, linux-arch, linux, xiaoguangrong,
wangyun, paulmck, nikunj, linux-pm, rusty, rostedt, rjw,
vincent.guittot, tglx, linux-arm-kernel, netdev, oleg, sbw, tj,
akpm, linuxppc-dev
In-Reply-To: <513105DD.8010908@linux.vnet.ibm.com>
On 02/03/13 03:47, Srivatsa S. Bhat wrote:
> On 03/01/2013 11:20 PM, Lai Jiangshan wrote:
>> On 28/02/13 05:19, Srivatsa S. Bhat wrote:
>>> On 02/27/2013 06:03 AM, Lai Jiangshan wrote:
>>>> On Wed, Feb 27, 2013 at 3:30 AM, Srivatsa S. Bhat
>>>> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>>>>> On 02/26/2013 09:55 PM, Lai Jiangshan wrote:
>>>>>> On Tue, Feb 26, 2013 at 10:22 PM, Srivatsa S. Bhat
>>>>>> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>>>>>>>
>>>>>>> Hi Lai,
>>>>>>>
>>>>>>> I'm really not convinced that piggy-backing on lglocks would help
>>>>>>> us in any way. But still, let me try to address some of the points
>>>>>>> you raised...
>>>>>>>
>>>>>>> On 02/26/2013 06:29 PM, Lai Jiangshan wrote:
>>>>>>>> On Tue, Feb 26, 2013 at 5:02 PM, Srivatsa S. Bhat
>>>>>>>> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>>>>>>>>> On 02/26/2013 05:47 AM, Lai Jiangshan wrote:
>>>>>>>>>> On Tue, Feb 26, 2013 at 3:26 AM, Srivatsa S. Bhat
>>>>>>>>>> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>>>>>>>>>>> Hi Lai,
>>>>>>>>>>>
>>>>>>>>>>> On 02/25/2013 09:23 PM, Lai Jiangshan wrote:
>>>>>>>>>>>> Hi, Srivatsa,
>>>>>>>>>>>>
>>>>>>>>>>>> The target of the whole patchset is nice for me.
>>>>>>>>>>>
>>>>>>>>>>> Cool! Thanks :-)
>>>>>>>>>>>
>>>>>>>>> [...]
>>>>>>>>>
>>>>>>>>> Unfortunately, I see quite a few issues with the code above. IIUC, the
>>>>>>>>> writer and the reader both increment the same counters. So how will the
>>>>>>>>> unlock() code in the reader path know when to unlock which of the locks?
>>>>>>>>
>>>>>>>> The same as your code, the reader(which nested in write C.S.) just dec
>>>>>>>> the counters.
>>>>>>>
>>>>>>> And that works fine in my case because the writer and the reader update
>>>>>>> _two_ _different_ counters.
>>>>>>
>>>>>> I can't find any magic in your code, they are the same counter.
>>>>>>
>>>>>> /*
>>>>>> * It is desirable to allow the writer to acquire the percpu-rwlock
>>>>>> * for read (if necessary), without deadlocking or getting complaints
>>>>>> * from lockdep. To achieve that, just increment the reader_refcnt of
>>>>>> * this CPU - that way, any attempt by the writer to acquire the
>>>>>> * percpu-rwlock for read, will get treated as a case of nested percpu
>>>>>> * reader, which is safe, from a locking perspective.
>>>>>> */
>>>>>> this_cpu_inc(pcpu_rwlock->rw_state->reader_refcnt);
>>>>>>
>>>>>
>>>>> Whoa! Hold on, were you really referring to _this_ increment when you said
>>>>> that, in your patch you would increment the refcnt at the writer? Then I guess
>>>>> there is a major disconnect in our conversations. (I had assumed that you were
>>>>> referring to the update of writer_signal, and were just trying to have a single
>>>>> refcnt instead of reader_refcnt and writer_signal).
>>>>
>>>> https://github.com/laijs/linux/commit/53e5053d5b724bea7c538b11743d0f420d98f38d
>>>>
>>>> Sorry the name "fallback_reader_refcnt" misled you.
>>>>
>>> [...]
>>>
>>>>>> All I was considered is "nested reader is seldom", so I always
>>>>>> fallback to rwlock when nested.
>>>>>> If you like, I can add 6 lines of code, the overhead is
>>>>>> 1 spin_try_lock()(fast path) + N __this_cpu_inc()
>>>>>>
>>>>>
>>>>> I'm assuming that calculation is no longer valid, considering that
>>>>> we just discussed how the per-cpu refcnt that you were using is quite
>>>>> unnecessary and can be removed.
>>>>>
>>>>> IIUC, the overhead with your code, as per above discussion would be:
>>>>> 1 spin_try_lock() [non-nested] + N read_lock(global_rwlock).
>>>>
>>>> https://github.com/laijs/linux/commit/46334544bb7961550b7065e015da76f6dab21f16
>>>>
>>>> Again, I'm so sorry the name "fallback_reader_refcnt" misled you.
>>>>
>>>
>>> At this juncture I really have to admit that I don't understand your
>>> intentions at all. What are you really trying to prove? Without giving
>>> a single good reason why my code is inferior, why are you even bringing
>>> up the discussion about a complete rewrite of the synchronization code?
>>> http://article.gmane.org/gmane.linux.kernel.cross-arch/17103
>>> http://article.gmane.org/gmane.linux.power-management.general/31345
>>>
>>> I'm beginning to add 2 + 2 together based on the kinds of questions you
>>> have been asking...
>>>
>>> You posted a patch in this thread and started a discussion around it without
>>> even establishing a strong reason to do so. Now you point me to your git
>>> tree where your patches have even more traces of ideas being borrowed from
>>> my patchset (apart from my own ideas/code, there are traces of others' ideas
>>> being borrowed too - for example, it was Oleg who originally proposed the
>>> idea of splitting up the counter into 2 parts and I'm seeing that it is
>>> slowly crawling into your code with no sign of appropriate credits).
>>> http://article.gmane.org/gmane.linux.network/260288
>>>
>>> And in reply to my mail pointing out the performance implications of the
>>> global read_lock at the reader side in your code, you said you'll come up
>>> with a comparison between that and my patchset.
>>> http://article.gmane.org/gmane.linux.network/260288
>>> The issue has been well-documented in my patch description of patch 4.
>>> http://article.gmane.org/gmane.linux.kernel/1443258
>>>
>>> Are you really trying to pit bits and pieces of my own ideas/versions
>>> against one another and claiming them as your own?
>>>
>>> You projected the work involved in handling the locking issues pertaining
>>> to CPU_DYING notifiers etc as a TODO, despite the fact that I had explicitly
>>> noted in my cover letter that I had audited and taken care of all of them.
>>> http://article.gmane.org/gmane.linux.documentation/9727
>>> http://article.gmane.org/gmane.linux.documentation/9520
>>>
>>> You failed to acknowledge (on purpose?) that I had done a tree-wide
>>> conversion despite the fact that you were replying to the very thread which
>>> had the 46 patches which did exactly that (and I had also mentioned it
>>> explicitly in my cover letter).
>>> http://article.gmane.org/gmane.linux.documentation/9727
>>> http://article.gmane.org/gmane.linux.documentation/9520
>>>
>>> You then started probing more and more about the technique I used to do
>>> the tree-wide conversion.
>>> http://article.gmane.org/gmane.linux.kernel.cross-arch/17111
>>>
>>> You also retorted saying you did go through my patch descriptions, so
>>> its not like you have missed reading them.
>>> http://article.gmane.org/gmane.linux.power-management.general/31345
>>>
>>> Each of these when considered individually, might appear like innocuous and
>>> honest attempts at evaluating my code. But when put together, I'm beginning
>>> to sense a whole different angle to it altogether, as if you are trying
>>> to spin your own patch series, complete with the locking framework _and_
>>> the tree-wide conversion, heavily borrowed from mine. At the beginning of
>>> this discussion, I predicted that the lglock version that you are proposing
>>> would end up being either less efficient than my version or look very similar
>>> to my version. http://article.gmane.org/gmane.linux.kernel/1447139
>>>
>>> I thought it was just the former till now, but its not hard to see how it
>>> is getting closer to becoming the latter too. So yeah, I'm not amused.
>>>
>>> Maybe (and hopefully) you are just trying out different ideas on your own,
>>> and I'm just being paranoid. I really hope that is the case. If you are just
>>> trying to review my code, then please stop sending patches with borrowed ideas
>>> with your sole Signed-off-by, and purposefully ignoring the work already done
>>> in my patchset, because it is really starting to look suspicious, at least
>>> to me.
>>>
>>> Don't get me wrong - I'll whole-heartedly acknowledge and appreciate if
>>> _your_ code is better than mine. I just don't like the idea of somebody
>>> plagiarizing my ideas/code (or even others' ideas for that matter).
>>> However, I sincerely apologize in advance if I misunderstood/misjudged your
>>> intentions; I just wanted to voice my concerns out loud at this point,
>>> considering the bad feeling I got by looking at your responses collectively.
>>>
>>
>> Hi, Srivatsa
>>
>> I'm sorry, big apology to you.
>> I'm bad in communication and I did be wrong.
>> I tended to improve the codes but in false direction.
>>
>
> OK, in that case, I'm extremely sorry too, for jumping on you like that.
> I hope you'll forgive me for the uneasiness it caused.
>
> Now that I understand that you were simply trying to help, I would like to
> express my gratitude for your time, effort and inputs in improving the design
> of the stop-machine replacement.
>
> I'm looking forward to working with you on this as well as future endeavours,
> so I sincerely hope that we can put this unfortunate incident behind us and
> collaborate effectively with renewed mutual trust and good-will.
>
> Thank you very much!
>
Hi, Srivatsa,
I'm sorry again, I delayed your works.
I have some thinkings about the way how to get this work done.
First step: (2~3 patches)
Use preempt_disable() to implement get_online_cpu_atomic(), and add lockdep for it.
Second step:
Conversion patches.
We can send the patchset of the above steps at first.
{
It does not change any behavior of the kernel.
and it is annotation(instead of direct preempt_diable() without comments sometimes),
so I expected they can be merged very early.
}
Third step:
After all people confide the conversion patches covered all cases and cpuhotplug site is ready for it,
we will implement get_online_cpu_atomic() via locks and remove stop_machine() from cpuhotplug.
Any thought?
Thanks,
Lai
If I have time, I will help you for the patches of the first step.
(I was assigned bad job in office-time, I can only do kernel-dev work in night.)
And for step2, I will write a checklist or spatch-script.
^ permalink raw reply
* Re: tipc: MTU discovery
From: Sebastian Pöhn @ 2013-03-05 16:22 UTC (permalink / raw)
To: Erik Hugne; +Cc: netdev, jon.maloy, allan.stephens
In-Reply-To: <20130305141824.GG15384@eerihug-hybrid.ki.sw.ericsson.se>
I run a DLink DGE-528T (PCI-ID: 1186:4300). Kernel is 2.6.32
There is a dedicated VLAN sitting over the physical device.
I even made some similar observations in a VM environment but I think
this is even a more fragile setup.
Will install ethtool ...
On Tue, Mar 5, 2013 at 3:18 PM, Erik Hugne <erik.hugne@ericsson.com> wrote:
> On Tue, Mar 05, 2013 at 01:43:29PM +0100, Sebastian Pöhn wrote:
>> State Messages larger than 1500 which as used for the MTU negotiation
>> do not appear in the TIPC stack. But I am able to seen them entering
>> the correct device.
>> Because of that no reply with the correct max_packet field set can be
>> send and the negotiation will always end up at 1.5k.
>>
>> So my questions are:
>> # Is TIPC meant to detect and use the MTU larger than 1.5k?
> Yes, it should probe and detect MTU's up to ~66k
>
>> # Why are the packets not passed to the TIPC stack?
> TIPC just registers itself as a handler for ETH_P_TIPC through
> dev_add_pack.
> If link mtu probes >1.5k are not passed to TIPC, it sounds to me that the
> NIC driver
> is to blame. What NIC type and kernel version are you running?
>
> Do you see any packet drops in ethtool statistics?
>
> //E
^ permalink raw reply
* Re: [PATCH V2] lglock: add read-preference local-global rwlock
From: Michel Lespinasse @ 2013-03-05 16:19 UTC (permalink / raw)
To: Lai Jiangshan
Cc: Oleg Nesterov, Srivatsa S. Bhat, Lai Jiangshan, linux-doc, peterz,
fweisbec, linux-kernel, namhyung, mingo, linux-arch, linux,
xiaoguangrong, wangyun, paulmck, nikunj, linux-pm, rusty, rostedt,
rjw, vincent.guittot, tglx, linux-arm-kernel, netdev, sbw, tj,
akpm, linuxppc-dev
In-Reply-To: <51360ED1.3030104@cn.fujitsu.com>
Hi Lai,
Just a few comments about your v2 proposal. Hopefully you'll catch
these before you send out v3 :)
- I would prefer reader_refcnt to be unsigned int instead of unsigned long
- I would like some comment to indicate that lgrwlocks don't have
reader-writer fairness and are thus somewhat discouraged
(people could use plain lglock if they don't need reader preference,
though even that use (as brlock) is discouraged already :)
- I don't think FALLBACK_BASE is necessary (you already mentioned you'd
drop it)
- I prefer using the fallback_rwlock's dep_map for lockdep tracking.
I feel this is more natural since we want the lgrwlock to behave as
the rwlock, not as the lglock.
- I prefer to avoid return statements in the middle of functions when
it's easyto do so.
Attached is my current version (based on an earlier version of your code).
You don't have to take it as is but I feel it makes for a more concrete
suggestion :)
Thanks,
----------------------------8<-------------------------------------------
lglock: add read-preference lgrwlock
Current lglock may be used as a fair rwlock; however sometimes a
read-preference rwlock is preferred. One such use case recently came
up for get_cpu_online_atomic().
This change adds a new lgrwlock with the following properties:
- high performance read side, using only cpu-local structures when there
is no write side to contend with;
- correctness guarantees similar to rwlock_t: recursive readers are allowed
and the lock's read side is not ordered vs other locks;
- low performance write side (comparable to lglocks' global side).
The implementation relies on the following principles:
- reader_refcnt is a local lock count; it indicates how many recursive
read locks are taken using the local lglock;
- lglock is used by readers for local locking; it must be acquired
before reader_refcnt becomes nonzero and released after reader_refcnt
goes back to zero;
- fallback_rwlock is used by readers for global locking; it is acquired
when fallback_reader_refcnt is zero and the trylock fails on lglock.
- writers take both the lglock write side and the fallback_rwlock, thus
making sure to exclude both local and global readers.
Thanks to Srivatsa S. Bhat for proposing a lock with these requirements
and Lai Jiangshan for proposing this algorithm as an lglock extension.
Signed-off-by: Michel Lespinasse <walken@google.com>
---
include/linux/lglock.h | 46 +++++++++++++++++++++++++++++++++++++++
kernel/lglock.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 104 insertions(+)
diff --git a/include/linux/lglock.h b/include/linux/lglock.h
index 0d24e932db0b..8b59084935d5 100644
--- a/include/linux/lglock.h
+++ b/include/linux/lglock.h
@@ -67,4 +67,50 @@ void lg_local_unlock_cpu(struct lglock *lg, int cpu);
void lg_global_lock(struct lglock *lg);
void lg_global_unlock(struct lglock *lg);
+/*
+ * lglock may be used as a read write spinlock if desired (though this is
+ * not encouraged as the write side scales badly on high CPU count machines).
+ * It has reader/writer fairness when used that way.
+ *
+ * However, sometimes it is desired to have an unfair rwlock instead, with
+ * reentrant readers that don't need to be ordered vs other locks, comparable
+ * to rwlock_t. lgrwlock implements such semantics.
+ */
+struct lgrwlock {
+ unsigned int __percpu *reader_refcnt;
+ struct lglock lglock;
+ rwlock_t fallback_rwlock;
+};
+
+#define __DEFINE_LGRWLOCK_PERCPU_DATA(name) \
+ static DEFINE_PER_CPU(unsigned int, name ## _refcnt); \
+ static DEFINE_PER_CPU(arch_spinlock_t, name ## _lock) \
+ = __ARCH_SPIN_LOCK_UNLOCKED;
+
+#define __LGRWLOCK_INIT(name) { \
+ .reader_refcnt = &name ## _refcnt, \
+ .lglock = { .lock = &name ## _lock }, \
+ .fallback_rwlock = __RW_LOCK_UNLOCKED(name.fallback_rwlock) \
+}
+
+#define DEFINE_LGRWLOCK(name) \
+ __DEFINE_LGRWLOCK_PERCPU_DATA(name) \
+ struct lgrwlock name = __LGRWLOCK_INIT(name)
+
+#define DEFINE_STATIC_LGRWLOCK(name) \
+ __DEFINE_LGRWLOCK_PERCPU_DATA(name) \
+ static struct lgrwlock name = __LGRWLOCK_INIT(name)
+
+static inline void lg_rwlock_init(struct lgrwlock *lgrw, char *name)
+{
+ lg_lock_init(&lgrw->lglock, name);
+}
+
+void lg_read_lock(struct lgrwlock *lgrw);
+void lg_read_unlock(struct lgrwlock *lgrw);
+void lg_write_lock(struct lgrwlock *lgrw);
+void lg_write_unlock(struct lgrwlock *lgrw);
+void __lg_read_write_lock(struct lgrwlock *lgrw);
+void __lg_read_write_unlock(struct lgrwlock *lgrw);
+
#endif
diff --git a/kernel/lglock.c b/kernel/lglock.c
index 86ae2aebf004..e78a7c95dbfd 100644
--- a/kernel/lglock.c
+++ b/kernel/lglock.c
@@ -87,3 +87,61 @@ void lg_global_unlock(struct lglock *lg)
preempt_enable();
}
EXPORT_SYMBOL(lg_global_unlock);
+
+void lg_read_lock(struct lgrwlock *lgrw)
+{
+ preempt_disable();
+
+ if (__this_cpu_read(*lgrw->reader_refcnt) ||
+ arch_spin_trylock(this_cpu_ptr(lgrw->lglock.lock))) {
+ __this_cpu_inc(*lgrw->reader_refcnt);
+ rwlock_acquire_read(&lgrw->fallback_rwlock.dep_map,
+ 0, 0, _RET_IP_);
+ } else {
+ read_lock(&lgrw->fallback_rwlock);
+ }
+}
+EXPORT_SYMBOL(lg_read_lock);
+
+void lg_read_unlock(struct lgrwlock *lgrw)
+{
+ if (likely(__this_cpu_read(*lgrw->reader_refcnt))) {
+ rwlock_release(&lgrw->fallback_rwlock.dep_map,
+ 1, _RET_IP_);
+ if (!__this_cpu_dec_return(*lgrw->reader_refcnt))
+ arch_spin_unlock(this_cpu_ptr(lgrw->lglock.lock));
+ } else {
+ read_unlock(&lgrw->fallback_rwlock);
+ }
+
+ preempt_enable();
+}
+EXPORT_SYMBOL(lg_read_unlock);
+
+void lg_write_lock(struct lgrwlock *lgrw)
+{
+ lg_global_lock(&lgrw->lglock);
+ write_lock(&lgrw->fallback_rwlock);
+}
+EXPORT_SYMBOL(lg_write_lock);
+
+void lg_write_unlock(struct lgrwlock *lgrw)
+{
+ write_unlock(&lgrw->fallback_rwlock);
+ lg_global_unlock(&lgrw->lglock);
+}
+EXPORT_SYMBOL(lg_write_unlock);
+
+void __lg_read_write_lock(struct lgrwlock *lgrw)
+{
+ lg_write_lock(lgrw);
+ __this_cpu_write(*lgrw->reader_refcnt, 1);
+}
+EXPORT_SYMBOL(__lg_read_write_lock);
+
+void __lg_read_write_unlock(struct lgrwlock *lgrw)
+{
+ __this_cpu_write(*lgrw->reader_refcnt, 0);
+ lg_write_unlock(lgrw);
+}
+EXPORT_SYMBOL(__lg_read_write_unlock);
--
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.
^ permalink raw reply related
* Re: [PATCH 4/7] usbnet: cdc_mbim: don't recover device if suspend fails in system sleep
From: Bjørn Mork @ 2013-03-05 16:08 UTC (permalink / raw)
To: Ming Lei
Cc: David S. Miller, Greg Kroah-Hartman, Jiri Kosina, Alan Stern,
Oliver Neukum, netdev, linux-usb, linux-input
In-Reply-To: <CACVXFVMDNtJOp+8jMOuhCBjrqq-L=jicEJBr_=ErG19Vr+7OGw@mail.gmail.com>
Ming Lei <ming.lei@canonical.com> writes:
> Yes, USB core will flush any outstanding URBs, but the driver still need
> to deal with suspend failure carefully, for example, suppose usb_resume()
> is called in suspend failure path, and the submitted URBs are killed
> by USB core later. But after the device is wakeup, and the resume() will
> do nothing since the suspend count is leaked. So it is what the patches
> are fixing, and it is better to not depend on the default flushing URBs of
> USB core.
I am starting to wonder why the USB core has combined system suspend and
runtime suspend if we are going to end up with every driver testing
PMSG_IS_AUTO(message) and selecting a completely different code path.
You are right that we will end up with problems if usbnet_resume is
called for a device usbnet hasn't suspended. But I'd still claim that
is a bug in the USB core, which is the one that decided to ignore the
suspend error and still call resume.
I guess proper error handling here require the USB core to see the
interface driver as dead if it fails to suspend on system suspend, and
do forced rebinding on resume.
I am not going to fight this any longer. The per-driver
PMSG_IS_AUTO(message) testing is an ugly workround for a core problem,
but they are already all over the place... Still, please make sure the
drivers all return 0 if they are pretending to suspend. No error code
return if the driver ignores the error.
Bjørn
^ permalink raw reply
* [PATCH 4/4] net/irda: Raise dtr in non-blocking open
From: Peter Hurley @ 2013-03-05 16:09 UTC (permalink / raw)
To: David Miller, Samuel Ortiz
Cc: Sasha Levin, Greg Kroah-Hartman, netdev, Jiri Slaby, linux-kernel,
Peter Hurley
In-Reply-To: <1362499747-4262-1-git-send-email-peter@hurleysoftware.com>
DTR/RTS need to be raised, regardless of the open() mode, but not
if the port has already shutdown.
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
net/irda/ircomm/ircomm_tty.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/net/irda/ircomm/ircomm_tty.c b/net/irda/ircomm/ircomm_tty.c
index 522543d..362ba47 100644
--- a/net/irda/ircomm/ircomm_tty.c
+++ b/net/irda/ircomm/ircomm_tty.c
@@ -289,8 +289,15 @@ static int ircomm_tty_block_til_ready(struct ircomm_tty_cb *self,
* If non-blocking mode is set, or the port is not enabled,
* then make the check up front and then exit.
*/
- if (filp->f_flags & O_NONBLOCK || tty->flags & (1 << TTY_IO_ERROR)){
- /* nonblock mode is set or port is not enabled */
+ if (test_bit(TTY_IO_ERROR, &tty->flags)) {
+ port->flags |= ASYNC_NORMAL_ACTIVE;
+ return 0;
+ }
+
+ if (filp->f_flags & O_NONBLOCK) {
+ /* nonblock mode is set */
+ if (tty->termios.c_cflag & CBAUD)
+ tty_port_raise_dtr_rts(port);
port->flags |= ASYNC_NORMAL_ACTIVE;
IRDA_DEBUG(1, "%s(), O_NONBLOCK requested!\n", __func__ );
return 0;
--
1.8.1.2
^ permalink raw reply related
* [PATCH 3/4] net/irda: Use barrier to set task state
From: Peter Hurley @ 2013-03-05 16:09 UTC (permalink / raw)
To: David Miller, Samuel Ortiz
Cc: Sasha Levin, Greg Kroah-Hartman, netdev, Jiri Slaby, linux-kernel,
Peter Hurley
In-Reply-To: <1362499747-4262-1-git-send-email-peter@hurleysoftware.com>
Without a memory and compiler barrier, the task state change
can migrate relative to the condition testing in a blocking loop.
However, the task state change must be visible across all cpus
prior to testing those conditions. Failing to do this can result
in the familiar 'lost wakeup' and this task will hang until killed.
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
net/irda/ircomm/ircomm_tty.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/irda/ircomm/ircomm_tty.c b/net/irda/ircomm/ircomm_tty.c
index d282bbe..522543d 100644
--- a/net/irda/ircomm/ircomm_tty.c
+++ b/net/irda/ircomm/ircomm_tty.c
@@ -324,7 +324,7 @@ static int ircomm_tty_block_til_ready(struct ircomm_tty_cb *self,
if (tty->termios.c_cflag & CBAUD)
tty_port_raise_dtr_rts(port);
- current->state = TASK_INTERRUPTIBLE;
+ set_current_state(TASK_INTERRUPTIBLE);
if (tty_hung_up_p(filp) ||
!test_bit(ASYNCB_INITIALIZED, &port->flags)) {
--
1.8.1.2
^ permalink raw reply related
* [PATCH 2/4] net/irda: Hold port lock while bumping blocked_open
From: Peter Hurley @ 2013-03-05 16:09 UTC (permalink / raw)
To: David Miller, Samuel Ortiz
Cc: Sasha Levin, Greg Kroah-Hartman, netdev, Jiri Slaby, linux-kernel,
Peter Hurley
In-Reply-To: <1362499747-4262-1-git-send-email-peter@hurleysoftware.com>
Although tty_lock() already protects concurrent update to
blocked_open, that fails to meet the separation-of-concerns between
tty_port and tty.
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
net/irda/ircomm/ircomm_tty.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/irda/ircomm/ircomm_tty.c b/net/irda/ircomm/ircomm_tty.c
index 1721dc7..d282bbe 100644
--- a/net/irda/ircomm/ircomm_tty.c
+++ b/net/irda/ircomm/ircomm_tty.c
@@ -317,8 +317,8 @@ static int ircomm_tty_block_til_ready(struct ircomm_tty_cb *self,
spin_lock_irqsave(&port->lock, flags);
if (!tty_hung_up_p(filp))
port->count--;
- spin_unlock_irqrestore(&port->lock, flags);
port->blocked_open++;
+ spin_unlock_irqrestore(&port->lock, flags);
while (1) {
if (tty->termios.c_cflag & CBAUD)
@@ -362,8 +362,8 @@ static int ircomm_tty_block_til_ready(struct ircomm_tty_cb *self,
spin_lock_irqsave(&port->lock, flags);
if (!tty_hung_up_p(filp))
port->count++;
- spin_unlock_irqrestore(&port->lock, flags);
port->blocked_open--;
+ spin_unlock_irqrestore(&port->lock, flags);
IRDA_DEBUG(1, "%s(%d):block_til_ready after blocking on %s open_count=%d\n",
__FILE__, __LINE__, tty->driver->name, port->count);
--
1.8.1.2
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox