[PATCH 1/3] uml: use -mcmodel=kernel for x86_64

[PATCH 1/3] uml: use -mcmodel=kernel for x86_64

[Paolo 'Blaisorblade' Giarrusso]

From: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>

We have never used this flag and recently one user experienced a complaining
warning about this (there was a symbol in the positive half of the address space
IIRC). So fix it.

Signed-off-by: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
---

 arch/um/Makefile-x86_64 |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/um/Makefile-x86_64 b/arch/um/Makefile-x86_64
index 9558a7c..11154b6 100644
--- a/arch/um/Makefile-x86_64
+++ b/arch/um/Makefile-x86_64
@@ -4,10 +4,13 @@ # Released under the GPL
 core-y += arch/um/sys-x86_64/
 START := 0x60000000
 
+_extra_flags_ = -fno-builtin -m64 -mcmodel=kernel
+
 #We #undef __x86_64__ for kernelspace, not for userspace where
 #it's needed for headers to work!
-CFLAGS += -U__$(SUBARCH)__ -fno-builtin -m64
-USER_CFLAGS += -fno-builtin -m64
+CFLAGS += -U__$(SUBARCH)__ $(_extra_flags_)
+USER_CFLAGS += $(_extra_flags_)
+
 CHECKFLAGS  += -m64
 AFLAGS += -m64
 LDFLAGS += -m elf_x86_64

[PATCH 2/3] uml: fix proc-vs-interrupt context spinlock deadlock

[Paolo 'Blaisorblade' Giarrusso]

From: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>

This spinlock can be taken on interrupt too, so spin_lock_irq[save] must be used.

However, Documentation/networking/netdevices.txt explains we are called with
rtnl_lock() held - so we don't need to care about other concurrent opens.
Verified also in LDD3 and by direct checking. Also verified that the network
layer (through a state machine) guarantees us that nobody will close the
interface while it's being used. Please correct me if I'm wrong.

Also, we must check we don't sleep with irqs disabled!!! But anyway, this is not
news - we already can't sleep while holding a spinlock. Who says this is
guaranted really by the present code?

Signed-off-by: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
---

 arch/um/drivers/net_kern.c |   16 ++++------------
 1 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/arch/um/drivers/net_kern.c b/arch/um/drivers/net_kern.c
index 6af229c..f5fba74 100644
--- a/arch/um/drivers/net_kern.c
+++ b/arch/um/drivers/net_kern.c
@@ -109,8 +109,6 @@ static int uml_net_open(struct net_devic
 	struct uml_net_private *lp = dev->priv;
 	int err;
 
-	spin_lock(&lp->lock);
-
 	if(lp->fd >= 0){
 		err = -ENXIO;
 		goto out;
@@ -144,8 +142,6 @@ static int uml_net_open(struct net_devic
 	 */
 	while((err = uml_net_rx(dev)) > 0) ;
 
-	spin_unlock(&lp->lock);
-
 	spin_lock(&opened_lock);
 	list_add(&lp->list, &opened);
 	spin_unlock(&opened_lock);
@@ -155,7 +151,6 @@ out_close:
 	if(lp->close != NULL) (*lp->close)(lp->fd, &lp->user);
 	lp->fd = -1;
 out:
-	spin_unlock(&lp->lock);
 	return err;
 }
 
@@ -164,15 +159,12 @@ static int uml_net_close(struct net_devi
 	struct uml_net_private *lp = dev->priv;
 	
 	netif_stop_queue(dev);
-	spin_lock(&lp->lock);
 
 	free_irq(dev->irq, dev);
 	if(lp->close != NULL)
 		(*lp->close)(lp->fd, &lp->user);
 	lp->fd = -1;
 
-	spin_unlock(&lp->lock);
-
 	spin_lock(&opened_lock);
 	list_del(&lp->list);
 	spin_unlock(&opened_lock);
@@ -241,9 +233,9 @@ static int uml_net_set_mac(struct net_de
 	struct uml_net_private *lp = dev->priv;
 	struct sockaddr *hwaddr = addr;
 
-	spin_lock(&lp->lock);
+	spin_lock_irq(&lp->lock);
 	memcpy(dev->dev_addr, hwaddr->sa_data, ETH_ALEN);
-	spin_unlock(&lp->lock);
+	spin_unlock_irq(&lp->lock);
 
 	return(0);
 }
@@ -253,7 +245,7 @@ static int uml_net_change_mtu(struct net
 	struct uml_net_private *lp = dev->priv;
 	int err = 0;
 
-	spin_lock(&lp->lock);
+	spin_lock_irq(&lp->lock);
 
 	new_mtu = (*lp->set_mtu)(new_mtu, &lp->user);
 	if(new_mtu < 0){
@@ -264,7 +256,7 @@ static int uml_net_change_mtu(struct net
 	dev->mtu = new_mtu;
 
  out:
-	spin_unlock(&lp->lock);
+	spin_unlock_irq(&lp->lock);
 	return err;
 }
 

Re: [PATCH 2/3] uml: fix proc-vs-interrupt context spinlock deadlock

[Jeff Dike]

On Sun, Aug 06, 2006 at 05:47:03PM +0200, Paolo 'Blaisorblade' Giarrusso wrote:
> From: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
> 
> This spinlock can be taken on interrupt too, so spin_lock_irq[save] must be used.
> 
> However, Documentation/networking/netdevices.txt explains we are called with
> rtnl_lock() held - so we don't need to care about other concurrent opens.
> Verified also in LDD3 and by direct checking. Also verified that the network
> layer (through a state machine) guarantees us that nobody will close the
> interface while it's being used. Please correct me if I'm wrong.
> 
> Also, we must check we don't sleep with irqs disabled!!! But anyway, this is not
> news - we already can't sleep while holding a spinlock. Who says this is
> guaranted really by the present code?

This patch looks fairly scary.  It's protecting the device private
data, you're removing the locking from some accesses and leaving it
(albeit with irqs off now) on others.  It seems to me that can't be
right.  It's either always there, or always not.

You observe that open and close are protected by rtnl_lock.  I observe
that uml_net_change_mtu and uml_net_set_mac are as well, in dev_ioctl.

The spinlock protecting this has to be _irqsave because the interrupt
routine takes it, to protect against receiving packets on an interface
that's being closed.  If that's impossible, we should prove that, and
remove the locking from uml_net_interrupt.

I can't decide about uml_net_start_xmit - there's some RCU stuff
around one call that leads to it, but I don't see any sign of
rtnl_lock.

So, I'd say there are some changes needed here, but they're not
entirely the ones in this patch.

				Jeff

Re: [PATCH 2/3] uml: fix proc-vs-interrupt context spinlock deadlock

[Paolo Giarrusso]

Jeff Dike <jdike@addtoit.com> ha scritto: 

> On Sun, Aug 06, 2006 at 05:47:03PM +0200, Paolo 'Blaisorblade'
> Giarrusso wrote:
> > From: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
> > 
> > This spinlock can be taken on interrupt too, so
> spin_lock_irq[save] must be used.
> > 
> > However, Documentation/networking/netdevices.txt explains we are
> called with
> > rtnl_lock() held - so we don't need to care about other
> concurrent opens.
> > Verified also in LDD3 and by direct checking. Also verified that
> the network
> > layer (through a state machine) guarantees us that nobody will
> close the
> > interface while it's being used. Please correct me if I'm wrong.
> > 
> > Also, we must check we don't sleep with irqs disabled!!! But
> anyway, this is not
> > news - we already can't sleep while holding a spinlock. Who says
> this is
> > guaranted really by the present code?

> This patch looks fairly scary.

Right, not to merge in "bugfixes only" time.

> It's protecting the device private
> data, you're removing the locking from some accesses and leaving it
> (albeit with irqs off now) on others.  It seems to me that can't be
> right.  It's either always there, or always not.

I disagree strongly but I needed time to reach this deep
understanding. LDD tells you what to do but skips this question; when
you want to convert code like ours to code like LDD's (i.e. what I
did in this patch) you need to deeply study the source and change
point of view (I recognize I'm a bit too messianic in this mail, but
I like these ideas).

The "state machine" thinking is a very deep one. Whoever said that
mutual exclusion (no two threads must act on a single object at the
same moment) means using locks (one thread waits the other finishes
its work)?

You can also return immediately instead of waiting the other thread
to finish. This solves various problems like "I need a spinlock for
exclusion vs. interrupts but also a mutex because I can sleep". I was
so astonished I want to write something on this (possibly a book or
my thesis, or both), and to apply this to the tty locking (when I'll
have time).

I could be wrong, but I trust that thanks to deep and good work by
who designed locking in the network layer, this patch is correct. And
indeed I addressed your issues below.

> You observe that open and close are protected by rtnl_lock.  I
> observe
> that uml_net_change_mtu and uml_net_set_mac are as well, in
> dev_ioctl.

Fine...

> The spinlock protecting this has to be _irqsave because the
> interrupt
> routine takes it, to protect against receiving packets on an
> interface
> that's being closed.

Yep, I must admit I don't remember verifying this one.

But it is solved; the interrupt routine has:

        if(!netif_running(dev)) // this tests __LINK_STATE_START
                return(IRQ_NONE);
_before_ anything else.

and dev_close has:
        clear_bit(__LINK_STATE_START, &dev->state);

> If that's impossible, we should prove that,
> and remove the locking from uml_net_interrupt.

That locking is there for other reasons I think: probably for
multiple irqs/tx vs rx. However there is also dev->xmit_lock (and you
can disable xmit_lock to use your own locking).

In all conflicts I could find the network layer makes sure you don't
need to lock process vs interrupt context (better, you don't have to
lock lifecycle progress against normal operations).

This is also true of char/block devices (you don't need to lock
against write/read in open/close; UBD doesn't know that but I have
unfinished patches for it), but there it's simpler: if userspace you
call close while a read is executing, thanks to refcounting (sys_read
does fget) the ->close (or ->release) is only called after the end of
->read.

On the other hand, the tty/console locking IMHO is problematic
because (to my knowledge) it does not satisfy this property (and
because you have to mix tty and console locking, and it is not easy
to design a clean solution to this).

> I can't decide about uml_net_start_xmit - there's some RCU stuff
> around one call that leads to it, but I don't see any sign of
> rtnl_lock.

It shouldn't use it - that lock is only for lifecycle.
See Documentation/networking/netdevices.txt (there is also a LWN
article on disabling xmit_lock to use custom locking).

http://lwn.net/Articles/101215/
http://lwn.net/Articles/121566/

> So, I'd say there are some changes needed here, but they're not
> entirely the ones in this patch.

Chiacchiera con i tuoi amici in tempo reale! 
 http://it.yahoo.com/mail_it/foot/*http://it.messenger.yahoo.com 

Re: [PATCH 2/3] uml: fix proc-vs-interrupt context spinlock deadlock

[Jeff Dike]

On Tue, Aug 08, 2006 at 12:59:05PM +0200, Paolo Giarrusso wrote:
> I could be wrong, but I trust that thanks to deep and good work by
> who designed locking in the network layer, this patch is correct. And
> indeed I addressed your issues below.

OK, but there will need to be comments explaining why it is OK that
this data only looks half-locked.

The locking, as it stands, looks consistent and conservative.
However, there are some places where critical sections are too big and
the locking should be narrowed.

> This is also true of char/block devices (you don't need to lock
> against write/read in open/close; UBD doesn't know that but I have
> unfinished patches for it), but there it's simpler: if userspace you
> call close while a read is executing, thanks to refcounting (sys_read
> does fget) the ->close (or ->release) is only called after the end of
> ->read.

In my current patchset, there is a per-queue lock which is mostly
managed by the block layer.

				Jeff

Re: [PATCH 2/3] uml: fix proc-vs-interrupt context spinlock deadlock

[Paolo Giarrusso]

Jeff Dike <jdike@addtoit.com> ha scritto: 

> On Tue, Aug 08, 2006 at 12:59:05PM +0200, Paolo Giarrusso wrote:
> > I could be wrong, but I trust that thanks to deep and good work
> by
> > who designed locking in the network layer, this patch is correct.
> And
> > indeed I addressed your issues below.
> 
> OK, but there will need to be comments explaining why it is OK that
> this data only looks half-locked.

Guess I'll put it in Documentation and reference it.

> The locking, as it stands, looks consistent and conservative.
Yes, it is.
> However, there are some places where critical sections are too big
> and
> the locking should be narrowed.

Yes, in particular we cannot hold a spinlock for the whole _open
since it must call sleeping functions. 

> > This is also true of char/block devices (you don't need to lock
> > against write/read in open/close; UBD doesn't know that but I
> have
> > unfinished patches for it), but there it's simpler: if userspace
> you
> > call close while a read is executing, thanks to refcounting
> (sys_read
> > does fget) the ->close (or ->release) is only called after the
> end of
> > ->read.
> 
> In my current patchset, there is a per-queue lock which is mostly
> managed by the block layer.

I'll try then to finish the patches soon and merge them; the main
problem is splitting (including the use of different locks) normal
locking from our peculiar locking of _open/_close against mconsole
changes.


Chiacchiera con i tuoi amici in tempo reale! 
 http://it.yahoo.com/mail_it/foot/*http://it.messenger.yahoo.com 

[PATCH 3/3] uml: clean our set_ether_mac

[Paolo 'Blaisorblade' Giarrusso]

From: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>

Clean set_ether_mac usage. Maybe could also be removed, but surely it can't be a
global function taking a void* argument.

You may want to defer this patch to next kernel release.

Signed-off-by: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
---

 arch/um/drivers/net_kern.c |   14 ++++++--------
 arch/um/include/net_user.h |    1 -
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/arch/um/drivers/net_kern.c b/arch/um/drivers/net_kern.c
index f5fba74..e26601a 100644
--- a/arch/um/drivers/net_kern.c
+++ b/arch/um/drivers/net_kern.c
@@ -31,6 +31,11 @@ #include "init.h"
 #include "irq_user.h"
 #include "irq_kern.h"
 
+static inline void set_ether_mac(struct net_device *dev, unsigned char *addr)
+{
+	memcpy(dev->dev_addr, addr, ETH_ALEN);
+}
+
 #define DRIVER_NAME "uml-netdev"
 
 static DEFINE_SPINLOCK(opened_lock);
@@ -234,7 +239,7 @@ static int uml_net_set_mac(struct net_de
 	struct sockaddr *hwaddr = addr;
 
 	spin_lock_irq(&lp->lock);
-	memcpy(dev->dev_addr, hwaddr->sa_data, ETH_ALEN);
+	set_ether_mac(dev, hwaddr->sa_data);
 	spin_unlock_irq(&lp->lock);
 
 	return(0);
@@ -788,13 +793,6 @@ void dev_ip_addr(void *d, unsigned char 
 	memcpy(bin_buf, &in->ifa_address, sizeof(in->ifa_address));
 }
 
-void set_ether_mac(void *d, unsigned char *addr)
-{
-	struct net_device *dev = d;
-
-	memcpy(dev->dev_addr, addr, ETH_ALEN);	
-}
-
 struct sk_buff *ether_adjust_skb(struct sk_buff *skb, int extra)
 {
 	if((skb != NULL) && (skb_tailroom(skb) < extra)){
diff --git a/arch/um/include/net_user.h b/arch/um/include/net_user.h
index 800c403..47ef7cb 100644
--- a/arch/um/include/net_user.h
+++ b/arch/um/include/net_user.h
@@ -26,7 +26,6 @@ struct net_user_info {
 
 extern void ether_user_init(void *data, void *dev);
 extern void dev_ip_addr(void *d, unsigned char *bin_buf);
-extern void set_ether_mac(void *d, unsigned char *addr);
 extern void iter_addresses(void *d, void (*cb)(unsigned char *, 
 					       unsigned char *, void *), 
 			   void *arg);

Re: [PATCH 3/3] uml: clean our set_ether_mac

[Jeff Dike]

On Sun, Aug 06, 2006 at 05:47:05PM +0200, Paolo 'Blaisorblade' Giarrusso wrote:
> From: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
> 
> Clean set_ether_mac usage. Maybe could also be removed, but surely it can't be a
> global function taking a void* argument.
> 
> You may want to defer this patch to next kernel release.
> 
> Signed-off-by: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>

This one I can go along with :-)

Acked-by: Jeff Dike <jdike@addtoit.com>

Re: [PATCH 1/3] uml: use -mcmodel=kernel for x86_64

[Jeff Dike]

On Sun, Aug 06, 2006 at 05:47:00PM +0200, Paolo 'Blaisorblade' Giarrusso wrote:
> +_extra_flags_ = -fno-builtin -m64 -mcmodel=kernel

What exactly does this do, and can you remember why you think it's needed?

				Jeff

Re: [PATCH 1/3] uml: use -mcmodel=kernel for x86_64

[Paolo Giarrusso]

Jeff Dike <jdike@addtoit.com> ha scritto: 

> On Sun, Aug 06, 2006 at 05:47:00PM +0200, Paolo 'Blaisorblade'
> Giarrusso wrote:
> > +_extra_flags_ = -fno-builtin -m64 -mcmodel=kernel

> What exactly does this do
go to "man gcc" and search mcmodel for the answer to this one.
And x86_64 uses it too, so this patch should go for 2.6.18.

>, and can you remember why you think it's
> needed?

Ok, here's my answer to the original bugreport, which is a complete
explaination - sorry for not providing the link, I have very little
time for UML this summer.

http://marc.theaimsgroup.com/?l=user-mode-linux-devel&m=115125101012707&w=2

Bye

Chiacchiera con i tuoi amici in tempo reale! 
 http://it.yahoo.com/mail_it/foot/*http://it.messenger.yahoo.com 

Re: [PATCH 1/3] uml: use -mcmodel=kernel for x86_64

[Andi Kleen]

Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it> writes:

> From: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
> 
> We have never used this flag and recently one user experienced a complaining
> warning about this (there was a symbol in the positive half of the address space
> IIRC). So fix it.

You can't use kernel cmodel in user space.  It requires running on negative
virtual addresses. I would be surprised if it worked for you.

-Andi
>

Re: [PATCH 1/3] uml: use -mcmodel=kernel for x86_64

[Paolo Giarrusso]

Andi Kleen <ak@suse.de> ha scritto: 

> Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it> writes:
> 
> > From: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
> > 
> > We have never used this flag and recently one user experienced a
> complaining
> > warning about this (there was a symbol in the positive half of
> the address space
> > IIRC). So fix it.
> 
> You can't use kernel cmodel in user space.  It requires running on
> negative
> virtual addresses. I would be surprised if it worked for you.

Argh, yes, I didn't test the patch and I didn't think to it a lot. So
what about the following bug? Should we hack our own module loader
based on x86-64's one?

Moreover, who has recently tested module loading in x86-64 uml
kernels? I don't remember doing such testing recently...

http://marc.theaimsgroup.com/?l=user-mode-linux-devel&m=115125101012707&w=2


Chiacchiera con i tuoi amici in tempo reale! 
 http://it.yahoo.com/mail_it/foot/*http://it.messenger.yahoo.com 

Re: [PATCH 1/3] uml: use -mcmodel=kernel for x86_64

[Andi Kleen]

On Tuesday 08 August 2006 16:03, Paolo Giarrusso wrote:
> Andi Kleen <ak@suse.de> ha scritto: 
> 
> > Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it> writes:
> > 
> > > From: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
> > > 
> > > We have never used this flag and recently one user experienced a
> > complaining
> > > warning about this (there was a symbol in the positive half of
> > the address space
> > > IIRC). So fix it.
> > 
> > You can't use kernel cmodel in user space.  It requires running on
> > negative
> > virtual addresses. I would be surprised if it worked for you.
> 
> Argh, yes, I didn't test the patch and I didn't think to it a lot. So
> what about the following bug? Should we hack our own module loader
> based on x86-64's one?

Add the positive relocations to the standard x86-64 loader
and send me a patch. Then you can reuse it.

That should be cleaner than forking it

-Andi

Re: [uml-devel] [PATCH 1/3] uml: use -mcmodel=kernel for x86_64

[Daniel Gryniewicz]

[-- Attachment #1: Type: text/plain, Size: 331 bytes --]

On Tue, 2006-08-08 at 16:03 +0200, Paolo Giarrusso wrote:

> Moreover, who has recently tested module loading in x86-64 uml
> kernels? I don't remember doing such testing recently...
> 

Well, modules seem to work fine here on x86_64.  I'm running 2.6.16-bs2.
I only tested the loop module, but others also load.

Daniel

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]