* Re: [PATCH 0/2] fix kernel crash with macvtap on top of LRO
From: David Miller @ 2013-02-11 1:15 UTC (permalink / raw)
To: bhutchings
Cc: mst, eric.dumazet, netdev, eilong, jeffrey.t.kirsher,
jesse.brandeburg, bruce.w.allan, carolyn.wyborny,
donald.c.skidmore, gregory.v.rose, peter.p.waskiewicz.jr,
alexander.h.duyck, john.ronciak, tushar.n.dave, jitendra.kalsaria,
sony.chacko, linux-driver, john.r.fastabend, jacob.e.keller,
linux-kernel, e1000-devel
In-Reply-To: <1360276295.3605.44.camel@bwh-desktop.uk.solarflarecom.com>
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Thu, 7 Feb 2013 22:31:35 +0000
> On Thu, 2013-02-07 at 23:33 +0200, Michael S. Tsirkin wrote:
>> We might want to add code to forward LRO status from macvlan
>> (not macvtap) back to the lowerdev, so that setting up forwarding
>> from macvlan disables LRO on the lowerdev, but that seems like another
>> issue.
>
> I think it's the same issue!
I do too, can someome please work to resolve this properly?
Thanks.
^ permalink raw reply
* Re: [PATCHv2-net-3.8 0/3] fix kernel crash with macvtap on top of LRO
From: David Miller @ 2013-02-11 1:15 UTC (permalink / raw)
To: mst
Cc: netdev, eilong, jeffrey.t.kirsher, jesse.brandeburg,
bruce.w.allan, carolyn.wyborny, donald.c.skidmore, gregory.v.rose,
peter.p.waskiewicz.jr, alexander.h.duyck, john.ronciak,
tushar.n.dave, jitendra.kalsaria, sony.chacko, linux-driver,
john.r.fastabend, jacob.e.keller, linux-kernel, e1000-devel,
bhutchings, eric.dumazet
In-Reply-To: <cover.1360236441.git.mst@redhat.com>
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Thu, 7 Feb 2013 15:12:56 +0200
> At the moment, macvtap crashes are observed if macvtap is attached
> to an interface with LRO enabled.
> The crash in question is BUG() in macvtap_skb_to_vnet_hdr.
> This happens because several drivers set gso_size but not gso_type
> in incoming skbs.
> This didn't use to be the case: with intel cards on 3.2 and older
> kernels, with qlogic - on 3.4 and older kernels, so it's a regression if
> not a recent one.
> The following patches fix this for qlogic, broadcom and intel drivers.
>
> I tested that the patch fixes the crash for ixgbe but
> don't have qlogic/broadcom hardware to test.
> I also only tested TCPv4.
>
> Please review, and consider for 3.8.
>
> Changes from v1:
> - added missing htons as suggested by Eric
> - backported the relevant bits from
> cbf1de72324a8105ddcc3d9ce9acbc613faea17e for bnx2x
Applied to 'net', thanks.
^ permalink raw reply
* Re: [PATCH (net.git) 2/2] stmmac: mdio register has to fail if the phy is not found
From: David Miller @ 2013-02-11 1:12 UTC (permalink / raw)
To: peppe.cavallaro; +Cc: netdev
In-Reply-To: <1360219672-27980-2-git-send-email-peppe.cavallaro@st.com>
From: Giuseppe CAVALLARO <peppe.cavallaro@st.com>
Date: Thu, 7 Feb 2013 07:47:52 +0100
> With this patch the stmmac fails in case of the phy device
> is not found; w/o this fix the mdio can be register twice when
> do down/up the iface and this is not correct.
>
> Reported-by: Stas <stsp@list.ru>
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Applied.
^ permalink raw reply
* Re: [PATCH (net.git) 1/2] stmmac: fix macro used for debugging the xmit
From: David Miller @ 2013-02-11 1:12 UTC (permalink / raw)
To: peppe.cavallaro; +Cc: netdev
In-Reply-To: <1360219672-27980-1-git-send-email-peppe.cavallaro@st.com>
From: Giuseppe CAVALLARO <peppe.cavallaro@st.com>
Date: Thu, 7 Feb 2013 07:47:51 +0100
> This patch fixes the name of the macro used for
> debugging the transmit process. I used STMMAC_TX_DEBUG
> instead of STMMAC_XMIT_DEBUG.
>
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Applied.
^ permalink raw reply
* Re: [PATCH 0/1] VM Sockets for Linux upstreaming
From: David Miller @ 2013-02-11 1:10 UTC (permalink / raw)
To: acking; +Cc: pv-drivers, netdev, linux-kernel, virtualization, gregkh
In-Reply-To: <1360196636-9357-1-git-send-email-acking@vmware.com>
From: Andy King <acking@vmware.com>
Date: Wed, 6 Feb 2013 16:23:55 -0800
> In an effort to improve the out-of-the-box experience with Linux kernels for
> VMware users, VMware is working on readying the VM Sockets (VSOCK, formerly
> VMCI Sockets) (vsock) kernel module for inclusion in the Linux kernel. The
> purpose of this post is to acquire feedback on the vsock kernel module.
>
> Unlike previous submissions, where the new socket family was entirely reliant
> on VMware's VMCI PCI device (and thus VMware's hypervisor), VM Sockets is now
> completely[1] separated out into two parts, each in its own module:
>
> o Core socket code, which is transport-neutral and invokes transport
> callbacks to communicate with the hypervisor. This is vsock.ko.
> o A VMCI transport, which communicates over VMCI with the VMware hypervisor.
> This is vmw_vsock_vmci_transport.ko, and it registers with the core module
> as a transport.
>
> This should provide a path to introducing additional transports, for example
> virtio, with the ultimate goal being to make this new socket family
> hypervisor-neutral.
Applied, thanks.
^ permalink raw reply
* Re: [RFC PATCH] tg3: Convert chip type macros to inline functions
From: David Miller @ 2013-02-11 0:39 UTC (permalink / raw)
To: joe; +Cc: netdev, hauke, mcarlson, mchan, nsujir
In-Reply-To: <1360194298.12464.35.camel@joe-AO722>
From: Joe Perches <joe@perches.com>
Date: Wed, 06 Feb 2013 15:44:58 -0800
> To me the negative to these conversions is at
> least for gcc 4.7.2, the overall code size
> increases
>
> $ size drivers/net/ethernet/broadcom/tg3.o*
> text data bss dec hex filename
> 203426 13446 55744 272616 428e8 drivers/net/ethernet/broadcom/tg3.o.new
> 202135 13446 55144 270725 42185 drivers/net/ethernet/broadcom/tg3.o.old
>
> I'm not sure why gcc doesn't do the optimization
> and code generation the same way.
Out of curiosity I looked at the assembler difference on sparc
and one thing stood out.
You're changing how the values are evaluated, specifically you
now are forcing the compiler to work with unsigned ID numbers
whereas before it was working with signed values.
So, looking at one example, the ASIC revision test in __tg3_set_mac_addr():
if (GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5703 ||
GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5704) {
...
}
The compiler previously turned the two tests into one:
asic_id -= 1;
if (asic_id <= 1) {
...
}
But this construct isn't valid for unsigned quantities, so now with
your patch this expands to two tests:
if (asic_id == 1 ||
asic_id == 2) {
...
}
The assembler is hard to compare manually, because the inlining
changes how hard registers are allocated, and issues like the above
will change all of the branch and file offsets as well. :-/
^ permalink raw reply
* Re: [GIT] Networking
From: David Miller @ 2013-02-11 0:07 UTC (permalink / raw)
To: romieu; +Cc: torvalds, akpm, netdev, linux-kernel, hayeswang, jrg.otte
In-Reply-To: <20130208225026.GA24672@electric-eye.fr.zoreil.com>
From: Francois Romieu <romieu@fr.zoreil.com>
Date: Fri, 8 Feb 2013 23:50:26 +0100
> Linus Torvalds <torvalds@linux-foundation.org> :
> [...]
>> However, there's still the r8169 regressions (see the emails with the
>> subject "regression: NETDEV WATCHDOG: eth0 (r8169): transmit queue 0
>> timed out"). It's bisected, and a revert is reported to fix things.
>> It's not in this pull request. Comments ?
>
> Jörg asked Hayes. Hayes suggested to revert. Francois reverts, fixes a
> pair of rejects, builds, boot. Anyone should be able to pull from
> branch 'davem.r8169' in repository
>
> git://violet.fr.zoreil.com/romieu/linux davem.r8169
Now that Jörg has tested these reverts successfully, I've pulled
them into my tree, thanks everyone.
^ permalink raw reply
* Re: [PATCH v2 net-next] skbuff: create skb_panic() to let caller specify panic
From: Jean Sacren @ 2013-02-10 23:46 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev
In-Reply-To: <20130210182451.GA1602@minipsycho.orion>
From: Jiri Pirko <jiri@resnulli.us>
Date: Sun, 10 Feb 2013 19:24:51 +0100
>
> How about to leave skb_under_panic() and skb_over_panic() wrapping
> functions there just to wrap "skb_panic(skb, sz, addr, __func__);" call?
>
> Seems nice that to state the name in str...
That's really a good idea. If David drops again, I'll implement it.
Thanks a lot,
--
Jean Sacren
^ permalink raw reply
* Re: [PATCH 1/2] net: calexdaxgmac: fix printing of hardware version
From: Rob Herring @ 2013-02-10 22:34 UTC (permalink / raw)
To: Ben Dooks; +Cc: netdev, David S. Miller, linux-arm-kernel
In-Reply-To: <1360510721-17860-2-git-send-email-ben.dooks@codethink.co.uk>
On 02/10/2013 09:38 AM, Ben Dooks wrote:
> The current driver attempts to print netdev_info() before registering the
> network device and allowing the name to be set. Change this print to be
> after the network deviec has been registered, and thus has been allocated
You missed the typo pointed out on the first version sent to lakml.
> a network device name.
>
> Fix the following issue:
>
> calxedaxgmac fff50000.ethernet (unregistered net_device): h/w version is 0x1012
>
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
Otherwise,
Acked-by: Rob Herring <rob.herring@calxeda.com>
> ---
> drivers/net/ethernet/calxeda/xgmac.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/calxeda/xgmac.c b/drivers/net/ethernet/calxeda/xgmac.c
> index f7f0290..f91d9b2 100644
> --- a/drivers/net/ethernet/calxeda/xgmac.c
> +++ b/drivers/net/ethernet/calxeda/xgmac.c
> @@ -1715,9 +1715,6 @@ static int xgmac_probe(struct platform_device *pdev)
> goto err_io;
> }
>
> - uid = readl(priv->base + XGMAC_VERSION);
> - netdev_info(ndev, "h/w version is 0x%x\n", uid);
> -
> writel(0, priv->base + XGMAC_DMA_INTR_ENA);
> ndev->irq = platform_get_irq(pdev, 0);
> if (ndev->irq == -ENXIO) {
> @@ -1771,6 +1768,9 @@ static int xgmac_probe(struct platform_device *pdev)
> if (ret)
> goto err_reg;
>
> + uid = readl(priv->base + XGMAC_VERSION);
> + netdev_info(ndev, "h/w version is 0x%x\n", uid);
> +
> return 0;
>
> err_reg:
>
^ permalink raw reply
* Re: [PATCH 2/2] net: calexdaxgmac: fixup endian issues after __raw IO function change
From: Rob Herring @ 2013-02-10 22:32 UTC (permalink / raw)
To: Ben Dooks; +Cc: netdev, linux-arm-kernel, David S. Miller
In-Reply-To: <1360510721-17860-3-git-send-email-ben.dooks@codethink.co.uk>
On 02/10/2013 09:38 AM, Ben Dooks wrote:
> When changing to __raw acccessors in 0ec6d343f7bcf9e0944aa9ff65287b987ec00c0f
> ("net: calxedaxgmac: use raw i/o accessors in rx and tx paths"), the driver
> is now broken on big endian systems as the readl/writel have an implict
> endian swap in them.
>
> Change all the places where the __raw calls are used to correctly convert
> the constants in big endian format to the little endian data that the
> peripheral expects to see.
This is a bit ugly. The correct fix is really to enable the _relaxed
accessors on more arches.
Perhaps a local definition of readl_relaxed and writel_relaxed would be
a cleaner temporary solution.
Rob
>
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
> drivers/net/ethernet/calxeda/xgmac.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/calxeda/xgmac.c b/drivers/net/ethernet/calxeda/xgmac.c
> index f91d9b2..96fd538 100644
> --- a/drivers/net/ethernet/calxeda/xgmac.c
> +++ b/drivers/net/ethernet/calxeda/xgmac.c
> @@ -1202,7 +1202,8 @@ static int xgmac_poll(struct napi_struct *napi, int budget)
>
> if (work_done < budget) {
> napi_complete(napi);
> - __raw_writel(DMA_INTR_DEFAULT_MASK, priv->base + XGMAC_DMA_INTR_ENA);
> + __raw_writel(le32_to_cpu((__force __le32)DMA_INTR_DEFAULT_MASK),
> + priv->base + XGMAC_DMA_INTR_ENA);
> }
> return work_done;
> }
> @@ -1348,7 +1349,7 @@ static irqreturn_t xgmac_pmt_interrupt(int irq, void *dev_id)
> void __iomem *ioaddr = priv->base;
>
> intr_status = __raw_readl(ioaddr + XGMAC_INT_STAT);
> - if (intr_status & XGMAC_INT_STAT_PMT) {
> + if (intr_status & le32_to_cpu((__force __le32)XGMAC_INT_STAT_PMT)) {
> netdev_dbg(priv->dev, "received Magic frame\n");
> /* clear the PMT bits 5 and 6 by reading the PMT */
> readl(ioaddr + XGMAC_PMT);
> @@ -1369,6 +1370,8 @@ static irqreturn_t xgmac_interrupt(int irq, void *dev_id)
> intr_status &= __raw_readl(priv->base + XGMAC_DMA_INTR_ENA);
> __raw_writel(intr_status, priv->base + XGMAC_DMA_STATUS);
>
> + intr_status = (__force u32)cpu_to_le32(intr_status);
> +
> /* It displays the DMA process states (CSR5 register) */
> /* ABNORMAL interrupts */
> if (unlikely(intr_status & DMA_STATUS_AIS)) {
> @@ -1403,7 +1406,8 @@ static irqreturn_t xgmac_interrupt(int irq, void *dev_id)
>
> /* TX/RX NORMAL interrupts */
> if (intr_status & (DMA_STATUS_RI | DMA_STATUS_TU | DMA_STATUS_TI)) {
> - __raw_writel(DMA_INTR_ABNORMAL, priv->base + XGMAC_DMA_INTR_ENA);
> + __raw_writel(le32_to_cpu((__force __le32)DMA_INTR_ABNORMAL),
> + priv->base + XGMAC_DMA_INTR_ENA);
> napi_schedule(&priv->napi);
> }
>
>
^ permalink raw reply
* Re: [PATCH v5 04/45] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks
From: Paul E. McKenney @ 2013-02-10 22:13 UTC (permalink / raw)
To: Srivatsa S. Bhat
Cc: Oleg Nesterov, tglx, peterz, tj, rusty, mingo, akpm, namhyung,
rostedt, wangyun, xiaoguangrong, rjw, sbw, fweisbec, linux,
nikunj, linux-pm, linux-arch, linux-arm-kernel, linuxppc-dev,
netdev, linux-doc, linux-kernel
In-Reply-To: <5117FE74.4020000@linux.vnet.ibm.com>
On Mon, Feb 11, 2013 at 01:39:24AM +0530, Srivatsa S. Bhat wrote:
> On 02/11/2013 01:20 AM, Oleg Nesterov wrote:
> > On 02/11, Srivatsa S. Bhat wrote:
> >>
> >> On 02/10/2013 11:36 PM, Oleg Nesterov wrote:
> >>>>> +static void announce_writer_inactive(struct percpu_rwlock *pcpu_rwlock)
> >>>>> +{
> >>>>> + unsigned int cpu;
> >>>>> +
> >>>>> + drop_writer_signal(pcpu_rwlock, smp_processor_id());
> >>>>
> >>>> Why do we drop ourselves twice? More to the point, why is it important to
> >>>> drop ourselves first?
> >>>
> >>> And don't we need mb() _before_ we clear ->writer_signal ?
> >>>
> >>
> >> Oh, right! Or, how about moving announce_writer_inactive() to _after_
> >> write_unlock()?
> >
> > Not sure this will help... but, either way it seems we have another
> > problem...
> >
> > percpu_rwlock tries to be "generic". This means we should "ignore" its
> > usage in hotplug, and _write_lock should not race with _write_unlock.
> >
>
> Yes, good point!
>
> > IOW. Suppose that _write_unlock clears ->writer_signal. We need to ensure
> > that this can't race with another write which wants to set this flag.
> > Perhaps it should be counter as well, and it should be protected by
> > the same ->global_rwlock, but _write_lock() should drop it before
> > sync_all_readers() and then take it again?
>
> Hmm, or we could just add an extra mb() like you suggested, and keep it
> simple...
>
> >
> >>>>> +static inline void sync_reader(struct percpu_rwlock *pcpu_rwlock,
> >>>>> + unsigned int cpu)
> >>>>> +{
> >>>>> + smp_rmb(); /* Paired with smp_[w]mb() in percpu_read_[un]lock() */
> >>>>
> >>>> As I understand it, the purpose of this memory barrier is to ensure
> >>>> that the stores in drop_writer_signal() happen before the reads from
> >>>> ->reader_refcnt in reader_uses_percpu_refcnt(), thus preventing the
> >>>> race between a new reader attempting to use the fastpath and this writer
> >>>> acquiring the lock. Unless I am confused, this must be smp_mb() rather
> >>>> than smp_rmb().
> >>>
> >>> And note that before sync_reader() we call announce_writer_active() which
> >>> already adds mb() before sync_all_readers/sync_reader, so this rmb() looks
> >>> unneeded.
> >>>
> >>
> >> My intention was to help the writer see the ->reader_refcnt drop to zero
> >> ASAP; hence I used smp_wmb() at reader and smp_rmb() here at the writer.
> >
> > Hmm, interesting... Not sure, but can't really comment. However I can
> > answer your next question:
>
> Paul told me in another mail that I was expecting too much out of memory
> barriers, like increasing the speed of electrons and what not ;-)
> [ It would have been cool though, if it had such magical powers :P ]
"But because you have used the special mb_tachyonic instruction, the
speed of light is 600,000 km/s for the next five clock cycles"... ;-)
Thanx, Paul
> >> Please correct me if my understanding of memory barriers is wrong here..
> >
> > Who? Me??? No we have paulmck for that ;)
> >
>
> Haha ;-)
>
> Regards,
> Srivatsa S. Bhat
>
^ permalink raw reply
* [patch net-next] bridge: use dev->addr_assign_type to see if user change mac
From: Jiri Pirko @ 2013-02-10 21:41 UTC (permalink / raw)
To: netdev; +Cc: davem, stephen
And remove no longer used br->flags.
Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
net/bridge/br_device.c | 1 -
net/bridge/br_private.h | 3 ---
net/bridge/br_stp_if.c | 2 +-
3 files changed, 1 insertion(+), 5 deletions(-)
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index e1bc090..ba6fb2d 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -176,7 +176,6 @@ static int br_set_mac_address(struct net_device *dev, void *p)
br_fdb_change_mac_address(br, addr->sa_data);
br_stp_change_bridge_id(br, addr->sa_data);
}
- br->flags |= BR_SET_MAC_ADDR;
spin_unlock_bh(&br->lock);
return 0;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 711094a..06e85d9 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -197,9 +197,6 @@ struct net_bridge
bool nf_call_ip6tables;
bool nf_call_arptables;
#endif
- unsigned long flags;
-#define BR_SET_MAC_ADDR 0x00000001
-
u16 group_fwd_mask;
/* STP */
diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
index 7b5197c..0bdb4eb 100644
--- a/net/bridge/br_stp_if.c
+++ b/net/bridge/br_stp_if.c
@@ -216,7 +216,7 @@ bool br_stp_recalculate_bridge_id(struct net_bridge *br)
struct net_bridge_port *p;
/* user has chosen a value so keep it */
- if (br->flags & BR_SET_MAC_ADDR)
+ if (br->dev->addr_assign_type == NET_ADDR_SET)
return false;
list_for_each_entry(p, &br->port_list, list) {
--
1.8.1.2
^ permalink raw reply related
* Re: [PATCH 5/8] wan: Remove unnecessary alloc/OOM messages
From: Krzysztof Halasa @ 2013-02-10 20:30 UTC (permalink / raw)
To: Joe Perches
Cc: netdev, Jan "Yenya" Kasprzak, Kevin Curtis, linux-kernel
In-Reply-To: <392e9b2c652a41f6c5791268dde1eb8c71d0be90.1359939013.git.joe@perches.com>
Joe Perches <joe@perches.com> writes:
> alloc failures already get standardized OOM
> messages and a dump_stack.
>
> Hoist assigns from if tests.
>
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
> drivers/net/wan/cosa.c | 9 ++++-----
> drivers/net/wan/farsync.c | 6 ++----
> drivers/net/wan/hdlc.c | 9 ++++-----
> drivers/net/wan/x25_asy.c | 1 -
> 4 files changed, 10 insertions(+), 15 deletions(-)
Good to see.
Acked-by: Krzysztof Halasa <khc@pm.waw.pl>
--
Krzysztof Halasa
^ permalink raw reply
* Re: [PATCH v5 04/45] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks
From: Srivatsa S. Bhat @ 2013-02-10 20:20 UTC (permalink / raw)
To: Oleg Nesterov
Cc: linux-doc, peterz, fweisbec, mingo, linux-arch, linux,
xiaoguangrong, wangyun, paulmck, nikunj, linux-pm, rusty, rostedt,
rjw, namhyung, tglx, linux-arm-kernel, netdev, linux-kernel, sbw,
tj, akpm, linuxppc-dev
In-Reply-To: <20130210201312.GB6236@redhat.com>
On 02/11/2013 01:43 AM, Oleg Nesterov wrote:
> On 02/11, Srivatsa S. Bhat wrote:
>>
>> On 02/09/2013 04:40 AM, Paul E. McKenney wrote:
>>>> +static void announce_writer_inactive(struct percpu_rwlock *pcpu_rwlock)
>>>> +{
>>>> + unsigned int cpu;
>>>> +
>>>> + drop_writer_signal(pcpu_rwlock, smp_processor_id());
>>>
>>> Why do we drop ourselves twice? More to the point, why is it important to
>>> drop ourselves first?
>>>
>>
>> I don't see where we are dropping ourselves twice. Note that we are no longer
>> in the cpu_online_mask, so the 'for' loop below won't include us. So we need
>> to manually drop ourselves. It doesn't matter whether we drop ourselves first
>> or later.
>
> Yes, but this just reflects its usage in cpu-hotplug. cpu goes away under
> _write_lock.
>
Ah, right. I guess the code still has remnants from the older version in which
this locking scheme wasn't generic and was tied to cpu-hotplug alone..
> Perhaps _write_lock/unlock shoud use for_each_possible_cpu() instead?
>
Hmm, that wouldn't be too bad.
> Hmm... I think this makes sense anyway. Otherwise, in theory,
> percpu_write_lock(random_non_hotplug_lock) can race with cpu_up?
>
Yeah, makes sense. Will change it to for_each_possible_cpu().
And I had previously fixed such races with lglocks with a complicated scheme (to
avoid the costly for_each_possible loop), which was finally rewritten to use
for_each_possible_cpu() for the sake of simplicity..
Regards,
Srivatsa S. Bhat
^ permalink raw reply
* Re: [PATCH v5 04/45] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks
From: Oleg Nesterov @ 2013-02-10 20:13 UTC (permalink / raw)
To: Srivatsa S. Bhat
Cc: paulmck, tglx, peterz, tj, rusty, mingo, akpm, namhyung, rostedt,
wangyun, xiaoguangrong, rjw, sbw, fweisbec, linux, nikunj,
linux-pm, linux-arch, linux-arm-kernel, linuxppc-dev, netdev,
linux-doc, linux-kernel
In-Reply-To: <5117F0C0.2030605@linux.vnet.ibm.com>
On 02/11, Srivatsa S. Bhat wrote:
>
> On 02/09/2013 04:40 AM, Paul E. McKenney wrote:
> >> +static void announce_writer_inactive(struct percpu_rwlock *pcpu_rwlock)
> >> +{
> >> + unsigned int cpu;
> >> +
> >> + drop_writer_signal(pcpu_rwlock, smp_processor_id());
> >
> > Why do we drop ourselves twice? More to the point, why is it important to
> > drop ourselves first?
> >
>
> I don't see where we are dropping ourselves twice. Note that we are no longer
> in the cpu_online_mask, so the 'for' loop below won't include us. So we need
> to manually drop ourselves. It doesn't matter whether we drop ourselves first
> or later.
Yes, but this just reflects its usage in cpu-hotplug. cpu goes away under
_write_lock.
Perhaps _write_lock/unlock shoud use for_each_possible_cpu() instead?
Hmm... I think this makes sense anyway. Otherwise, in theory,
percpu_write_lock(random_non_hotplug_lock) can race with cpu_up?
Oleg.
^ permalink raw reply
* Re: [PATCH v5 04/45] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks
From: Srivatsa S. Bhat @ 2013-02-10 20:09 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Paul E. McKenney, tglx, peterz, tj, rusty, mingo, akpm, namhyung,
rostedt, wangyun, xiaoguangrong, rjw, sbw, fweisbec, linux,
nikunj, linux-pm, linux-arch, linux-arm-kernel, linuxppc-dev,
netdev, linux-doc, linux-kernel
In-Reply-To: <20130210195042.GA6236@redhat.com>
On 02/11/2013 01:20 AM, Oleg Nesterov wrote:
> On 02/11, Srivatsa S. Bhat wrote:
>>
>> On 02/10/2013 11:36 PM, Oleg Nesterov wrote:
>>>>> +static void announce_writer_inactive(struct percpu_rwlock *pcpu_rwlock)
>>>>> +{
>>>>> + unsigned int cpu;
>>>>> +
>>>>> + drop_writer_signal(pcpu_rwlock, smp_processor_id());
>>>>
>>>> Why do we drop ourselves twice? More to the point, why is it important to
>>>> drop ourselves first?
>>>
>>> And don't we need mb() _before_ we clear ->writer_signal ?
>>>
>>
>> Oh, right! Or, how about moving announce_writer_inactive() to _after_
>> write_unlock()?
>
> Not sure this will help... but, either way it seems we have another
> problem...
>
> percpu_rwlock tries to be "generic". This means we should "ignore" its
> usage in hotplug, and _write_lock should not race with _write_unlock.
>
Yes, good point!
> IOW. Suppose that _write_unlock clears ->writer_signal. We need to ensure
> that this can't race with another write which wants to set this flag.
> Perhaps it should be counter as well, and it should be protected by
> the same ->global_rwlock, but _write_lock() should drop it before
> sync_all_readers() and then take it again?
Hmm, or we could just add an extra mb() like you suggested, and keep it
simple...
>
>>>>> +static inline void sync_reader(struct percpu_rwlock *pcpu_rwlock,
>>>>> + unsigned int cpu)
>>>>> +{
>>>>> + smp_rmb(); /* Paired with smp_[w]mb() in percpu_read_[un]lock() */
>>>>
>>>> As I understand it, the purpose of this memory barrier is to ensure
>>>> that the stores in drop_writer_signal() happen before the reads from
>>>> ->reader_refcnt in reader_uses_percpu_refcnt(), thus preventing the
>>>> race between a new reader attempting to use the fastpath and this writer
>>>> acquiring the lock. Unless I am confused, this must be smp_mb() rather
>>>> than smp_rmb().
>>>
>>> And note that before sync_reader() we call announce_writer_active() which
>>> already adds mb() before sync_all_readers/sync_reader, so this rmb() looks
>>> unneeded.
>>>
>>
>> My intention was to help the writer see the ->reader_refcnt drop to zero
>> ASAP; hence I used smp_wmb() at reader and smp_rmb() here at the writer.
>
> Hmm, interesting... Not sure, but can't really comment. However I can
> answer your next question:
>
Paul told me in another mail that I was expecting too much out of memory
barriers, like increasing the speed of electrons and what not ;-)
[ It would have been cool though, if it had such magical powers :P ]
>> Please correct me if my understanding of memory barriers is wrong here..
>
> Who? Me??? No we have paulmck for that ;)
>
Haha ;-)
Regards,
Srivatsa S. Bhat
^ permalink raw reply
* Re: [PATCH v5 09/45] smp, cpu hotplug: Fix smp_call_function_*() to prevent CPU offline properly
From: Srivatsa S. Bhat @ 2013-02-10 19:59 UTC (permalink / raw)
To: paulmck
Cc: tglx, peterz, tj, oleg, rusty, mingo, akpm, namhyung, rostedt,
wangyun, xiaoguangrong, rjw, sbw, fweisbec, linux, nikunj,
linux-pm, linux-arch, linux-arm-kernel, linuxppc-dev, netdev,
linux-doc, linux-kernel
In-Reply-To: <20130210195639.GL2666@linux.vnet.ibm.com>
On 02/11/2013 01:26 AM, Paul E. McKenney wrote:
> On Mon, Feb 11, 2013 at 01:11:29AM +0530, Srivatsa S. Bhat wrote:
>> On 02/09/2013 05:37 AM, Paul E. McKenney wrote:
>>> On Tue, Jan 22, 2013 at 01:05:10PM +0530, Srivatsa S. Bhat wrote:
>>>> Once stop_machine() is gone from the CPU offline path, we won't be able to
>>>> depend on preempt_disable() to prevent CPUs from going offline from under us.
>>>>
>>>> Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
>>>> while invoking from atomic context.
>>>>
>>>> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>>>
>>> Would it make sense for get_online_cpus_atomic() to return the current
>>> CPU number?
>>
>> Hmm, I'm not so sure. I tried to model it after get_online_cpus(), which doesn't
>> return anything (for other reasons, of course..)
>>
>> Moreover, a function name like *_cpu_* returning the CPU number would be intuitive.
>> But a name such as *_cpus_* (plural) returning a CPU number might appear confusing..
>>
>> And also I don't think it'll make a huge improvement in the callers.. (We might
>> be better off avoiding an smp_processor_id() if possible, since this function could
>> be called in very hot paths).. So I don't see a strong case for returning the
>> CPU number. But let me know if you think it'll still be worth it for some reason...
>
> I just noted a lot of two-line code sequences in your patch that would be
> one line if the CPU number was returned.
Ah, in that case, I'll reconsider your suggestion while working on the next version.
Thanks!
Regards,
Srivatsa S. Bhat
> But I don't feel strongly about
> it, so if people are OK with the current version, no problem.
>
> Thanx, Paul
>
^ permalink raw reply
* Re: [PATCH v5 04/45] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks
From: Srivatsa S. Bhat @ 2013-02-10 19:57 UTC (permalink / raw)
To: paulmck
Cc: tglx, peterz, tj, oleg, rusty, mingo, akpm, namhyung, rostedt,
wangyun, xiaoguangrong, rjw, sbw, fweisbec, linux, nikunj,
linux-pm, linux-arch, linux-arm-kernel, linuxppc-dev, netdev,
linux-doc, linux-kernel
In-Reply-To: <20130210194759.GJ2666@linux.vnet.ibm.com>
On 02/11/2013 01:17 AM, Paul E. McKenney wrote:
> On Mon, Feb 11, 2013 at 12:40:56AM +0530, Srivatsa S. Bhat wrote:
>> On 02/09/2013 04:40 AM, Paul E. McKenney wrote:
>>> On Tue, Jan 22, 2013 at 01:03:53PM +0530, Srivatsa S. Bhat wrote:
>>>> Using global rwlocks as the backend for per-CPU rwlocks helps us avoid many
>>>> lock-ordering related problems (unlike per-cpu locks). However, global
>>>> rwlocks lead to unnecessary cache-line bouncing even when there are no
>>>> writers present, which can slow down the system needlessly.
>>>>
>> [...]
>>>> + /*
>>>> + * We never allow heterogeneous nesting of readers. So it is trivial
>>>> + * to find out the kind of reader we are, and undo the operation
>>>> + * done by our corresponding percpu_read_lock().
>>>> + */
>>>> + if (__this_cpu_read(*pcpu_rwlock->reader_refcnt)) {
>>>> + this_cpu_dec(*pcpu_rwlock->reader_refcnt);
>>>> + smp_wmb(); /* Paired with smp_rmb() in sync_reader() */
>>>
>>> Given an smp_mb() above, I don't understand the need for this smp_wmb().
>>> Isn't the idea that if the writer sees ->reader_refcnt decremented to
>>> zero, it also needs to see the effects of the corresponding reader's
>>> critical section?
>>>
>>
>> Not sure what you meant, but my idea here was that the writer should see
>> the reader_refcnt falling to zero as soon as possible, to avoid keeping the
>> writer waiting in a tight loop for longer than necessary.
>> I might have been a little over-zealous to use lighter memory barriers though,
>> (given our lengthy discussions in the previous versions to reduce the memory
>> barrier overheads), so the smp_wmb() used above might be wrong.
>>
>> So, are you saying that the smp_mb() you indicated above would be enough
>> to make the writer observe the 1->0 transition of reader_refcnt immediately?
>>
>>> Or am I missing something subtle here? In any case, if this smp_wmb()
>>> really is needed, there should be some subsequent write that the writer
>>> might observe. From what I can see, there is no subsequent write from
>>> this reader that the writer cares about.
>>
>> I thought the smp_wmb() here and the smp_rmb() at the writer would ensure
>> immediate reflection of the reader state at the writer side... Please correct
>> me if my understanding is incorrect.
>
> Ah, but memory barriers are not so much about making data move faster
> through the machine, but more about making sure that ordering constraints
> are met. After all, memory barriers cannot make electrons flow faster
> through silicon. You should therefore use memory barriers only to
> constrain ordering, not to try to expedite electrons.
>
I guess I must have been confused after looking at that graph which showed
how much time it takes for other CPUs to notice the change in value of a
variable performed in a given CPU.. and must have gotten the (wrong) idea
that memory barriers also help speed that up! Very sorry about that!
>>>> + } else {
>>>> + read_unlock(&pcpu_rwlock->global_rwlock);
>>>> + }
>>>> +
>>>> + preempt_enable();
>>>> +}
>>>> +
>>>> +static inline void raise_writer_signal(struct percpu_rwlock *pcpu_rwlock,
>>>> + unsigned int cpu)
>>>> +{
>>>> + per_cpu(*pcpu_rwlock->writer_signal, cpu) = true;
>>>> +}
>>>> +
>>>> +static inline void drop_writer_signal(struct percpu_rwlock *pcpu_rwlock,
>>>> + unsigned int cpu)
>>>> +{
>>>> + per_cpu(*pcpu_rwlock->writer_signal, cpu) = false;
>>>> +}
>>>> +
>>>> +static void announce_writer_active(struct percpu_rwlock *pcpu_rwlock)
>>>> +{
>>>> + unsigned int cpu;
>>>> +
>>>> + for_each_online_cpu(cpu)
>>>> + raise_writer_signal(pcpu_rwlock, cpu);
>>>> +
>>>> + smp_mb(); /* Paired with smp_rmb() in percpu_read_[un]lock() */
>>>> +}
>>>> +
>>>> +static void announce_writer_inactive(struct percpu_rwlock *pcpu_rwlock)
>>>> +{
>>>> + unsigned int cpu;
>>>> +
>>>> + drop_writer_signal(pcpu_rwlock, smp_processor_id());
>>>
>>> Why do we drop ourselves twice? More to the point, why is it important to
>>> drop ourselves first?
>>
>> I don't see where we are dropping ourselves twice. Note that we are no longer
>> in the cpu_online_mask, so the 'for' loop below won't include us. So we need
>> to manually drop ourselves. It doesn't matter whether we drop ourselves first
>> or later.
>
> Good point, apologies for my confusion! Still worth a commment, though.
>
Sure, will add it.
>>>> +
>>>> + for_each_online_cpu(cpu)
>>>> + drop_writer_signal(pcpu_rwlock, cpu);
>>>> +
>>>> + smp_mb(); /* Paired with smp_rmb() in percpu_read_[un]lock() */
>>>> +}
>>>> +
>>>> +/*
>>>> + * Wait for the reader to see the writer's signal and switch from percpu
>>>> + * refcounts to global rwlock.
>>>> + *
>>>> + * If the reader is still using percpu refcounts, wait for him to switch.
>>>> + * Else, we can safely go ahead, because either the reader has already
>>>> + * switched over, or the next reader that comes along on that CPU will
>>>> + * notice the writer's signal and will switch over to the rwlock.
>>>> + */
>>>> +static inline void sync_reader(struct percpu_rwlock *pcpu_rwlock,
>>>> + unsigned int cpu)
>>>> +{
>>>> + smp_rmb(); /* Paired with smp_[w]mb() in percpu_read_[un]lock() */
>>>
>>> As I understand it, the purpose of this memory barrier is to ensure
>>> that the stores in drop_writer_signal() happen before the reads from
>>> ->reader_refcnt in reader_uses_percpu_refcnt(),
>>
>> No, that was not what I intended. announce_writer_inactive() already does
>> a full smp_mb() after calling drop_writer_signal().
>>
>> I put the smp_rmb() here and the smp_wmb() at the reader side (after updates
>> to the ->reader_refcnt) to reflect the state change of ->reader_refcnt
>> immediately at the writer, so that the writer doesn't have to keep spinning
>> unnecessarily still referring to the old (non-zero) value of ->reader_refcnt.
>> Or perhaps I am confused about how to use memory barriers properly.. :-(
>
> Sadly, no, memory barriers don't make electrons move faster. So you
> should only need the one -- the additional memory barriers are just
> slowing things down.
>
Ok..
>>> thus preventing the
>>> race between a new reader attempting to use the fastpath and this writer
>>> acquiring the lock. Unless I am confused, this must be smp_mb() rather
>>> than smp_rmb().
>>>
>>> Also, why not just have a single smp_mb() at the beginning of
>>> sync_all_readers() instead of executing one barrier per CPU?
>>
>> Well, since my intention was to help the writer see the update (->reader_refcnt
>> dropping to zero) ASAP, I kept the multiple smp_rmb()s.
>
> At least you were consistent. ;-)
>
Haha, that's an optimistic way of looking at it, but its no good if I was
consistently _wrong_! ;-)
>>>> +
>>>> + while (reader_uses_percpu_refcnt(pcpu_rwlock, cpu))
>>>> + cpu_relax();
>>>> +}
>>>> +
>>>> +static void sync_all_readers(struct percpu_rwlock *pcpu_rwlock)
>>>> +{
>>>> + unsigned int cpu;
>>>> +
>>>> + for_each_online_cpu(cpu)
>>>> + sync_reader(pcpu_rwlock, cpu);
>>>> }
>>>>
>>>> void percpu_write_lock(struct percpu_rwlock *pcpu_rwlock)
>>>> {
>>>> + /*
>>>> + * Tell all readers that a writer is becoming active, so that they
>>>> + * start switching over to the global rwlock.
>>>> + */
>>>> + announce_writer_active(pcpu_rwlock);
>>>> + sync_all_readers(pcpu_rwlock);
>>>> write_lock(&pcpu_rwlock->global_rwlock);
>>>> }
>>>>
>>>> void percpu_write_unlock(struct percpu_rwlock *pcpu_rwlock)
>>>> {
>>>> + /*
>>>> + * Inform all readers that we are done, so that they can switch back
>>>> + * to their per-cpu refcounts. (We don't need to wait for them to
>>>> + * see it).
>>>> + */
>>>> + announce_writer_inactive(pcpu_rwlock);
>>>> write_unlock(&pcpu_rwlock->global_rwlock);
>>>> }
>>>>
>>>>
>>
>> Thanks a lot for your detailed review and comments! :-)
>
> It will be good to get this in!
>
Thank you :-) I'll try to address the review comments and respin the
patchset soon.
Regards,
Srivatsa S. Bhat
^ permalink raw reply
* Re: [PATCH v5 09/45] smp, cpu hotplug: Fix smp_call_function_*() to prevent CPU offline properly
From: Paul E. McKenney @ 2013-02-10 19:56 UTC (permalink / raw)
To: Srivatsa S. Bhat
Cc: tglx, peterz, tj, oleg, rusty, mingo, akpm, namhyung, rostedt,
wangyun, xiaoguangrong, rjw, sbw, fweisbec, linux, nikunj,
linux-pm, linux-arch, linux-arm-kernel, linuxppc-dev, netdev,
linux-doc, linux-kernel
In-Reply-To: <5117F7E9.7070906@linux.vnet.ibm.com>
On Mon, Feb 11, 2013 at 01:11:29AM +0530, Srivatsa S. Bhat wrote:
> On 02/09/2013 05:37 AM, Paul E. McKenney wrote:
> > On Tue, Jan 22, 2013 at 01:05:10PM +0530, Srivatsa S. Bhat wrote:
> >> Once stop_machine() is gone from the CPU offline path, we won't be able to
> >> depend on preempt_disable() to prevent CPUs from going offline from under us.
> >>
> >> Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
> >> while invoking from atomic context.
> >>
> >> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> >
> > Would it make sense for get_online_cpus_atomic() to return the current
> > CPU number?
>
> Hmm, I'm not so sure. I tried to model it after get_online_cpus(), which doesn't
> return anything (for other reasons, of course..)
>
> Moreover, a function name like *_cpu_* returning the CPU number would be intuitive.
> But a name such as *_cpus_* (plural) returning a CPU number might appear confusing..
>
> And also I don't think it'll make a huge improvement in the callers.. (We might
> be better off avoiding an smp_processor_id() if possible, since this function could
> be called in very hot paths).. So I don't see a strong case for returning the
> CPU number. But let me know if you think it'll still be worth it for some reason...
I just noted a lot of two-line code sequences in your patch that would be
one line if the CPU number was returned. But I don't feel strongly about
it, so if people are OK with the current version, no problem.
Thanx, Paul
> > Looks good otherwise.
> >
>
> Thank you very much for the detailed review, Paul!
>
> Regards,
> Srivatsa S. Bhat
>
> >
> >> ---
> >>
> >> kernel/smp.c | 40 ++++++++++++++++++++++++++--------------
> >> 1 file changed, 26 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/kernel/smp.c b/kernel/smp.c
> >> index 29dd40a..f421bcc 100644
> >> --- a/kernel/smp.c
> >> +++ b/kernel/smp.c
> >> @@ -310,7 +310,8 @@ int smp_call_function_single(int cpu, smp_call_func_t func, void *info,
> >> * prevent preemption and reschedule on another processor,
> >> * as well as CPU removal
> >> */
> >> - this_cpu = get_cpu();
> >> + get_online_cpus_atomic();
> >> + this_cpu = smp_processor_id();
> >>
> >> /*
> >> * Can deadlock when called with interrupts disabled.
> >> @@ -342,7 +343,7 @@ int smp_call_function_single(int cpu, smp_call_func_t func, void *info,
> >> }
> >> }
> >>
> >> - put_cpu();
> >> + put_online_cpus_atomic();
> >>
> >> return err;
> >> }
> >> @@ -371,8 +372,10 @@ int smp_call_function_any(const struct cpumask *mask,
> >> const struct cpumask *nodemask;
> >> int ret;
> >>
> >> + get_online_cpus_atomic();
> >> /* Try for same CPU (cheapest) */
> >> - cpu = get_cpu();
> >> + cpu = smp_processor_id();
> >> +
> >> if (cpumask_test_cpu(cpu, mask))
> >> goto call;
> >>
> >> @@ -388,7 +391,7 @@ int smp_call_function_any(const struct cpumask *mask,
> >> cpu = cpumask_any_and(mask, cpu_online_mask);
> >> call:
> >> ret = smp_call_function_single(cpu, func, info, wait);
> >> - put_cpu();
> >> + put_online_cpus_atomic();
> >> return ret;
> >> }
> >> EXPORT_SYMBOL_GPL(smp_call_function_any);
> >> @@ -409,25 +412,28 @@ void __smp_call_function_single(int cpu, struct call_single_data *data,
> >> unsigned int this_cpu;
> >> unsigned long flags;
> >>
> >> - this_cpu = get_cpu();
> >> + get_online_cpus_atomic();
> >> +
> >> + this_cpu = smp_processor_id();
> >> +
> >> /*
> >> * Can deadlock when called with interrupts disabled.
> >> * We allow cpu's that are not yet online though, as no one else can
> >> * send smp call function interrupt to this cpu and as such deadlocks
> >> * can't happen.
> >> */
> >> - WARN_ON_ONCE(cpu_online(smp_processor_id()) && wait && irqs_disabled()
> >> + WARN_ON_ONCE(cpu_online(this_cpu) && wait && irqs_disabled()
> >> && !oops_in_progress);
> >>
> >> if (cpu == this_cpu) {
> >> local_irq_save(flags);
> >> data->func(data->info);
> >> local_irq_restore(flags);
> >> - } else {
> >> + } else if ((unsigned)cpu < nr_cpu_ids && cpu_online(cpu)) {
> >> csd_lock(data);
> >> generic_exec_single(cpu, data, wait);
> >> }
> >> - put_cpu();
> >> + put_online_cpus_atomic();
> >> }
> >>
> >> /**
> >> @@ -451,6 +457,8 @@ void smp_call_function_many(const struct cpumask *mask,
> >> unsigned long flags;
> >> int refs, cpu, next_cpu, this_cpu = smp_processor_id();
> >>
> >> + get_online_cpus_atomic();
> >> +
> >> /*
> >> * Can deadlock when called with interrupts disabled.
> >> * We allow cpu's that are not yet online though, as no one else can
> >> @@ -467,17 +475,18 @@ void smp_call_function_many(const struct cpumask *mask,
> >>
> >> /* No online cpus? We're done. */
> >> if (cpu >= nr_cpu_ids)
> >> - return;
> >> + goto out_unlock;
> >>
> >> /* Do we have another CPU which isn't us? */
> >> next_cpu = cpumask_next_and(cpu, mask, cpu_online_mask);
> >> if (next_cpu == this_cpu)
> >> - next_cpu = cpumask_next_and(next_cpu, mask, cpu_online_mask);
> >> + next_cpu = cpumask_next_and(next_cpu, mask,
> >> + cpu_online_mask);
> >>
> >> /* Fastpath: do that cpu by itself. */
> >> if (next_cpu >= nr_cpu_ids) {
> >> smp_call_function_single(cpu, func, info, wait);
> >> - return;
> >> + goto out_unlock;
> >> }
> >>
> >> data = &__get_cpu_var(cfd_data);
> >> @@ -523,7 +532,7 @@ void smp_call_function_many(const struct cpumask *mask,
> >> /* Some callers race with other cpus changing the passed mask */
> >> if (unlikely(!refs)) {
> >> csd_unlock(&data->csd);
> >> - return;
> >> + goto out_unlock;
> >> }
> >>
> >> raw_spin_lock_irqsave(&call_function.lock, flags);
> >> @@ -554,6 +563,9 @@ void smp_call_function_many(const struct cpumask *mask,
> >> /* Optionally wait for the CPUs to complete */
> >> if (wait)
> >> csd_lock_wait(&data->csd);
> >> +
> >> +out_unlock:
> >> + put_online_cpus_atomic();
> >> }
> >> EXPORT_SYMBOL(smp_call_function_many);
> >>
> >> @@ -574,9 +586,9 @@ EXPORT_SYMBOL(smp_call_function_many);
> >> */
> >> int smp_call_function(smp_call_func_t func, void *info, int wait)
> >> {
> >> - preempt_disable();
> >> + get_online_cpus_atomic();
> >> smp_call_function_many(cpu_online_mask, func, info, wait);
> >> - preempt_enable();
> >> + put_online_cpus_atomic();
> >>
> >> return 0;
> >> }
> >>
>
^ permalink raw reply
* Re: [PATCH v5 04/45] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks
From: Paul E. McKenney @ 2013-02-10 19:54 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Srivatsa S. Bhat, tglx, peterz, tj, rusty, mingo, akpm, namhyung,
rostedt, wangyun, xiaoguangrong, rjw, sbw, fweisbec, linux,
nikunj, linux-pm, linux-arch, linux-arm-kernel, linuxppc-dev,
netdev, linux-doc, linux-kernel
In-Reply-To: <20130210180607.GA1375@redhat.com>
On Sun, Feb 10, 2013 at 07:06:07PM +0100, Oleg Nesterov wrote:
> On 02/08, Paul E. McKenney wrote:
[ . . . ]
> > > +static inline void sync_reader(struct percpu_rwlock *pcpu_rwlock,
> > > + unsigned int cpu)
> > > +{
> > > + smp_rmb(); /* Paired with smp_[w]mb() in percpu_read_[un]lock() */
> >
> > As I understand it, the purpose of this memory barrier is to ensure
> > that the stores in drop_writer_signal() happen before the reads from
> > ->reader_refcnt in reader_uses_percpu_refcnt(), thus preventing the
> > race between a new reader attempting to use the fastpath and this writer
> > acquiring the lock. Unless I am confused, this must be smp_mb() rather
> > than smp_rmb().
>
> And note that before sync_reader() we call announce_writer_active() which
> already adds mb() before sync_all_readers/sync_reader, so this rmb() looks
> unneeded.
>
> But, at the same time, could you confirm that we do not need another mb()
> after sync_all_readers() in percpu_write_lock() ? I mean, without mb(),
> can't this reader_uses_percpu_refcnt() LOAD leak into the critical section
> protected by ->global_rwlock? Then this LOAD can be re-ordered with other
> memory operations done by the writer.
As soon as I get the rest of the way through Thomas's patchset. ;-)
Thanx, Paul
^ permalink raw reply
* Re: [PATCH v5 04/45] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks
From: Oleg Nesterov @ 2013-02-10 19:50 UTC (permalink / raw)
To: Srivatsa S. Bhat
Cc: Paul E. McKenney, tglx, peterz, tj, rusty, mingo, akpm, namhyung,
rostedt, wangyun, xiaoguangrong, rjw, sbw, fweisbec, linux,
nikunj, linux-pm, linux-arch, linux-arm-kernel, linuxppc-dev,
netdev, linux-doc, linux-kernel
In-Reply-To: <5117F403.1050300@linux.vnet.ibm.com>
On 02/11, Srivatsa S. Bhat wrote:
>
> On 02/10/2013 11:36 PM, Oleg Nesterov wrote:
> >>> +static void announce_writer_inactive(struct percpu_rwlock *pcpu_rwlock)
> >>> +{
> >>> + unsigned int cpu;
> >>> +
> >>> + drop_writer_signal(pcpu_rwlock, smp_processor_id());
> >>
> >> Why do we drop ourselves twice? More to the point, why is it important to
> >> drop ourselves first?
> >
> > And don't we need mb() _before_ we clear ->writer_signal ?
> >
>
> Oh, right! Or, how about moving announce_writer_inactive() to _after_
> write_unlock()?
Not sure this will help... but, either way it seems we have another
problem...
percpu_rwlock tries to be "generic". This means we should "ignore" its
usage in hotplug, and _write_lock should not race with _write_unlock.
IOW. Suppose that _write_unlock clears ->writer_signal. We need to ensure
that this can't race with another write which wants to set this flag.
Perhaps it should be counter as well, and it should be protected by
the same ->global_rwlock, but _write_lock() should drop it before
sync_all_readers() and then take it again?
> >>> +static inline void sync_reader(struct percpu_rwlock *pcpu_rwlock,
> >>> + unsigned int cpu)
> >>> +{
> >>> + smp_rmb(); /* Paired with smp_[w]mb() in percpu_read_[un]lock() */
> >>
> >> As I understand it, the purpose of this memory barrier is to ensure
> >> that the stores in drop_writer_signal() happen before the reads from
> >> ->reader_refcnt in reader_uses_percpu_refcnt(), thus preventing the
> >> race between a new reader attempting to use the fastpath and this writer
> >> acquiring the lock. Unless I am confused, this must be smp_mb() rather
> >> than smp_rmb().
> >
> > And note that before sync_reader() we call announce_writer_active() which
> > already adds mb() before sync_all_readers/sync_reader, so this rmb() looks
> > unneeded.
> >
>
> My intention was to help the writer see the ->reader_refcnt drop to zero
> ASAP; hence I used smp_wmb() at reader and smp_rmb() here at the writer.
Hmm, interesting... Not sure, but can't really comment. However I can
answer your next question:
> Please correct me if my understanding of memory barriers is wrong here..
Who? Me??? No we have paulmck for that ;)
Oleg.
^ permalink raw reply
* Re: [PATCH v5 04/45] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks
From: Paul E. McKenney @ 2013-02-10 19:47 UTC (permalink / raw)
To: Srivatsa S. Bhat
Cc: tglx, peterz, tj, oleg, rusty, mingo, akpm, namhyung, rostedt,
wangyun, xiaoguangrong, rjw, sbw, fweisbec, linux, nikunj,
linux-pm, linux-arch, linux-arm-kernel, linuxppc-dev, netdev,
linux-doc, linux-kernel
In-Reply-To: <5117F0C0.2030605@linux.vnet.ibm.com>
On Mon, Feb 11, 2013 at 12:40:56AM +0530, Srivatsa S. Bhat wrote:
> On 02/09/2013 04:40 AM, Paul E. McKenney wrote:
> > On Tue, Jan 22, 2013 at 01:03:53PM +0530, Srivatsa S. Bhat wrote:
> >> Using global rwlocks as the backend for per-CPU rwlocks helps us avoid many
> >> lock-ordering related problems (unlike per-cpu locks). However, global
> >> rwlocks lead to unnecessary cache-line bouncing even when there are no
> >> writers present, which can slow down the system needlessly.
> >>
> [...]
> >
> > Looks pretty close! Some comments interspersed below. Please either
> > fix the code or my confusion, as the case may be. ;-)
> >
>
> Sure :-)
>
> >> ---
> >>
> >> include/linux/percpu-rwlock.h | 10 +++
> >> lib/percpu-rwlock.c | 128 ++++++++++++++++++++++++++++++++++++++++-
> >> 2 files changed, 136 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/include/linux/percpu-rwlock.h b/include/linux/percpu-rwlock.h
> >> index 8dec8fe..6819bb8 100644
> >> --- a/include/linux/percpu-rwlock.h
> >> +++ b/include/linux/percpu-rwlock.h
> >> @@ -68,4 +68,14 @@ extern void percpu_free_rwlock(struct percpu_rwlock *);
> >> __percpu_init_rwlock(pcpu_rwlock, #pcpu_rwlock, &rwlock_key); \
> >> })
> >>
> >> +#define reader_uses_percpu_refcnt(pcpu_rwlock, cpu) \
> >> + (ACCESS_ONCE(per_cpu(*((pcpu_rwlock)->reader_refcnt), cpu)))
> >> +
> >> +#define reader_nested_percpu(pcpu_rwlock) \
> >> + (__this_cpu_read(*((pcpu_rwlock)->reader_refcnt)) > 1)
> >> +
> >> +#define writer_active(pcpu_rwlock) \
> >> + (__this_cpu_read(*((pcpu_rwlock)->writer_signal)))
> >> +
> >> #endif
> >> +
> >> diff --git a/lib/percpu-rwlock.c b/lib/percpu-rwlock.c
> >> index 80dad93..992da5c 100644
> >> --- a/lib/percpu-rwlock.c
> >> +++ b/lib/percpu-rwlock.c
> >> @@ -64,21 +64,145 @@ void percpu_free_rwlock(struct percpu_rwlock *pcpu_rwlock)
> >>
> >> void percpu_read_lock(struct percpu_rwlock *pcpu_rwlock)
> >> {
> >> - read_lock(&pcpu_rwlock->global_rwlock);
> >> + preempt_disable();
> >> +
> >> + /* First and foremost, let the writer know that a reader is active */
> >> + this_cpu_inc(*pcpu_rwlock->reader_refcnt);
> >> +
> >> + /*
> >> + * If we are already using per-cpu refcounts, it is not safe to switch
> >> + * the synchronization scheme. So continue using the refcounts.
> >> + */
> >> + if (reader_nested_percpu(pcpu_rwlock)) {
> >> + goto out;
> >> + } else {
> >> + /*
> >> + * The write to 'reader_refcnt' must be visible before we
> >> + * read 'writer_signal'.
> >> + */
> >> + smp_mb(); /* Paired with smp_rmb() in sync_reader() */
> >> +
> >> + if (likely(!writer_active(pcpu_rwlock))) {
> >> + goto out;
> >> + } else {
> >> + /* Writer is active, so switch to global rwlock. */
> >> + read_lock(&pcpu_rwlock->global_rwlock);
> >> +
> >> + /*
> >> + * We might have raced with a writer going inactive
> >> + * before we took the read-lock. So re-evaluate whether
> >> + * we still need to hold the rwlock or if we can switch
> >> + * back to per-cpu refcounts. (This also helps avoid
> >> + * heterogeneous nesting of readers).
> >> + */
> >> + if (writer_active(pcpu_rwlock))
> >
> > The above writer_active() can be reordered with the following this_cpu_dec(),
> > strange though it might seem. But this is OK because holding the rwlock
> > is conservative. But might be worth a comment.
> >
>
> Ok..
>
> >> + this_cpu_dec(*pcpu_rwlock->reader_refcnt);
> >> + else
> >
> > In contrast, no reordering can happen here because read_unlock() is
> > required to keep the critical section underneath the lock.
> >
>
> Ok..
>
> >> + read_unlock(&pcpu_rwlock->global_rwlock);
> >> + }
> >> + }
> >> +
> >> +out:
> >> + /* Prevent reordering of any subsequent reads */
> >> + smp_rmb();
> >
> > This should be smp_mb(). "Readers" really can do writes. Hence the
> > name lglock -- "local/global" rather than "reader/writer".
> >
>
> Ok!
>
> [ We were trying to avoid full memory barriers in get/put_online_cpus_atomic()
> in the fastpath, as far as possible... Now it feels like all that discussion
> was for nothing :-( ]
>
> >> }
> >>
> >> void percpu_read_unlock(struct percpu_rwlock *pcpu_rwlock)
> >> {
> >> - read_unlock(&pcpu_rwlock->global_rwlock);
> >
> > We need an smp_mb() here to keep the critical section ordered before the
> > this_cpu_dec() below. Otherwise, if a writer shows up just after we
> > exit the fastpath, that writer is not guaranteed to see the effects of
> > our critical section. Equivalently, the prior read-side critical section
> > just might see some of the writer's updates, which could be a bit of
> > a surprise to the reader.
> >
>
> Ok, will add it.
>
> >> + /*
> >> + * We never allow heterogeneous nesting of readers. So it is trivial
> >> + * to find out the kind of reader we are, and undo the operation
> >> + * done by our corresponding percpu_read_lock().
> >> + */
> >> + if (__this_cpu_read(*pcpu_rwlock->reader_refcnt)) {
> >> + this_cpu_dec(*pcpu_rwlock->reader_refcnt);
> >> + smp_wmb(); /* Paired with smp_rmb() in sync_reader() */
> >
> > Given an smp_mb() above, I don't understand the need for this smp_wmb().
> > Isn't the idea that if the writer sees ->reader_refcnt decremented to
> > zero, it also needs to see the effects of the corresponding reader's
> > critical section?
> >
>
> Not sure what you meant, but my idea here was that the writer should see
> the reader_refcnt falling to zero as soon as possible, to avoid keeping the
> writer waiting in a tight loop for longer than necessary.
> I might have been a little over-zealous to use lighter memory barriers though,
> (given our lengthy discussions in the previous versions to reduce the memory
> barrier overheads), so the smp_wmb() used above might be wrong.
>
> So, are you saying that the smp_mb() you indicated above would be enough
> to make the writer observe the 1->0 transition of reader_refcnt immediately?
>
> > Or am I missing something subtle here? In any case, if this smp_wmb()
> > really is needed, there should be some subsequent write that the writer
> > might observe. From what I can see, there is no subsequent write from
> > this reader that the writer cares about.
>
> I thought the smp_wmb() here and the smp_rmb() at the writer would ensure
> immediate reflection of the reader state at the writer side... Please correct
> me if my understanding is incorrect.
Ah, but memory barriers are not so much about making data move faster
through the machine, but more about making sure that ordering constraints
are met. After all, memory barriers cannot make electrons flow faster
through silicon. You should therefore use memory barriers only to
constrain ordering, not to try to expedite electrons.
> >> + } else {
> >> + read_unlock(&pcpu_rwlock->global_rwlock);
> >> + }
> >> +
> >> + preempt_enable();
> >> +}
> >> +
> >> +static inline void raise_writer_signal(struct percpu_rwlock *pcpu_rwlock,
> >> + unsigned int cpu)
> >> +{
> >> + per_cpu(*pcpu_rwlock->writer_signal, cpu) = true;
> >> +}
> >> +
> >> +static inline void drop_writer_signal(struct percpu_rwlock *pcpu_rwlock,
> >> + unsigned int cpu)
> >> +{
> >> + per_cpu(*pcpu_rwlock->writer_signal, cpu) = false;
> >> +}
> >> +
> >> +static void announce_writer_active(struct percpu_rwlock *pcpu_rwlock)
> >> +{
> >> + unsigned int cpu;
> >> +
> >> + for_each_online_cpu(cpu)
> >> + raise_writer_signal(pcpu_rwlock, cpu);
> >> +
> >> + smp_mb(); /* Paired with smp_rmb() in percpu_read_[un]lock() */
> >> +}
> >> +
> >> +static void announce_writer_inactive(struct percpu_rwlock *pcpu_rwlock)
> >> +{
> >> + unsigned int cpu;
> >> +
> >> + drop_writer_signal(pcpu_rwlock, smp_processor_id());
> >
> > Why do we drop ourselves twice? More to the point, why is it important to
> > drop ourselves first?
>
> I don't see where we are dropping ourselves twice. Note that we are no longer
> in the cpu_online_mask, so the 'for' loop below won't include us. So we need
> to manually drop ourselves. It doesn't matter whether we drop ourselves first
> or later.
Good point, apologies for my confusion! Still worth a commment, though.
> >> +
> >> + for_each_online_cpu(cpu)
> >> + drop_writer_signal(pcpu_rwlock, cpu);
> >> +
> >> + smp_mb(); /* Paired with smp_rmb() in percpu_read_[un]lock() */
> >> +}
> >> +
> >> +/*
> >> + * Wait for the reader to see the writer's signal and switch from percpu
> >> + * refcounts to global rwlock.
> >> + *
> >> + * If the reader is still using percpu refcounts, wait for him to switch.
> >> + * Else, we can safely go ahead, because either the reader has already
> >> + * switched over, or the next reader that comes along on that CPU will
> >> + * notice the writer's signal and will switch over to the rwlock.
> >> + */
> >> +static inline void sync_reader(struct percpu_rwlock *pcpu_rwlock,
> >> + unsigned int cpu)
> >> +{
> >> + smp_rmb(); /* Paired with smp_[w]mb() in percpu_read_[un]lock() */
> >
> > As I understand it, the purpose of this memory barrier is to ensure
> > that the stores in drop_writer_signal() happen before the reads from
> > ->reader_refcnt in reader_uses_percpu_refcnt(),
>
> No, that was not what I intended. announce_writer_inactive() already does
> a full smp_mb() after calling drop_writer_signal().
>
> I put the smp_rmb() here and the smp_wmb() at the reader side (after updates
> to the ->reader_refcnt) to reflect the state change of ->reader_refcnt
> immediately at the writer, so that the writer doesn't have to keep spinning
> unnecessarily still referring to the old (non-zero) value of ->reader_refcnt.
> Or perhaps I am confused about how to use memory barriers properly.. :-(
Sadly, no, memory barriers don't make electrons move faster. So you
should only need the one -- the additional memory barriers are just
slowing things down.
> > thus preventing the
> > race between a new reader attempting to use the fastpath and this writer
> > acquiring the lock. Unless I am confused, this must be smp_mb() rather
> > than smp_rmb().
> >
> > Also, why not just have a single smp_mb() at the beginning of
> > sync_all_readers() instead of executing one barrier per CPU?
>
> Well, since my intention was to help the writer see the update (->reader_refcnt
> dropping to zero) ASAP, I kept the multiple smp_rmb()s.
At least you were consistent. ;-)
> >> +
> >> + while (reader_uses_percpu_refcnt(pcpu_rwlock, cpu))
> >> + cpu_relax();
> >> +}
> >> +
> >> +static void sync_all_readers(struct percpu_rwlock *pcpu_rwlock)
> >> +{
> >> + unsigned int cpu;
> >> +
> >> + for_each_online_cpu(cpu)
> >> + sync_reader(pcpu_rwlock, cpu);
> >> }
> >>
> >> void percpu_write_lock(struct percpu_rwlock *pcpu_rwlock)
> >> {
> >> + /*
> >> + * Tell all readers that a writer is becoming active, so that they
> >> + * start switching over to the global rwlock.
> >> + */
> >> + announce_writer_active(pcpu_rwlock);
> >> + sync_all_readers(pcpu_rwlock);
> >> write_lock(&pcpu_rwlock->global_rwlock);
> >> }
> >>
> >> void percpu_write_unlock(struct percpu_rwlock *pcpu_rwlock)
> >> {
> >> + /*
> >> + * Inform all readers that we are done, so that they can switch back
> >> + * to their per-cpu refcounts. (We don't need to wait for them to
> >> + * see it).
> >> + */
> >> + announce_writer_inactive(pcpu_rwlock);
> >> write_unlock(&pcpu_rwlock->global_rwlock);
> >> }
> >>
> >>
>
> Thanks a lot for your detailed review and comments! :-)
It will be good to get this in!
Thanx, Paul
^ permalink raw reply
* Re: [PATCH v5 44/45] CPU hotplug, stop_machine: Decouple CPU hotplug from stop_machine() in Kconfig
From: Srivatsa S. Bhat @ 2013-02-10 19:45 UTC (permalink / raw)
To: paulmck
Cc: tglx, peterz, tj, oleg, rusty, mingo, akpm, namhyung, rostedt,
wangyun, xiaoguangrong, rjw, sbw, fweisbec, linux, nikunj,
linux-pm, linux-arch, linux-arm-kernel, linuxppc-dev, netdev,
linux-doc, linux-kernel
In-Reply-To: <20130209001554.GR2666@linux.vnet.ibm.com>
On 02/09/2013 05:45 AM, Paul E. McKenney wrote:
> On Tue, Jan 22, 2013 at 01:15:22PM +0530, Srivatsa S. Bhat wrote:
>> ... and also cleanup a comment that refers to CPU hotplug being dependent on
>> stop_machine().
>>
>> Cc: David Howells <dhowells@redhat.com>
>> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>
> Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>
> (Hey, I thought I owed myself an easy one!)
>
Haha ;-)
Regards,
Srivatsa S. Bhat
>> ---
>>
>> include/linux/stop_machine.h | 2 +-
>> init/Kconfig | 2 +-
>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/stop_machine.h b/include/linux/stop_machine.h
>> index 3b5e910..ce2d3c4 100644
>> --- a/include/linux/stop_machine.h
>> +++ b/include/linux/stop_machine.h
>> @@ -120,7 +120,7 @@ int stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus);
>> * @cpus: the cpus to run the @fn() on (NULL = any online cpu)
>> *
>> * Description: This is a special version of the above, which assumes cpus
>> - * won't come or go while it's being called. Used by hotplug cpu.
>> + * won't come or go while it's being called.
>> */
>> int __stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus);
>>
>> diff --git a/init/Kconfig b/init/Kconfig
>> index be8b7f5..048a0c5 100644
>> --- a/init/Kconfig
>> +++ b/init/Kconfig
>> @@ -1711,7 +1711,7 @@ config INIT_ALL_POSSIBLE
>> config STOP_MACHINE
>> bool
>> default y
>> - depends on (SMP && MODULE_UNLOAD) || HOTPLUG_CPU
>> + depends on (SMP && MODULE_UNLOAD)
>> help
>> Need stop_machine() primitive.
>>
>>
^ permalink raw reply
* Re: [PATCH v5 14/45] rcu, CPU hotplug: Fix comment referring to stop_machine()
From: Srivatsa S. Bhat @ 2013-02-10 19:43 UTC (permalink / raw)
To: paulmck
Cc: tglx, peterz, tj, oleg, rusty, mingo, akpm, namhyung, rostedt,
wangyun, xiaoguangrong, rjw, sbw, fweisbec, linux, nikunj,
linux-pm, linux-arch, linux-arm-kernel, linuxppc-dev, netdev,
linux-doc, linux-kernel
In-Reply-To: <20130209001439.GQ2666@linux.vnet.ibm.com>
On 02/09/2013 05:44 AM, Paul E. McKenney wrote:
> On Tue, Jan 22, 2013 at 01:06:34PM +0530, Srivatsa S. Bhat wrote:
>> Don't refer to stop_machine() in the CPU hotplug path, since we are going
>> to get rid of it. Also, move the comment referring to callback adoption
>> to the CPU_DEAD case, because that's where it happens now.
>>
>> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>
> Ouch! That comment is indeed obsolete and must die.
>
> I queued this to -rcu with your Signed-off-by. However, I omitted
> the added comment, as it is imcomplete -- it is easy to look at
> rcu_cleanup_dead_cpu() to see what it does.
>
Cool! I'll drop it from my patch series then. Thank you!
Regards,
Srivatsa S. Bhat
^ permalink raw reply
* Re: [PATCH v5 09/45] smp, cpu hotplug: Fix smp_call_function_*() to prevent CPU offline properly
From: Srivatsa S. Bhat @ 2013-02-10 19:41 UTC (permalink / raw)
To: paulmck
Cc: tglx, peterz, tj, oleg, rusty, mingo, akpm, namhyung, rostedt,
wangyun, xiaoguangrong, rjw, sbw, fweisbec, linux, nikunj,
linux-pm, linux-arch, linux-arm-kernel, linuxppc-dev, netdev,
linux-doc, linux-kernel
In-Reply-To: <20130209000717.GP2666@linux.vnet.ibm.com>
On 02/09/2013 05:37 AM, Paul E. McKenney wrote:
> On Tue, Jan 22, 2013 at 01:05:10PM +0530, Srivatsa S. Bhat wrote:
>> Once stop_machine() is gone from the CPU offline path, we won't be able to
>> depend on preempt_disable() to prevent CPUs from going offline from under us.
>>
>> Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
>> while invoking from atomic context.
>>
>> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>
> Would it make sense for get_online_cpus_atomic() to return the current
> CPU number?
Hmm, I'm not so sure. I tried to model it after get_online_cpus(), which doesn't
return anything (for other reasons, of course..)
Moreover, a function name like *_cpu_* returning the CPU number would be intuitive.
But a name such as *_cpus_* (plural) returning a CPU number might appear confusing..
And also I don't think it'll make a huge improvement in the callers.. (We might
be better off avoiding an smp_processor_id() if possible, since this function could
be called in very hot paths).. So I don't see a strong case for returning the
CPU number. But let me know if you think it'll still be worth it for some reason...
> Looks good otherwise.
>
Thank you very much for the detailed review, Paul!
Regards,
Srivatsa S. Bhat
>
>> ---
>>
>> kernel/smp.c | 40 ++++++++++++++++++++++++++--------------
>> 1 file changed, 26 insertions(+), 14 deletions(-)
>>
>> diff --git a/kernel/smp.c b/kernel/smp.c
>> index 29dd40a..f421bcc 100644
>> --- a/kernel/smp.c
>> +++ b/kernel/smp.c
>> @@ -310,7 +310,8 @@ int smp_call_function_single(int cpu, smp_call_func_t func, void *info,
>> * prevent preemption and reschedule on another processor,
>> * as well as CPU removal
>> */
>> - this_cpu = get_cpu();
>> + get_online_cpus_atomic();
>> + this_cpu = smp_processor_id();
>>
>> /*
>> * Can deadlock when called with interrupts disabled.
>> @@ -342,7 +343,7 @@ int smp_call_function_single(int cpu, smp_call_func_t func, void *info,
>> }
>> }
>>
>> - put_cpu();
>> + put_online_cpus_atomic();
>>
>> return err;
>> }
>> @@ -371,8 +372,10 @@ int smp_call_function_any(const struct cpumask *mask,
>> const struct cpumask *nodemask;
>> int ret;
>>
>> + get_online_cpus_atomic();
>> /* Try for same CPU (cheapest) */
>> - cpu = get_cpu();
>> + cpu = smp_processor_id();
>> +
>> if (cpumask_test_cpu(cpu, mask))
>> goto call;
>>
>> @@ -388,7 +391,7 @@ int smp_call_function_any(const struct cpumask *mask,
>> cpu = cpumask_any_and(mask, cpu_online_mask);
>> call:
>> ret = smp_call_function_single(cpu, func, info, wait);
>> - put_cpu();
>> + put_online_cpus_atomic();
>> return ret;
>> }
>> EXPORT_SYMBOL_GPL(smp_call_function_any);
>> @@ -409,25 +412,28 @@ void __smp_call_function_single(int cpu, struct call_single_data *data,
>> unsigned int this_cpu;
>> unsigned long flags;
>>
>> - this_cpu = get_cpu();
>> + get_online_cpus_atomic();
>> +
>> + this_cpu = smp_processor_id();
>> +
>> /*
>> * Can deadlock when called with interrupts disabled.
>> * We allow cpu's that are not yet online though, as no one else can
>> * send smp call function interrupt to this cpu and as such deadlocks
>> * can't happen.
>> */
>> - WARN_ON_ONCE(cpu_online(smp_processor_id()) && wait && irqs_disabled()
>> + WARN_ON_ONCE(cpu_online(this_cpu) && wait && irqs_disabled()
>> && !oops_in_progress);
>>
>> if (cpu == this_cpu) {
>> local_irq_save(flags);
>> data->func(data->info);
>> local_irq_restore(flags);
>> - } else {
>> + } else if ((unsigned)cpu < nr_cpu_ids && cpu_online(cpu)) {
>> csd_lock(data);
>> generic_exec_single(cpu, data, wait);
>> }
>> - put_cpu();
>> + put_online_cpus_atomic();
>> }
>>
>> /**
>> @@ -451,6 +457,8 @@ void smp_call_function_many(const struct cpumask *mask,
>> unsigned long flags;
>> int refs, cpu, next_cpu, this_cpu = smp_processor_id();
>>
>> + get_online_cpus_atomic();
>> +
>> /*
>> * Can deadlock when called with interrupts disabled.
>> * We allow cpu's that are not yet online though, as no one else can
>> @@ -467,17 +475,18 @@ void smp_call_function_many(const struct cpumask *mask,
>>
>> /* No online cpus? We're done. */
>> if (cpu >= nr_cpu_ids)
>> - return;
>> + goto out_unlock;
>>
>> /* Do we have another CPU which isn't us? */
>> next_cpu = cpumask_next_and(cpu, mask, cpu_online_mask);
>> if (next_cpu == this_cpu)
>> - next_cpu = cpumask_next_and(next_cpu, mask, cpu_online_mask);
>> + next_cpu = cpumask_next_and(next_cpu, mask,
>> + cpu_online_mask);
>>
>> /* Fastpath: do that cpu by itself. */
>> if (next_cpu >= nr_cpu_ids) {
>> smp_call_function_single(cpu, func, info, wait);
>> - return;
>> + goto out_unlock;
>> }
>>
>> data = &__get_cpu_var(cfd_data);
>> @@ -523,7 +532,7 @@ void smp_call_function_many(const struct cpumask *mask,
>> /* Some callers race with other cpus changing the passed mask */
>> if (unlikely(!refs)) {
>> csd_unlock(&data->csd);
>> - return;
>> + goto out_unlock;
>> }
>>
>> raw_spin_lock_irqsave(&call_function.lock, flags);
>> @@ -554,6 +563,9 @@ void smp_call_function_many(const struct cpumask *mask,
>> /* Optionally wait for the CPUs to complete */
>> if (wait)
>> csd_lock_wait(&data->csd);
>> +
>> +out_unlock:
>> + put_online_cpus_atomic();
>> }
>> EXPORT_SYMBOL(smp_call_function_many);
>>
>> @@ -574,9 +586,9 @@ EXPORT_SYMBOL(smp_call_function_many);
>> */
>> int smp_call_function(smp_call_func_t func, void *info, int wait)
>> {
>> - preempt_disable();
>> + get_online_cpus_atomic();
>> smp_call_function_many(cpu_online_mask, func, info, wait);
>> - preempt_enable();
>> + put_online_cpus_atomic();
>>
>> return 0;
>> }
>>
^ 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