Re: [PATCH 2/2] net: make net and forcedeth to use kmalloc_node

Re: [PATCH 2/2] net: make net and forcedeth to use kmalloc_node

[Andrew Morton]

On Fri, 29 Jun 2007 13:30:36 -0700
Yinghai Lu <Yinghai.Lu@Sun.COM> wrote:

> [PATCH 2/2] net: make net and forcedeth to use kmalloc_node

Please copy netdev@vger.kernel.org on net patches.

This patch modifies a net driver as well as the networking core.  These
subsytems have different maintainers who run different development trees. 
Consequently I must split your patch into two and change their titles and
changelogs appropriately.

Please do this for us in future, thanks.


> diff --git a/drivers/net/forcedeth.c b/drivers/net/forcedeth.c
> index 42ba1c0..6d53b52 100644
> --- a/drivers/net/forcedeth.c
> +++ b/drivers/net/forcedeth.c
> @@ -1383,7 +1383,7 @@ static int nv_alloc_rx(struct net_device *dev)
>  		less_rx = np->last_rx.orig;
>  
>  	while (np->put_rx.orig != less_rx) {
> -		struct sk_buff *skb = dev_alloc_skb(np->rx_buf_sz + NV_RX_ALLOC_PAD);
> +		struct sk_buff *skb = dev_alloc_skb_node(np->rx_buf_sz + NV_RX_ALLOC_PAD, dev_to_node(&dev->dev));

If we're going to do this then it would probably make sense to change
dev_alloc_skb_node() so that it takes a `struct net_device *' rather than a
bare node-id.  That's a call which the net guys will need to make, which is
why we cc them on our emails.

>  		if (skb) {
>  			np->put_rx_ctx->skb = skb;
>  			np->put_rx_ctx->dma = pci_map_single(np->pci_dev,
> @@ -1415,7 +1415,7 @@ static int nv_alloc_rx_optimized(struct net_device *dev)
>  		less_rx = np->last_rx.ex;
>  
>  	while (np->put_rx.ex != less_rx) {
> -		struct sk_buff *skb = dev_alloc_skb(np->rx_buf_sz + NV_RX_ALLOC_PAD);
> +		struct sk_buff *skb = dev_alloc_skb_node(np->rx_buf_sz + NV_RX_ALLOC_PAD, dev_to_node(&dev->dev));

It might help to edit code in an 80-col xterm.

Please run scritps/checkpatch.pl across all your diffs.  Its suggestions
are by no means compulsory, but they do provide things for you to check up
and and have a think about, thanks.

I won't apply this (these) patches because I expect API changes.

Re: [PATCH 2/2] net: make net and forcedeth to use kmalloc_node

[Stephen Hemminger]

On Tue, 3 Jul 2007 11:58:40 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Fri, 29 Jun 2007 13:30:36 -0700
> Yinghai Lu <Yinghai.Lu@Sun.COM> wrote:
> 
> > [PATCH 2/2] net: make net and forcedeth to use kmalloc_node
> 
> Please copy netdev@vger.kernel.org on net patches.
> 
> This patch modifies a net driver as well as the networking core.  These
> subsytems have different maintainers who run different development trees. 
> Consequently I must split your patch into two and change their titles and
> changelogs appropriately.
> 
> Please do this for us in future, thanks.
> 
> 
> > diff --git a/drivers/net/forcedeth.c b/drivers/net/forcedeth.c
> > index 42ba1c0..6d53b52 100644
> > --- a/drivers/net/forcedeth.c
> > +++ b/drivers/net/forcedeth.c
> > @@ -1383,7 +1383,7 @@ static int nv_alloc_rx(struct net_device *dev)
> >  		less_rx = np->last_rx.orig;
> >  
> >  	while (np->put_rx.orig != less_rx) {
> > -		struct sk_buff *skb = dev_alloc_skb(np->rx_buf_sz + NV_RX_ALLOC_PAD);
> > +		struct sk_buff *skb = dev_alloc_skb_node(np->rx_buf_sz + NV_RX_ALLOC_PAD, dev_to_node(&dev->dev));
> 
> If we're going to do this then it would probably make sense to change
> dev_alloc_skb_node() so that it takes a `struct net_device *' rather than a
> bare node-id.  That's a call which the net guys will need to make, which is
> why we cc them on our emails.

The driver should use netdev_alloc_skb. That routine is supposed to be overriden
on architecture's that need per node changes.

> >  		if (skb) {
> >  			np->put_rx_ctx->skb = skb;
> >  			np->put_rx_ctx->dma = pci_map_single(np->pci_dev,
> > @@ -1415,7 +1415,7 @@ static int nv_alloc_rx_optimized(struct net_device *dev)
> >  		less_rx = np->last_rx.ex;
> >  
> >  	while (np->put_rx.ex != less_rx) {
> > -		struct sk_buff *skb = dev_alloc_skb(np->rx_buf_sz + NV_RX_ALLOC_PAD);
> > +		struct sk_buff *skb = dev_alloc_skb_node(np->rx_buf_sz + NV_RX_ALLOC_PAD, dev_to_node(&dev->dev));
> 
> It might help to edit code in an 80-col xterm.
> 
> Please run scritps/checkpatch.pl across all your diffs.  Its suggestions
> are by no means compulsory, but they do provide things for you to check up
> and and have a think about, thanks.
> 
> I won't apply this (these) patches because I expect API changes.
> 
> 
> -
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Stephen Hemminger <shemminger@linux-foundation.org>

Re: [PATCH 2/2] net: make net and forcedeth to use kmalloc_node

[Yinghai Lu]

Stephen Hemminger wrote:
> On Tue, 3 Jul 2007 11:58:40 -0700
> Andrew Morton <akpm@linux-foundation.org> wrote:
> 
>> On Fri, 29 Jun 2007 13:30:36 -0700
>> Yinghai Lu <Yinghai.Lu@Sun.COM> wrote:
>>
>>> [PATCH 2/2] net: make net and forcedeth to use kmalloc_node
>> Please copy netdev@vger.kernel.org on net patches.
>>
>> This patch modifies a net driver as well as the networking core.  These
>> subsytems have different maintainers who run different development trees. 
>> Consequently I must split your patch into two and change their titles and
>> changelogs appropriately.
>>
>> Please do this for us in future, thanks.
>>
>>
>>> diff --git a/drivers/net/forcedeth.c b/drivers/net/forcedeth.c
>>> index 42ba1c0..6d53b52 100644
>>> --- a/drivers/net/forcedeth.c
>>> +++ b/drivers/net/forcedeth.c
>>> @@ -1383,7 +1383,7 @@ static int nv_alloc_rx(struct net_device *dev)
>>>  		less_rx = np->last_rx.orig;
>>>  
>>>  	while (np->put_rx.orig != less_rx) {
>>> -		struct sk_buff *skb = dev_alloc_skb(np->rx_buf_sz + NV_RX_ALLOC_PAD);
>>> +		struct sk_buff *skb = dev_alloc_skb_node(np->rx_buf_sz + NV_RX_ALLOC_PAD, dev_to_node(&dev->dev));
>> If we're going to do this then it would probably make sense to change
>> dev_alloc_skb_node() so that it takes a `struct net_device *' rather than a
>> bare node-id.  That's a call which the net guys will need to make, which is
>> why we cc them on our emails.
> 
> The driver should use netdev_alloc_skb. That routine is supposed to be overriden
> on architecture's that need per node changes.

do you mean:
use dev_alloc_skb_node for numa arch.
and other non numa will use macro or static inline to dev_alloc_skb

YH

Re: [PATCH 2/2] net: make net and forcedeth to use kmalloc_node

[Stephen Hemminger]

On Tue, 03 Jul 2007 14:28:50 -0700
Yinghai Lu <Yinghai.Lu@Sun.COM> wrote:

> Stephen Hemminger wrote:
> > On Tue, 3 Jul 2007 11:58:40 -0700
> > Andrew Morton <akpm@linux-foundation.org> wrote:
> > 
> >> On Fri, 29 Jun 2007 13:30:36 -0700
> >> Yinghai Lu <Yinghai.Lu@Sun.COM> wrote:
> >>
> >>> [PATCH 2/2] net: make net and forcedeth to use kmalloc_node
> >> Please copy netdev@vger.kernel.org on net patches.
> >>
> >> This patch modifies a net driver as well as the networking core.  These
> >> subsytems have different maintainers who run different development trees. 
> >> Consequently I must split your patch into two and change their titles and
> >> changelogs appropriately.
> >>
> >> Please do this for us in future, thanks.
> >>
> >>
> >>> diff --git a/drivers/net/forcedeth.c b/drivers/net/forcedeth.c
> >>> index 42ba1c0..6d53b52 100644
> >>> --- a/drivers/net/forcedeth.c
> >>> +++ b/drivers/net/forcedeth.c
> >>> @@ -1383,7 +1383,7 @@ static int nv_alloc_rx(struct net_device *dev)
> >>>  		less_rx = np->last_rx.orig;
> >>>  
> >>>  	while (np->put_rx.orig != less_rx) {
> >>> -		struct sk_buff *skb = dev_alloc_skb(np->rx_buf_sz + NV_RX_ALLOC_PAD);
> >>> +		struct sk_buff *skb = dev_alloc_skb_node(np->rx_buf_sz + NV_RX_ALLOC_PAD, dev_to_node(&dev->dev));
> >> If we're going to do this then it would probably make sense to change
> >> dev_alloc_skb_node() so that it takes a `struct net_device *' rather than a
> >> bare node-id.  That's a call which the net guys will need to make, which is
> >> why we cc them on our emails.
> > 
> > The driver should use netdev_alloc_skb. That routine is supposed to be overriden
> > on architecture's that need per node changes.
> 
> do you mean:
> use dev_alloc_skb_node for numa arch.

Look at __netdev_alloc_skb() source. It does all the necessary dev_to_node()
stuff.

dev_alloc_skb is the old routine and kept around only because not all
usage has been converted yet. It should probably be deprecated at some point.


-- 
Stephen Hemminger <shemminger@linux-foundation.org>

Re: [PATCH 2/2] net: make net and forcedeth to use kmalloc_node

[Yinghai Lu]

Andrew Morton wrote:
> On Fri, 29 Jun 2007 13:30:36 -0700
> Yinghai Lu <Yinghai.Lu@Sun.COM> wrote:
> 
>> [PATCH 2/2] net: make net and forcedeth to use kmalloc_node
> 
> Please copy netdev@vger.kernel.org on net patches.
> 
> It might help to edit code in an 80-col xterm.
> 
> Please run scritps/checkpatch.pl across all your diffs.  Its suggestions
> are by no means compulsory, but they do provide things for you to check up
> and and have a think about, thanks.

I see. later.

Thanks

Yinghai Lu

[PATCH 2/4] net: use numa_node in net_devcice->dev instead of parent

[Yinghai Lu]

[PATCH 2/4] net: use numa_node in net_devcice->dev instead of parent

Signed-off-by: Yinghai Lu <yinghai.lu@sun.com>
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 27cfe5f..005cc1c 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -217,7 +217,7 @@ nodata:
 struct sk_buff *__netdev_alloc_skb(struct net_device *dev,
 		unsigned int length, gfp_t gfp_mask)
 {
-	int node = dev->dev.parent ? dev_to_node(dev->dev.parent) : -1;
+	int node = dev_to_node(&dev->dev);
 	struct sk_buff *skb;
 
 	skb = __alloc_skb(length + NET_SKB_PAD, gfp_mask, 0, node);

[PATCH 3/4] net: make forcedeth to use kmalloc_node and __netdev_alloc_skb for skb allocation

[Yinghai Lu]

[PATCH 3/4] net: make forcedeth to use kmalloc_node and __netdev_alloc_skb for skb allocation

Signed-off-by: Yinghai Lu <yinghai.lu@sun.com>

diff --git a/drivers/net/forcedeth.c b/drivers/net/forcedeth.c
index 42ba1c0..aa188f4 100644
--- a/drivers/net/forcedeth.c
+++ b/drivers/net/forcedeth.c
@@ -1383,7 +1383,8 @@ static int nv_alloc_rx(struct net_device *dev)
 		less_rx = np->last_rx.orig;
 
 	while (np->put_rx.orig != less_rx) {
-		struct sk_buff *skb = dev_alloc_skb(np->rx_buf_sz + NV_RX_ALLOC_PAD);
+		struct sk_buff *skb = __netdev_alloc_skb(dev, np->rx_buf_sz +
+						NV_RX_ALLOC_PAD, GFP_ATOMIC);
 		if (skb) {
 			np->put_rx_ctx->skb = skb;
 			np->put_rx_ctx->dma = pci_map_single(np->pci_dev,
@@ -1415,7 +1416,8 @@ static int nv_alloc_rx_optimized(struct net_device *dev)
 		less_rx = np->last_rx.ex;
 
 	while (np->put_rx.ex != less_rx) {
-		struct sk_buff *skb = dev_alloc_skb(np->rx_buf_sz + NV_RX_ALLOC_PAD);
+		struct sk_buff *skb = __netdev_alloc_skb(dev, np->rx_buf_sz +
+						NV_RX_ALLOC_PAD, GFP_ATOMIC);
 		if (skb) {
 			np->put_rx_ctx->skb = skb;
 			np->put_rx_ctx->dma = pci_map_single(np->pci_dev,
@@ -3976,8 +3978,10 @@ static int nv_set_ringparam(struct net_device *dev, struct ethtool_ringparam* ri
 					    sizeof(struct ring_desc_ex) * (ring->rx_pending + ring->tx_pending),
 					    &ring_addr);
 	}
-	rx_skbuff = kmalloc(sizeof(struct nv_skb_map) * ring->rx_pending, GFP_KERNEL);
-	tx_skbuff = kmalloc(sizeof(struct nv_skb_map) * ring->tx_pending, GFP_KERNEL);
+	rx_skbuff = kmalloc_node(sizeof(struct nv_skb_map) * ring->rx_pending,
+				 GFP_KERNEL, dev_to_node(&dev->dev));
+	tx_skbuff = kmalloc_node(sizeof(struct nv_skb_map) * ring->tx_pending,
+				 GFP_KERNEL, dev_to_node(&dev->dev));
 	if (!rxtx_ring || !rx_skbuff || !tx_skbuff) {
 		/* fall back to old rings */
 		if (np->desc_ver == DESC_VER_1 || np->desc_ver == DESC_VER_2) {
@@ -4372,7 +4376,7 @@ static int nv_loopback_test(struct net_device *dev)
 
 	/* setup packet for tx */
 	pkt_len = ETH_DATA_LEN;
-	tx_skb = dev_alloc_skb(pkt_len);
+	tx_skb = __netdev_alloc_skb(dev, pkt_len, GFP_ATOMIC);
 	if (!tx_skb) {
 		printk(KERN_ERR "dev_alloc_skb() failed during loopback test"
 			 " of %s\n", dev->name);
@@ -4976,6 +4980,8 @@ static int __devinit nv_probe(struct pci_dev *pci_dev, const struct pci_device_i
 	dev->base_addr = (unsigned long)np->base;
 
 	dev->irq = pci_dev->irq;
+	printk(KERN_INFO "nv_probe: numa_node : %02d\n",
+		dev_to_node(&pci_dev->dev));
 
 	np->rx_ring_size = RX_RING_DEFAULT;
 	np->tx_ring_size = TX_RING_DEFAULT;
@@ -4995,8 +5001,11 @@ static int __devinit nv_probe(struct pci_dev *pci_dev, const struct pci_device_i
 			goto out_unmap;
 		np->tx_ring.ex = &np->rx_ring.ex[np->rx_ring_size];
 	}
-	np->rx_skb = kmalloc(sizeof(struct nv_skb_map) * np->rx_ring_size, GFP_KERNEL);
-	np->tx_skb = kmalloc(sizeof(struct nv_skb_map) * np->tx_ring_size, GFP_KERNEL);
+	np->rx_skb = kmalloc_node(sizeof(struct nv_skb_map) * np->rx_ring_size,
+				  GFP_KERNEL, dev_to_node(&pci_dev->dev));
+	np->tx_skb = kmalloc_node(sizeof(struct nv_skb_map) * np->tx_ring_size,
+				  GFP_KERNEL, dev_to_node(&pci_dev->dev));
+
 	if (!np->rx_skb || !np->tx_skb)
 		goto out_freering;
 	memset(np->rx_skb, 0, sizeof(struct nv_skb_map) * np->rx_ring_size);

Re: [PATCH 2/2] net: make net and forcedeth to use kmalloc_node

[Christoph Hellwig]

On Tue, Jul 03, 2007 at 11:58:40AM -0700, Andrew Morton wrote:
> >  
> >  	while (np->put_rx.orig != less_rx) {
> > -		struct sk_buff *skb = dev_alloc_skb(np->rx_buf_sz + NV_RX_ALLOC_PAD);
> > +		struct sk_buff *skb = dev_alloc_skb_node(np->rx_buf_sz + NV_RX_ALLOC_PAD, dev_to_node(&dev->dev));
> 
> If we're going to do this then it would probably make sense to change
> dev_alloc_skb_node() so that it takes a `struct net_device *' rather than a
> bare node-id.  That's a call which the net guys will need to make, which is
> why we cc them on our emails.

That thing is called netdev_alloc_skb and already exists.  And it's indeed
what they should be using in the rx path.

Re: [PATCH 2/4] net: use numa_node in net_devcice->dev instead of parent

[Christoph Hellwig]

On Tue, Jul 03, 2007 at 05:30:51PM -0700, Yinghai Lu wrote:
> [PATCH 2/4] net: use numa_node in net_devcice->dev instead of parent
> 
> Signed-off-by: Yinghai Lu <yinghai.lu@sun.com>
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 27cfe5f..005cc1c 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -217,7 +217,7 @@ nodata:
>  struct sk_buff *__netdev_alloc_skb(struct net_device *dev,
>  		unsigned int length, gfp_t gfp_mask)
>  {
> -	int node = dev->dev.parent ? dev_to_node(dev->dev.parent) : -1;
> +	int node = dev_to_node(&dev->dev);

This looks wrong to me unless the device model has once again changed
silently.  When I wrote this code &dev->dev was a device allocated
as part of the netdevice and the parent is the pci (or whatever) device
that has the node information.

Re: [PATCH 2/4] net: use numa_node in net_devcice->dev instead of parent

[David Miller]

From: Christoph Hellwig <hch@infradead.org>
Date: Wed, 4 Jul 2007 09:51:20 +0100

> On Tue, Jul 03, 2007 at 05:30:51PM -0700, Yinghai Lu wrote:
> > [PATCH 2/4] net: use numa_node in net_devcice->dev instead of parent
> > 
> > Signed-off-by: Yinghai Lu <yinghai.lu@sun.com>
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index 27cfe5f..005cc1c 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -217,7 +217,7 @@ nodata:
> >  struct sk_buff *__netdev_alloc_skb(struct net_device *dev,
> >  		unsigned int length, gfp_t gfp_mask)
> >  {
> > -	int node = dev->dev.parent ? dev_to_node(dev->dev.parent) : -1;
> > +	int node = dev_to_node(&dev->dev);
> 
> This looks wrong to me unless the device model has once again changed
> silently.  When I wrote this code &dev->dev was a device allocated
> as part of the netdevice and the parent is the pci (or whatever) device
> that has the node information.

Correct, this change is completely bogus.

Re: [PATCH 2/4] net: use numa_node in net_devcice->dev instead of parent

[Stefan Richter]

David Miller wrote:
> From: Christoph Hellwig <hch@infradead.org>
> Date: Wed, 4 Jul 2007 09:51:20 +0100
> 
>> On Tue, Jul 03, 2007 at 05:30:51PM -0700, Yinghai Lu wrote:
...
>> >  struct sk_buff *__netdev_alloc_skb(struct net_device *dev,
>> >  		unsigned int length, gfp_t gfp_mask)
>> >  {
>> > -	int node = dev->dev.parent ? dev_to_node(dev->dev.parent) : -1;
>> > +	int node = dev_to_node(&dev->dev);
>> 
>> This looks wrong to me unless the device model has once again changed
>> silently.  When I wrote this code &dev->dev was a device allocated
>> as part of the netdevice and the parent is the pci (or whatever) device
>> that has the node information.
> 
> Correct, this change is completely bogus.

I've got a question from a network driver writer's perspective:  In
drivers which sit on top of a midlayer/ multi-protocol infrastructure,
should dev->dev.parent be the lowlevel's pci_dev.dev or the midlayer's
whatever_abstracted_dev.dev?

(Example:  eth1394 network driver sits on top of ieee1394 = FireWire
midlayer, this sits on top of ohci1394 = PCI driver for FireWire
controllers.)

I guess it should be the lowlevel's pci_dev.dev, unless the midlayer
cares to set
  set_dev_node(&midlayer_dev.dev, dev_to_node(midlayer_dev.dev.parent));

And either way, for full effect of NUMA awareness in the highlevel
network driver, the midlayer should be NUMA aware too...
-- 
Stefan Richter
-=====-=-=== -=== --=--
http://arcgraph.de/sr/

Re: [PATCH 2/4] net: use numa_node in net_devcice->dev instead of parent

[Yinghai Lu]

On 7/4/07, David Miller <davem@davemloft.net> wrote:
> From: Christoph Hellwig <hch@infradead.org>
> Date: Wed, 4 Jul 2007 09:51:20 +0100
>
> > On Tue, Jul 03, 2007 at 05:30:51PM -0700, Yinghai Lu wrote:
> > > [PATCH 2/4] net: use numa_node in net_devcice->dev instead of parent
> > >
> > > Signed-off-by: Yinghai Lu <yinghai.lu@sun.com>
> > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > > index 27cfe5f..005cc1c 100644
> > > --- a/net/core/skbuff.c
> > > +++ b/net/core/skbuff.c
> > > @@ -217,7 +217,7 @@ nodata:
> > >  struct sk_buff *__netdev_alloc_skb(struct net_device *dev,
> > >             unsigned int length, gfp_t gfp_mask)
> > >  {
> > > -   int node = dev->dev.parent ? dev_to_node(dev->dev.parent) : -1;
> > > +   int node = dev_to_node(&dev->dev);
> >
> > This looks wrong to me unless the device model has once again changed
> > silently.  When I wrote this code &dev->dev was a device allocated
> > as part of the netdevice and the parent is the pci (or whatever) device
> > that has the node information.
>
> Correct, this change is completely bogus.
> -

please check that with

http://lkml.org/lkml/2007/7/3/419

[PATCH 1/4] try parent numa_node at first before using default

YH

Re: [PATCH 2/2] net: make net and forcedeth to use kmalloc_node

[Yinghai Lu]

On 7/4/07, Christoph Hellwig <hch@infradead.org> wrote:
> On Tue, Jul 03, 2007 at 11:58:40AM -0700, Andrew Morton wrote:
> > >
> > >     while (np->put_rx.orig != less_rx) {
> > > -           struct sk_buff *skb = dev_alloc_skb(np->rx_buf_sz + NV_RX_ALLOC_PAD);
> > > +           struct sk_buff *skb = dev_alloc_skb_node(np->rx_buf_sz + NV_RX_ALLOC_PAD, dev_to_node(&dev->dev));
> >
> > If we're going to do this then it would probably make sense to change
> > dev_alloc_skb_node() so that it takes a `struct net_device *' rather than a
> > bare node-id.  That's a call which the net guys will need to make, which is
> > why we cc them on our emails.
>
> That thing is called netdev_alloc_skb and already exists.  And it's indeed
> what they should be using in the rx path.
>
> -
please check
http://lkml.org/lkml/2007/7/3/418

[PATCH 3/4] net: make forcedeth to use kmalloc_node and
__netdev_alloc_skb for skb allocation

but need to replace __netdev_alloc_skb to netdev_alloc_skb.

YH