Netdev List
 help / color / mirror / Atom feed
* Re: tbench regression in 2.6.25-rc1
From: Eric Dumazet @ 2008-02-19  7:35 UTC (permalink / raw)
  To: Zhang, Yanmin; +Cc: David Miller, herbert, linux-kernel, netdev
In-Reply-To: <1203389095.3248.6.camel@ymzhang>

Zhang, Yanmin a écrit :
> On Mon, 2008-02-18 at 11:11 +0100, Eric Dumazet wrote:
>> On Mon, 18 Feb 2008 16:12:38 +0800
>> "Zhang, Yanmin" <yanmin_zhang@linux.intel.com> wrote:
>>
>>> On Fri, 2008-02-15 at 15:22 -0800, David Miller wrote:
>>>> From: Eric Dumazet <dada1@cosmosbay.com>
>>>> Date: Fri, 15 Feb 2008 15:21:48 +0100
>>>>
>>>>> On linux-2.6.25-rc1 x86_64 :
>>>>>
>>>>> offsetof(struct dst_entry, lastuse)=0xb0
>>>>> offsetof(struct dst_entry, __refcnt)=0xb8
>>>>> offsetof(struct dst_entry, __use)=0xbc
>>>>> offsetof(struct dst_entry, next)=0xc0
>>>>>
>>>>> So it should be optimal... I dont know why tbench prefers __refcnt being 
>>>>> on 0xc0, since in this case lastuse will be on a different cache line...
>>>>>
>>>>> Each incoming IP packet will need to change lastuse, __refcnt and __use, 
>>>>> so keeping them in the same cache line is a win.
>>>>>
>>>>> I suspect then that even this patch could help tbench, since it avoids 
>>>>> writing lastuse...
>>>> I think your suspicions are right, and even moreso
>>>> it helps to keep __refcnt out of the same cache line
>>>> as input/output/ops which are read-almost-entirely :-
>>> I think you are right. The issue is these three variables sharing the same cache line
>>> with input/output/ops.
>>>
>>>> )
>>>>
>>>> I haven't done an exhaustive analysis, but it seems that
>>>> the write traffic to lastuse and __refcnt are about the
>>>> same.  However if we find that __refcnt gets hit more
>>>> than lastuse in this workload, it explains the regression.
>>> I also think __refcnt is the key. I did a new testing by adding 2 unsigned long
>>> pading before lastuse, so the 3 members are moved to next cache line. The performance is
>>> recovered.
>>>
>>> How about below patch? Almost all performance is recovered with the new patch.
>>>
>>> Signed-off-by: Zhang Yanmin <yanmin.zhang@intel.com>
>>>
>>> ---
>>>
>>> --- linux-2.6.25-rc1/include/net/dst.h	2008-02-21 14:33:43.000000000 +0800
>>> +++ linux-2.6.25-rc1_work/include/net/dst.h	2008-02-21 14:36:22.000000000 +0800
>>> @@ -52,11 +52,10 @@ struct dst_entry
>>>  	unsigned short		header_len;	/* more space at head required */
>>>  	unsigned short		trailer_len;	/* space to reserve at tail */
>>>  
>>> -	u32			metrics[RTAX_MAX];
>>> -	struct dst_entry	*path;
>>> -
>>> -	unsigned long		rate_last;	/* rate limiting for ICMP */
>>>  	unsigned int		rate_tokens;
>>> +	unsigned long		rate_last;	/* rate limiting for ICMP */
>>> +
>>> +	struct dst_entry	*path;
>>>  
>>>  #ifdef CONFIG_NET_CLS_ROUTE
>>>  	__u32			tclassid;
>>> @@ -70,10 +69,12 @@ struct dst_entry
>>>  	int			(*output)(struct sk_buff*);
>>>  
>>>  	struct  dst_ops	        *ops;
>>> -		
>>> -	unsigned long		lastuse;
>>> +
>>> +	u32			metrics[RTAX_MAX];
>>> +
>>>  	atomic_t		__refcnt;	/* client references	*/
>>>  	int			__use;
>>> +	unsigned long		lastuse;
>>>  	union {
>>>  		struct dst_entry *next;
>>>  		struct rtable    *rt_next;
>>>
>>>
>> Well, after this patch, we grow dst_entry by 8 bytes :
> With my .config, it doesn't grow. Perhaps because of CONFIG_NET_CLS_ROUTE, I don't
> enable it. I will move tclassid under ops.
> 
>> sizeof(struct dst_entry)=0xd0
>> offsetof(struct dst_entry, input)=0x68
>> offsetof(struct dst_entry, output)=0x70
>> offsetof(struct dst_entry, __refcnt)=0xb4
>> offsetof(struct dst_entry, lastuse)=0xc0
>> offsetof(struct dst_entry, __use)=0xb8
>> sizeof(struct rtable)=0x140
>>
>>
>> So we dirty two cache lines instead of one, unless your cpu have 128 bytes cache lines ?
>>
>> I am quite suprised that my patch to not change lastuse if already set to jiffies changes nothing...
>>
>> If you have some time, could you also test this (unrelated) patch ?
>>
>> We can avoid dirty all the time a cache line of loopback device.
>>
>> diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
>> index f2a6e71..0a4186a 100644
>> --- a/drivers/net/loopback.c
>> +++ b/drivers/net/loopback.c
>> @@ -150,7 +150,10 @@ static int loopback_xmit(struct sk_buff *skb, struct net_device *dev)
>>                 return 0;
>>         }
>>  #endif
>> -       dev->last_rx = jiffies;
>> +#ifdef CONFIG_SMP
>> +       if (dev->last_rx != jiffies)
>> +#endif
>> +               dev->last_rx = jiffies;
>>  
>>         /* it's OK to use per_cpu_ptr() because BHs are off */
>>         pcpu_lstats = netdev_priv(dev);
>>
> Although I didn't test it, I don't think it's ok. The key is __refcnt shares the same
> cache line with ops/input/output.
> 

Note it was unrelated to struct dst, but dirtying of one cache line of 
'loopback netdevice'

I tested it, and tbench result was better with this patch : 890 MB/s instead 
of 870 MB/s on a bi dual core machine.


I was curious of the potential gain on your 16 cores (4x4) machine.

^ permalink raw reply

* Re: tbench regression in 2.6.25-rc1
From: Eric Dumazet @ 2008-02-19  7:40 UTC (permalink / raw)
  To: Zhang, Yanmin
  Cc: Valdis.Kletnieks, David Miller, herbert, linux-kernel, netdev
In-Reply-To: <1203403903.3248.29.camel@ymzhang>

Zhang, Yanmin a écrit :
> On Mon, 2008-02-18 at 12:33 -0500, Valdis.Kletnieks@vt.edu wrote: 
>> On Mon, 18 Feb 2008 16:12:38 +0800, "Zhang, Yanmin" said:
>>
>>> I also think __refcnt is the key. I did a new testing by adding 2 unsigned long
>>> pading before lastuse, so the 3 members are moved to next cache line. The performance is
>>> recovered.
>>>
>>> How about below patch? Almost all performance is recovered with the new patch.
>>>
>>> Signed-off-by: Zhang Yanmin <yanmin.zhang@intel.com>
>> Could you add a comment someplace that says "refcnt wants to be on a different
>> cache line from input/output/ops or performance tanks badly", to warn some
>> future kernel hacker who starts adding new fields to the structure?
> Ok. Below is the new patch.
> 
> 1) Move tclassid under ops in case CONFIG_NET_CLS_ROUTE=y. So sizeof(dst_entry)=200
> no matter if CONFIG_NET_CLS_ROUTE=y/n. I tested many patches on my 16-core tigerton by
> moving tclassid to different place. It looks like tclassid could also have impact on
> performance.
> If moving tclassid before metrics, or just don't move tclassid, the performance isn't
> good. So I move it behind metrics.
> 
> 2) Add comments before __refcnt.
> 
> If CONFIG_NET_CLS_ROUTE=y, the result with below patch is about 18% better than
> the one without the patch.
> 
> If CONFIG_NET_CLS_ROUTE=n, the result with below patch is about 30% better than
> the one without the patch.
> 
> Signed-off-by: Zhang Yanmin <yanmin.zhang@intel.com>
> 
> ---
> 
> --- linux-2.6.25-rc1/include/net/dst.h	2008-02-21 14:33:43.000000000 +0800
> +++ linux-2.6.25-rc1_work/include/net/dst.h	2008-02-22 12:52:19.000000000 +0800
> @@ -52,15 +52,10 @@ struct dst_entry
>  	unsigned short		header_len;	/* more space at head required */
>  	unsigned short		trailer_len;	/* space to reserve at tail */
>  
> -	u32			metrics[RTAX_MAX];
> -	struct dst_entry	*path;
> -
> -	unsigned long		rate_last;	/* rate limiting for ICMP */
>  	unsigned int		rate_tokens;
> +	unsigned long		rate_last;	/* rate limiting for ICMP */
>  
> -#ifdef CONFIG_NET_CLS_ROUTE
> -	__u32			tclassid;
> -#endif
> +	struct dst_entry	*path;
>  
>  	struct neighbour	*neighbour;
>  	struct hh_cache		*hh;
> @@ -70,10 +65,20 @@ struct dst_entry
>  	int			(*output)(struct sk_buff*);
>  
>  	struct  dst_ops	        *ops;
> -		
> -	unsigned long		lastuse;
> +
> +	u32			metrics[RTAX_MAX];
> +
> +#ifdef CONFIG_NET_CLS_ROUTE
> +	__u32			tclassid;
> +#endif
> +
> +	/*
> +	 * __refcnt wants to be on a different cache line from
> +	 * input/output/ops or performance tanks badly
> +	 */
>  	atomic_t		__refcnt;	/* client references	*/
>  	int			__use;
> +	unsigned long		lastuse;
>  	union {
>  		struct dst_entry *next;
>  		struct rtable    *rt_next;
> 
> 
> 

I prefer this patch, but unfortunatly your perf numbers are for 64 bits kernels.

Could you please test now with 32 bits one ?

Thank you

^ permalink raw reply

* Re: [patch] pegasus.c
From: Petko Manolov @ 2008-02-19  8:11 UTC (permalink / raw)
  To: Jeff Garzik; +Cc: netdev
In-Reply-To: <47B06BF4.7010009@garzik.org>

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1126 bytes --]

This is another attempt on sending the pegasus patch with Alpine.  I know 
it's even worse, but i've attached gzip-ed version just in case.

Here goes a brief description of what's changed:

---
This patch is fixing a driver bug triggered when malformed string is 
passed to the 'devid' module parameter.  The expected format is:

 	"device_name:vendor_id:device_id:flags"

but it turned out people often type:

 	"somename::0"

instead of:

 	"somename:::0"

which typically ends up dereferencing null pointer.


Signed-off-by: Petko Manolov <petkan@nucleusys.com>
---


cheers,
Petko


On Mon, 11 Feb 2008, Jeff Garzik wrote:

> Petko Manolov wrote:
>>     Hi Jeff,
>> 
>> Attached you'll find a patch that is fixing a driver bug triggered when 
>> malformed string is passed to the 'devid' module parameter.  The expected 
>> format is:
>>
>>     "device_name:vendor_id:device_id:flags"
>> 
>> but it turned out people often type:
>>
>>     "somename::0"
>> 
>> instead of:
>>
>>     "somename:::0"
>
> ACK but two process problems preventing application:
>
> * patch is base64-encoded
>
> * no signed-off-by included
>
>
>

[-- Attachment #2: Type: APPLICATION/OCTET-STREAM, Size: 580 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: Type: TEXT/x-diff; name=pegasus.c.patch, Size: 1373 bytes --]

--- drivers/net/usb/pegasus.c.orig	2008-01-09 12:16:52.000000000 +0200
+++ drivers/net/usb/pegasus.c	2008-01-09 12:16:58.000000000 +0200
@@ -1461,12 +1461,24 @@ static void parse_id(char *id)
 
 	if ((token = strsep(&id, ":")) != NULL)
 		name = token;
+	else
+		goto err;
 	/* name now points to a null terminated string*/
 	if ((token = strsep(&id, ":")) != NULL)
 		vendor_id = simple_strtoul(token, NULL, 16);
+	else
+		goto err;
+
 	if ((token = strsep(&id, ":")) != NULL)
 		device_id = simple_strtoul(token, NULL, 16);
-	flags = simple_strtoul(id, NULL, 16);
+	else
+		goto err;
+
+	if (id != NULL)
+		flags = simple_strtoul(id, NULL, 16);
+	else
+		goto err;
+
 	pr_info("%s: new device %s, vendor ID 0x%04x, device ID 0x%04x, flags: 0x%x\n",
 	        driver_name, name, vendor_id, device_id, flags);
 
@@ -1476,6 +1488,7 @@ static void parse_id(char *id)
 		return;
 
 	for (i=0; usb_dev_id[i].name; i++);
+
 	usb_dev_id[i].name = name;
 	usb_dev_id[i].vendor = vendor_id;
 	usb_dev_id[i].device = device_id;
@@ -1483,6 +1496,11 @@ static void parse_id(char *id)
 	pegasus_ids[i].match_flags = USB_DEVICE_ID_MATCH_DEVICE;
 	pegasus_ids[i].idVendor = vendor_id;
 	pegasus_ids[i].idProduct = device_id;
+
+	return;
+
+err:
+	pr_info("malformed 'devid' module parameter\n");
 }
 
 static int __init pegasus_init(void)

^ permalink raw reply

* AW: Problem receiving multicast/promiscuous-mode with kernel.2.6.24
From: Reither Robert @ 2008-02-19  8:40 UTC (permalink / raw)
  To: David Stevens; +Cc: netdev

Visit AVD on prolight+sound in Frankfurt from 12.-15. March 2008 - Hall 8.0, Stand G16
________________________________________________________________________



Hi,

ok, i managed to shrink down my code to show the behaviour in small size ;-)

It shows the same effect as my app, sometimes i get all the packets after a mc_join, sometimes i get only the timeouts.
I find no predictive behaviour, i'm really desperate ...
Hope you can see the effect too .. Do u need an app sending the multicast VLAN packets ?

Robert

-----------------------------------------

#include <stdint.h>
#include <sys/select.h>
#include <errno.h>
#include <stdio.h>
#include <string.h>
#include <time.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <netdb.h>
#include <netinet/in.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <fcntl.h>
#include <arpa/inet.h>
#include <net/if.h>
#include <errno.h>
#include <netdb.h>

//#include <linux/if.h>
#include <linux/sockios.h>
#include <sys/ioctl.h>

//#define JOIN_SINGLE_IF
#undef JOIN_SINGLE_IF


#define PORT 10500

/*
** Try to receive VLAN-tagged UDP multicast packets 
** VLAN-ID:3, VLAN_PRI=6, Addr: 224.1.9.0, dstnport 10500
**
** Kernel used: 2.6.24
**
** HW: VIA Epia 5000, VIA Epia LT (VIA Rhine II and VIA VT6107)
**
** Additional network settings:
** vconfig add eth0 3
** vconfig set_egress_map eth0.3 6 6
** ifconfig eth0.3 10.0.0.1
** route add -net 224.0.0.0 netmask 240.0.0.0 dev eth0.3
**
*/

#define MAX_PAYLOAD 1000

/* 
 * Network representation of the rtp header.
 */

struct rtp_header {
#ifdef WORDS_BIGENDIAN
        uint16_t version:2;
        uint16_t padbit:1;
        uint16_t extbit:1;
        uint16_t cc:4;
        uint16_t markbit:1;
        uint16_t paytype:7;
#else
        uint16_t cc:4;
        uint16_t extbit:1;
        uint16_t padbit:1;
        uint16_t version:2;
        uint16_t paytype:7;
        uint16_t markbit:1;
#endif
        uint16_t seq_number;
        uint32_t timestamp;
        uint32_t ssrc;

	/* In fact we rely on rtp_header being not larger than 
	 * the minimum header length. So, there's no contributing
	 * sync source field here. */
};

struct rtp_packet {
	struct rtp_header header;
	unsigned char payload[MAX_PAYLOAD];
};


#ifdef WORDS_BIGENDIAN
#error Sorry this is not supported on bigendian targets.
#endif

#define BYTE0(x) ((x) & 0xff)
#define BYTE1(x) (((x) >> 8)  & 0xff)
#define BYTE2(x) (((x) >> 16)  & 0xff)
#define BYTE3(x) (((x) >> 24)  & 0xff)

static char *my_inet_ntoa (char *buf, size_t n, struct in_addr in)
{
	if (snprintf (buf, n, "%u.%u.%u.%u", BYTE0(in.s_addr), BYTE1(in.s_addr), BYTE2(in.s_addr), BYTE3(in.s_addr))  >= n) {
		return NULL;
	}
	return buf;
}

static int set_non_blocking (int fd)
{
	int flags;

	if ((flags = fcntl (fd, F_GETFL)) < 0) {
		perror ("fcntl getflags ()");
		return -1;
	}
	if (fcntl (fd, F_SETFL, flags | O_NONBLOCK) < 0) {
		perror ("fcntl setflags ()");
		return -1;
	}

	return fd;
}

int main()
{
  int fd;
  fd_set read_fds;
  struct timespec timeout;
  
  while(1)
  {
    int i, ret;

  	if ((fd = subscribe_udp (PORT)) < 0) {
  		printf("Could not register UDP port.\n");
   		return 1;
    }
    	
  
  	if (join_multicast_group (fd, "224.1.9.0") < 0)
  	{
  	  printf("Error joining multicast !\n");
  	  return 1;  
  	}

    for(i=0;i<10;i++)
    {

      FD_ZERO (&read_fds);
    	FD_SET (fd, &read_fds);
    	timeout.tv_sec = 1;
    	timeout.tv_nsec = 0;
  
  
  		ret = pselect (fd+1, &read_fds, NULL, NULL, &timeout, NULL);
  
  
  		if (ret < 0) {
  			if (errno == EINTR) {
  				continue;
  			} else {
  				printf("select failed.\n");
  				return 1;
  			}
  		}

      if (FD_ISSET(fd, &read_fds))
      {
        ssize_t len;
      	struct rtp_packet p; 
      	struct sockaddr_in from;
      	socklen_t fromlen;
      	char from_ip[20];
        
    	  len = recvfrom (fd, &p, sizeof (p), 0, (struct sockaddr*) &from, &fromlen);
      	my_inet_ntoa (from_ip, sizeof (from_ip), from.sin_addr);
    
        printf("Received data packet from %s, length %u\n", from_ip, len);
      }
      else
      {
        printf("Got timeout receiving Multicast packets !\n");  
      }
    } /* loop 10 */
  
    if (leave_multicast_group(fd, "224.1.9.0") < 0)
  	{
  	  printf("Error leaving multicast !\n");
  	  return 1;  
  	}
    close(fd);	
  }
}

int subscribe_udp (int port)
{
	int s; 

	s = create_listening_socket (port, 1);
	return s;
}

int create_listening_socket (int listen_port, int udp) 
{
	struct sockaddr_in a;
	int s;
	int yes;
	if ((s = socket (AF_INET, udp ? SOCK_DGRAM : SOCK_STREAM, 0)) < 0) {
		perror ("socket");
		return -1;
	}
	yes = 1;
	if (setsockopt (s, SOL_SOCKET, SO_REUSEADDR,
		 (char *) &yes, sizeof (yes)) < 0) {
		perror ("setsockopt reuseaddr");
		close (s);
		return -1;
	}

	memset (&a, 0, sizeof (a));

	a.sin_port = htons (listen_port);
	a.sin_family = AF_INET;
	if (bind (s, (struct sockaddr *) &a, sizeof (a)) < 0) {
		perror ("bind");
		close (s);
		return -1;
	}
	if (set_non_blocking (s) < 0) {
		close (s);
		return -1;
	}

	if (!udp) {
		listen (s, 10);
	}
	return s;
}

int join_multicast_group (int fd, const char *mc_address)
{
	struct ip_mreqn imr;

  printf("In join_mc_group: %s\n",mc_address);


  if (inet_pton(AF_INET, mc_address, &imr.imr_multiaddr) != 1) {
//old	if (inet_aton (mc_address, &imr.imr_multiaddr) == 0) {
		printf ( "join_mc:Bad IP address format: %s\n", mc_address);
		return -1;
	}

	imr.imr_address.s_addr = INADDR_ANY;

#ifdef JOIN_SINGLE_IF
	imr.imr_ifindex = if_nametoindex("eth0.3"); // 0;
//	imr.imr_ifindex = if_nametoindex("eth0"); // Testing
  if (imr.imr_ifindex == 0)
  {
		printf ("join_mc:Got no interface-number from name !\n");
		return -1;    
  }
#else
	imr.imr_ifindex = 0;
#endif

  // Check if already member
	if (setsockopt (fd, IPPROTO_IP, IP_ADD_MEMBERSHIP, &imr, sizeof (imr)) < 0)
	{
	  if (errno == EADDRINUSE) // Address already joined for IP_ADD_MEMBERSHIP
	    printf("Already joined IP_ADD_MEMBERSHIP for %s.\n", mc_address);
    else
    {
  		perror ("getsockopt ip_add_membership");
  		return -1;
	  }
	}
	return 0;
}

int leave_multicast_group(int fd, const char *mc_address)
{
	struct ip_mreqn imr;

  printf("In leave_mc_group: %s\n",mc_address);

  if (inet_pton(AF_INET, mc_address, &imr.imr_multiaddr) != 1) {
//	if (inet_aton (mc_address, &imr.imr_multiaddr) == 0) {
		printf ("leave_mc:Bad IP address format: %s\n", mc_address);
		return -1;
	}

	imr.imr_address.s_addr = INADDR_ANY;

#ifdef JOIN_SINGLE_IF
	imr.imr_ifindex = if_nametoindex("eth0.3"); // 0 VLAN_ID ist aber vernderbar !!!!!
//	imr.imr_ifindex = if_nametoindex("eth0"); // 0
  if (imr.imr_ifindex == 0)
  {
		printf ("leave_mc:Got no interface-namber from name !\n");
		return -1;    
  }
#else
	imr.imr_ifindex = 0;
#endif

	if (setsockopt (fd, IPPROTO_IP, IP_DROP_MEMBERSHIP, &imr, sizeof (imr)) < 0) {
		perror ("setsockopt ip_drop_membership");
		return -1;
	}
  return 0;
}

^ permalink raw reply

* Re: tbench regression in 2.6.25-rc1
From: Zhang, Yanmin @ 2008-02-19  8:40 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: David Miller, herbert, linux-kernel, netdev
In-Reply-To: <47BA86C8.4050207@cosmosbay.com>

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 5384 bytes --]

On Tue, 2008-02-19 at 08:35 +0100, Eric Dumazet wrote:
> Zhang, Yanmin a écrit :
> > On Mon, 2008-02-18 at 11:11 +0100, Eric Dumazet wrote:
> >> On Mon, 18 Feb 2008 16:12:38 +0800
> >> "Zhang, Yanmin" <yanmin_zhang@linux.intel.com> wrote:
> >>
> >>> On Fri, 2008-02-15 at 15:22 -0800, David Miller wrote:
> >>>> From: Eric Dumazet <dada1@cosmosbay.com>
> >>>> Date: Fri, 15 Feb 2008 15:21:48 +0100
> >>>>
> >>>>> On linux-2.6.25-rc1 x86_64 :
> >>>>>
> >>>>> offsetof(struct dst_entry, lastuse)=0xb0
> >>>>> offsetof(struct dst_entry, __refcnt)=0xb8
> >>>>> offsetof(struct dst_entry, __use)=0xbc
> >>>>> offsetof(struct dst_entry, next)=0xc0
> >>>>>
> >>>>> So it should be optimal... I dont know why tbench prefers __refcnt being 
> >>>>> on 0xc0, since in this case lastuse will be on a different cache line...
> >>>>>
> >>>>> Each incoming IP packet will need to change lastuse, __refcnt and __use, 
> >>>>> so keeping them in the same cache line is a win.
> >>>>>
> >>>>> I suspect then that even this patch could help tbench, since it avoids 
> >>>>> writing lastuse...
> >>>> I think your suspicions are right, and even moreso
> >>>> it helps to keep __refcnt out of the same cache line
> >>>> as input/output/ops which are read-almost-entirely :-
> >>> I think you are right. The issue is these three variables sharing the same cache line
> >>> with input/output/ops.
> >>>
> >>>> )
> >>>>
> >>>> I haven't done an exhaustive analysis, but it seems that
> >>>> the write traffic to lastuse and __refcnt are about the
> >>>> same.  However if we find that __refcnt gets hit more
> >>>> than lastuse in this workload, it explains the regression.
> >>> I also think __refcnt is the key. I did a new testing by adding 2 unsigned long
> >>> pading before lastuse, so the 3 members are moved to next cache line. The performance is
> >>> recovered.
> >>>
> >>> How about below patch? Almost all performance is recovered with the new patch.
> >>>
> >>> Signed-off-by: Zhang Yanmin <yanmin.zhang@intel.com>
> >>>
> >>> ---
> >>>
> >>> --- linux-2.6.25-rc1/include/net/dst.h	2008-02-21 14:33:43.000000000 +0800
> >>> +++ linux-2.6.25-rc1_work/include/net/dst.h	2008-02-21 14:36:22.000000000 +0800
> >>> @@ -52,11 +52,10 @@ struct dst_entry
> >>>  	unsigned short		header_len;	/* more space at head required */
> >>>  	unsigned short		trailer_len;	/* space to reserve at tail */
> >>>  
> >>> -	u32			metrics[RTAX_MAX];
> >>> -	struct dst_entry	*path;
> >>> -
> >>> -	unsigned long		rate_last;	/* rate limiting for ICMP */
> >>>  	unsigned int		rate_tokens;
> >>> +	unsigned long		rate_last;	/* rate limiting for ICMP */
> >>> +
> >>> +	struct dst_entry	*path;
> >>>  
> >>>  #ifdef CONFIG_NET_CLS_ROUTE
> >>>  	__u32			tclassid;
> >>> @@ -70,10 +69,12 @@ struct dst_entry
> >>>  	int			(*output)(struct sk_buff*);
> >>>  
> >>>  	struct  dst_ops	        *ops;
> >>> -		
> >>> -	unsigned long		lastuse;
> >>> +
> >>> +	u32			metrics[RTAX_MAX];
> >>> +
> >>>  	atomic_t		__refcnt;	/* client references	*/
> >>>  	int			__use;
> >>> +	unsigned long		lastuse;
> >>>  	union {
> >>>  		struct dst_entry *next;
> >>>  		struct rtable    *rt_next;
> >>>
> >>>
> >> Well, after this patch, we grow dst_entry by 8 bytes :
> > With my .config, it doesn't grow. Perhaps because of CONFIG_NET_CLS_ROUTE, I don't
> > enable it. I will move tclassid under ops.
> > 
> >> sizeof(struct dst_entry)=0xd0
> >> offsetof(struct dst_entry, input)=0x68
> >> offsetof(struct dst_entry, output)=0x70
> >> offsetof(struct dst_entry, __refcnt)=0xb4
> >> offsetof(struct dst_entry, lastuse)=0xc0
> >> offsetof(struct dst_entry, __use)=0xb8
> >> sizeof(struct rtable)=0x140
> >>
> >>
> >> So we dirty two cache lines instead of one, unless your cpu have 128 bytes cache lines ?
> >>
> >> I am quite suprised that my patch to not change lastuse if already set to jiffies changes nothing...
> >>
> >> If you have some time, could you also test this (unrelated) patch ?
> >>
> >> We can avoid dirty all the time a cache line of loopback device.
> >>
> >> diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
> >> index f2a6e71..0a4186a 100644
> >> --- a/drivers/net/loopback.c
> >> +++ b/drivers/net/loopback.c
> >> @@ -150,7 +150,10 @@ static int loopback_xmit(struct sk_buff *skb, struct net_device *dev)
> >>                 return 0;
> >>         }
> >>  #endif
> >> -       dev->last_rx = jiffies;
> >> +#ifdef CONFIG_SMP
> >> +       if (dev->last_rx != jiffies)
> >> +#endif
> >> +               dev->last_rx = jiffies;
> >>  
> >>         /* it's OK to use per_cpu_ptr() because BHs are off */
> >>         pcpu_lstats = netdev_priv(dev);
> >>
> > Although I didn't test it, I don't think it's ok. The key is __refcnt shares the same
> > cache line with ops/input/output.
> > 
> 
> Note it was unrelated to struct dst, but dirtying of one cache line of 
> 'loopback netdevice'
> 
> I tested it, and tbench result was better with this patch : 890 MB/s instead 
> of 870 MB/s on a bi dual core machine.
I tested your new patch and it doesn't help tbench.

On my 8-core stoakley machine, the regression is only 5%, but it's 30% on 16-core tigerton.
It looks like the scalability is poor.

> 
> 
> I was curious of the potential gain on your 16 cores (4x4) machine.



^ permalink raw reply

* Re: Linux 2.6.24.1 - kernel does not boot; IRQ trouble?
From: Kay Sievers @ 2008-02-19  8:47 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: Chris Rankin, Andrew Morton, linux-acpi, linux-kernel, netdev
In-Reply-To: <20080218120636.00451d46@extreme>

On Feb 18, 2008 9:06 PM, Stephen Hemminger
<shemminger@linux-foundation.org> wrote:
> On Mon, 18 Feb 2008 19:42:25 +0000 (GMT)
> Chris Rankin <rankincj@yahoo.com> wrote:
>
> > --- Stephen Hemminger <shemminger@linux-foundation.org> wrote:
> > > > > sysfs: duplicate filename 'bridge' can not be created
> > > > > WARNING: at fs/sysfs/dir.c:424 sysfs_add_one()
> > > > > Pid: 1, comm: swapper Not tainted 2.6.24.1 #1
> > > > >  [<c0105020>] show_trace_log_lvl+0x1a/0x2f
> > > > >  [<c0105990>] show_trace+0x12/0x14
> > > > >  [<c010613d>] dump_stack+0x6c/0x72
> > > > >  [<c01991bf>] sysfs_add_one+0x57/0xbc
> > > > >  [<c0199e41>] sysfs_create_link+0xc2/0x10d
> > > > >  [<c01bae9a>] pci_bus_add_devices+0xbd/0x103
> > > > >  [<c034016c>] pci_legacy_init+0x56/0xe3
> > > > >  [<c03274e1>] kernel_init+0x157/0x2c3
> > > > >  [<c0104c83>] kernel_thread_helper+0x7/0x10
> > > > >  =======================
> > > > > pci 0000:00:01.0: Error creating sysfs bridge symlink, continuing...
> > > > > sysfs: duplicate filename 'bridge' can not be created
> > > > > WARNING: at fs/sysfs/dir.c:424 sysfs_add_one()
> > > > > Pid: 1, comm: swapper Not tainted 2.6.24.1 #1
> > > > >  [<c0105020>] show_trace_log_lvl+0x1a/0x2f
> > > > >  [<c0105990>] show_trace+0x12/0x14
> > > > >  [<c010613d>] dump_stack+0x6c/0x72
> > > > >  [<c01991bf>] sysfs_add_one+0x57/0xbc
> > > > >  [<c0199e41>] sysfs_create_link+0xc2/0x10d
> > > > >  [<c01bae9a>] pci_bus_add_devices+0xbd/0x103
> > > > >  [<c01bae82>] pci_bus_add_devices+0xa5/0x103
> > > > >  [<c034016c>] pci_legacy_init+0x56/0xe3
> > > > >  [<c03274e1>] kernel_init+0x157/0x2c3
> > > > >  [<c0104c83>] kernel_thread_helper+0x7/0x10
> > > > >  =======================
> > > >
> > > > I have a vague feeling that this was fixed, perhaps in 2.6.24.x?
> > >
> > > Never heard of this, what is the initialization script that causes this?
> > > Also do you have the SYSFS_DEPRECATED option configured? that caused issues
> > > with regular network drivers.
> >
> > Yes, SYSFS_DEPRECATED is enabled. And the init scripts are from Fedora 8.
>
> There was a bug (fixed in 2.6.24) that had to do with sysfs_create_link
> and SYSFS_DEPRECATED probably there is a similar problem with directories.

Chris, could you enable CONFIG_DEBUG_KOBJECT=y, it might show what
objects try to claim the same name.

Thanks,
Kay

^ permalink raw reply

* Re: [PATCH][PPPOL2TP]: Fix SMP oops in pppol2tp driver
From: James Chapman @ 2008-02-19  9:03 UTC (permalink / raw)
  To: David Miller; +Cc: jarkao2, netdev
In-Reply-To: <20080218.202934.79548477.davem@davemloft.net>

David Miller wrote:
> From: James Chapman <jchapman@katalix.com>
> Date: Mon, 18 Feb 2008 22:09:24 +0000
> 
>> Here's a new version of the patch. The patch avoids disabling irqs
>> and fixes the sk_dst_get() usage that DaveM mentioned. But even with
>> this patch, lockdep still complains if hundreds of ppp sessions are
>> inserted into a tunnel as rapidly as possible (lockdep trace is
>> below). I can stop these errors by wrapping the call to ppp_input()
>> in pppol2tp_recv_dequeue_skb() with local_irq_save/restore. What is
>> a better fix?
> 
> Firstly, let's fix one thing at a time.  Leave the sk_dst_get()
> thing alone until we can prove that it's part of the lockdep
> traces.

In reproducing the problem, I obtained several lockdep traces that 
implicated sk_dst_get(). I changed the code to use __sk_dst_check() as 
you suggested and they went away. At that point, I was hopeful the 
locking issues were fixed. But after several minutes of 
creating/deleting hundreds of ppp sessions, lockdep dumped another 
error. It is that error that I posted yesterday.

> Next, I can't see why ppp_input() needs to be invoked with
> interrupts disabled.  There are many other things that invoke
> that in software interrupt context, such as pppoe.

I agree. I'm seeking advice on what the underlying cause is of this new 
trace.

> Please provide the lockdep traces without the ppp_input() IRQ
> disabling so this can be properly analyzed.

The trace _was_ without ppp_input IRQ disabling. The trace doesn't occur 
if I disable IRQs around the ppp_input() call. The patch I sent showed 
the changes I made before running the tests that created the new lockdep 
trace. I'm sorry this wasn't clear.


-- 
James Chapman
Katalix Systems Ltd
http://www.katalix.com
Catalysts for your Embedded Linux software development


^ permalink raw reply

* Re: [PATCH][PPPOL2TP]: Fix SMP oops in pppol2tp driver
From: James Chapman @ 2008-02-19  9:09 UTC (permalink / raw)
  To: Jarek Poplawski; +Cc: David Miller, netdev
In-Reply-To: <20080218230154.GB6373@ami.dom.local>

Jarek Poplawski wrote:
> On Mon, Feb 18, 2008 at 10:09:24PM +0000, James Chapman wrote:
>> Jarek Poplawski wrote:
>>> Hi,
>>>
>>> It seems, this nice report is still uncomplete: could you check if
>>> there could have been something more yet?
>> Unfortunately the ISP's syslog stops. But I've been able to borrow two
>> Quad Xeon boxes and have reproduced the problem.
>>
>> Here's a new version of the patch. The patch avoids disabling irqs and
>> fixes the sk_dst_get() usage that DaveM mentioned. But even with this
>> patch, lockdep still complains if hundreds of ppp sessions are inserted
>> into a tunnel as rapidly as possible (lockdep trace is below). I can
>> stop these errors by wrapping the call to ppp_input() in
>> pppol2tp_recv_dequeue_skb() with local_irq_save/restore. What is a
>> better fix?
> 
> Hmm... This is a really long report and quite a bit different from
> the previous one. I need some time for this. BTW: you sent before a
> lockdep report with hlist_lock problem. I think this could be fixed
> in some independent patch to make this all more readable. Are all
> the other changes in this current patch only because of this or
> previous lockdep report or for some other reasons (or reports) yet?

As I mentioned in my reply to davem, modifying the pppol2tp driver as 
described in the patch I sent made the original lockdep problems go away.


-- 
James Chapman
Katalix Systems Ltd
http://www.katalix.com
Catalysts for your Embedded Linux software development


^ permalink raw reply

* Re: [PATCH 6/17 net-2.6.26] [NETNS]: Default arp parameters lookup.
From: Daniel Lezcano @ 2008-02-19  9:14 UTC (permalink / raw)
  To: Denis V. Lunev; +Cc: davem, netdev, containers, devel
In-Reply-To: <1203406297-32725-6-git-send-email-den@openvz.org>

Denis V. Lunev wrote:
> Default ARP parameters should be findable regardless of the context.
> Required to make inetdev_event working.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> ---
>  net/core/neighbour.c |    4 +---
>  1 files changed, 1 insertions(+), 3 deletions(-)
> 
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index c895ad4..45ed620 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -1275,9 +1275,7 @@ static inline struct neigh_parms *lookup_neigh_params(struct neigh_table *tbl,
>  	struct neigh_parms *p;
> 
>  	for (p = &tbl->parms; p; p = p->next) {
> -		if (p->net != net)
> -			continue;
> -		if ((p->dev && p->dev->ifindex == ifindex) ||
> +		if ((p->dev && p->dev->ifindex == ifindex && p->net == net) ||
>  		    (!p->dev && !ifindex))
>  			return p;
>  	}

If the values are:
	p->dev == NULL
	ifindex == 0
	p->net != net

The parms should not be taken into account and the looping must 
continue. But with this modification it is not the case, if we specify 
parms ifindex == 0, the first parms with the dev field set to NULL will 
be taken belonging or not to the right net.

IMO the right test is:

if (p->net == net && ((p->dev && p->dev->ifindex == ifindex) || !p->dev 
&& !ifindex)))

I definitively prefer the first notation :)

 > -		if (p->net != net)
 > -			continue;


^ permalink raw reply

* Re: [PATCH 6/17 net-2.6.26] [NETNS]: Default arp parameters lookup.
From: Denis V. Lunev @ 2008-02-19  9:39 UTC (permalink / raw)
  To: Daniel Lezcano; +Cc: davem, netdev, containers, devel
In-Reply-To: <47BA9E0B.6080209@fr.ibm.com>

On Tue, 2008-02-19 at 10:14 +0100, Daniel Lezcano wrote:
> Denis V. Lunev wrote:
> > Default ARP parameters should be findable regardless of the context.
> > Required to make inetdev_event working.
> > 
> > Signed-off-by: Denis V. Lunev <den@openvz.org>
> > ---
> >  net/core/neighbour.c |    4 +---
> >  1 files changed, 1 insertions(+), 3 deletions(-)
> > 
> > diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> > index c895ad4..45ed620 100644
> > --- a/net/core/neighbour.c
> > +++ b/net/core/neighbour.c
> > @@ -1275,9 +1275,7 @@ static inline struct neigh_parms *lookup_neigh_params(struct neigh_table *tbl,
> >  	struct neigh_parms *p;
> > 
> >  	for (p = &tbl->parms; p; p = p->next) {
> > -		if (p->net != net)
> > -			continue;
> > -		if ((p->dev && p->dev->ifindex == ifindex) ||
> > +		if ((p->dev && p->dev->ifindex == ifindex && p->net == net) ||
> >  		    (!p->dev && !ifindex))
> >  			return p;
> >  	}
> 
> If the values are:
> 	p->dev == NULL
> 	ifindex == 0
> 	p->net != net
> 
> The parms should not be taken into account and the looping must 
> continue. But with this modification it is not the case, if we specify 
> parms ifindex == 0, the first parms with the dev field set to NULL will 
> be taken belonging or not to the right net.

They should be taken. In the other case inetdev_event will fail for sure
in the middle. You could check.

These are ARP defaults and I do not see a problem for now to get them.


^ permalink raw reply

* Re: [PATCH 6/17 net-2.6.26] [NETNS]: Default arp parameters lookup.
From: Daniel Lezcano @ 2008-02-19  9:51 UTC (permalink / raw)
  To: Denis V. Lunev; +Cc: davem, netdev, containers, devel
In-Reply-To: <1203413980.27296.9.camel@iris.sw.ru>

Denis V. Lunev wrote:
> On Tue, 2008-02-19 at 10:14 +0100, Daniel Lezcano wrote:
>> Denis V. Lunev wrote:
>>> Default ARP parameters should be findable regardless of the context.
>>> Required to make inetdev_event working.
>>>
>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>> ---
>>>  net/core/neighbour.c |    4 +---
>>>  1 files changed, 1 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
>>> index c895ad4..45ed620 100644
>>> --- a/net/core/neighbour.c
>>> +++ b/net/core/neighbour.c
>>> @@ -1275,9 +1275,7 @@ static inline struct neigh_parms *lookup_neigh_params(struct neigh_table *tbl,
>>>  	struct neigh_parms *p;
>>>
>>>  	for (p = &tbl->parms; p; p = p->next) {
>>> -		if (p->net != net)
>>> -			continue;
>>> -		if ((p->dev && p->dev->ifindex == ifindex) ||
>>> +		if ((p->dev && p->dev->ifindex == ifindex && p->net == net) ||
>>>  		    (!p->dev && !ifindex))
>>>  			return p;
>>>  	}
>> If the values are:
>> 	p->dev == NULL
>> 	ifindex == 0
>> 	p->net != net
>>
>> The parms should not be taken into account and the looping must 
>> continue. But with this modification it is not the case, if we specify 
>> parms ifindex == 0, the first parms with the dev field set to NULL will 
>> be taken belonging or not to the right net.
> 
> They should be taken. In the other case inetdev_event will fail for sure
> in the middle. You could check.
> 
> These are ARP defaults and I do not see a problem for now to get them.

Because there is a parms default per namespace. So several instances of 
them per nd table. That was the initial approach with Eric's patchset.


^ permalink raw reply

* Re: [PATCH 6/17 net-2.6.26] [NETNS]: Default arp parameters lookup.
From: Denis V. Lunev @ 2008-02-19 10:05 UTC (permalink / raw)
  To: Daniel Lezcano; +Cc: davem, netdev, containers, devel
In-Reply-To: <47BAA690.2010504@fr.ibm.com>

On Tue, 2008-02-19 at 10:51 +0100, Daniel Lezcano wrote:
> Denis V. Lunev wrote:
> > On Tue, 2008-02-19 at 10:14 +0100, Daniel Lezcano wrote:
> >> Denis V. Lunev wrote:
> >>> Default ARP parameters should be findable regardless of the context.
> >>> Required to make inetdev_event working.
> >>>
> >>> Signed-off-by: Denis V. Lunev <den@openvz.org>
> >>> ---
> >>>  net/core/neighbour.c |    4 +---
> >>>  1 files changed, 1 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> >>> index c895ad4..45ed620 100644
> >>> --- a/net/core/neighbour.c
> >>> +++ b/net/core/neighbour.c
> >>> @@ -1275,9 +1275,7 @@ static inline struct neigh_parms *lookup_neigh_params(struct neigh_table *tbl,
> >>>  	struct neigh_parms *p;
> >>>
> >>>  	for (p = &tbl->parms; p; p = p->next) {
> >>> -		if (p->net != net)
> >>> -			continue;
> >>> -		if ((p->dev && p->dev->ifindex == ifindex) ||
> >>> +		if ((p->dev && p->dev->ifindex == ifindex && p->net == net) ||
> >>>  		    (!p->dev && !ifindex))
> >>>  			return p;
> >>>  	}
> >> If the values are:
> >> 	p->dev == NULL
> >> 	ifindex == 0
> >> 	p->net != net
> >>
> >> The parms should not be taken into account and the looping must 
> >> continue. But with this modification it is not the case, if we specify 
> >> parms ifindex == 0, the first parms with the dev field set to NULL will 
> >> be taken belonging or not to the right net.
> > 
> > They should be taken. In the other case inetdev_event will fail for sure
> > in the middle. You could check.
> > 
> > These are ARP defaults and I do not see a problem for now to get them.
> 
> Because there is a parms default per namespace. So several instances of 
> them per nd table. That was the initial approach with Eric's patchset.
> 

These changes are not in mainstream and I do not want to touch ARP as
this is not a simple thing. In reality ARP will be needed only when
we'll have a real device inside a namespace.

Right now I prefer to have minimal set of working changes to finish IP
and upper layers.

Regards,
	Den


^ permalink raw reply

* Re: [PATCH][PPPOL2TP]: Fix SMP oops in pppol2tp driver
From: Jarek Poplawski @ 2008-02-19 10:30 UTC (permalink / raw)
  To: James Chapman; +Cc: David Miller, netdev
In-Reply-To: <47BA9B50.8040404@katalix.com>

On Tue, Feb 19, 2008 at 09:03:12AM +0000, James Chapman wrote:
> David Miller wrote:
>> From: James Chapman <jchapman@katalix.com>
>> Date: Mon, 18 Feb 2008 22:09:24 +0000
>>
>>> Here's a new version of the patch. The patch avoids disabling irqs
>>> and fixes the sk_dst_get() usage that DaveM mentioned. But even with
>>> this patch, lockdep still complains if hundreds of ppp sessions are
>>> inserted into a tunnel as rapidly as possible (lockdep trace is
>>> below). I can stop these errors by wrapping the call to ppp_input()
>>> in pppol2tp_recv_dequeue_skb() with local_irq_save/restore. What is
>>> a better fix?
>>
>> Firstly, let's fix one thing at a time.  Leave the sk_dst_get()
>> thing alone until we can prove that it's part of the lockdep
>> traces.
>
> In reproducing the problem, I obtained several lockdep traces that  
> implicated sk_dst_get().

As a matter of fact I missed just that kind information on previous
lockdep report, so if you could send them too this should be still
helpful.

...
> I agree. I'm seeking advice on what the underlying cause is of this new  
> trace.

IMHO, just like I wrote earlier, the main problem is in ppp_generic(),
especially ppp_connect_channel(), where main tx & rx locks are used.
I didn't know enough about this sk_dst_lock traces yet. I hope I could
help with this, but after these changes I need some time to figure
this out again.

Jarek P.

^ permalink raw reply

* Re: [PATCH 6/17 net-2.6.26] [NETNS]: Default arp parameters lookup.
From: Daniel Lezcano @ 2008-02-19 10:22 UTC (permalink / raw)
  To: Denis V. Lunev; +Cc: davem, netdev, containers, devel
In-Reply-To: <1203415549.27296.16.camel@iris.sw.ru>

Denis V. Lunev wrote:
> On Tue, 2008-02-19 at 10:51 +0100, Daniel Lezcano wrote:
>> Denis V. Lunev wrote:
>>> On Tue, 2008-02-19 at 10:14 +0100, Daniel Lezcano wrote:
>>>> Denis V. Lunev wrote:
>>>>> Default ARP parameters should be findable regardless of the context.
>>>>> Required to make inetdev_event working.
>>>>>
>>>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>>>> ---
>>>>>  net/core/neighbour.c |    4 +---
>>>>>  1 files changed, 1 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
>>>>> index c895ad4..45ed620 100644
>>>>> --- a/net/core/neighbour.c
>>>>> +++ b/net/core/neighbour.c
>>>>> @@ -1275,9 +1275,7 @@ static inline struct neigh_parms *lookup_neigh_params(struct neigh_table *tbl,
>>>>>  	struct neigh_parms *p;
>>>>>
>>>>>  	for (p = &tbl->parms; p; p = p->next) {
>>>>> -		if (p->net != net)
>>>>> -			continue;
>>>>> -		if ((p->dev && p->dev->ifindex == ifindex) ||
>>>>> +		if ((p->dev && p->dev->ifindex == ifindex && p->net == net) ||
>>>>>  		    (!p->dev && !ifindex))
>>>>>  			return p;
>>>>>  	}
>>>> If the values are:
>>>> 	p->dev == NULL
>>>> 	ifindex == 0
>>>> 	p->net != net
>>>>
>>>> The parms should not be taken into account and the looping must 
>>>> continue. But with this modification it is not the case, if we specify 
>>>> parms ifindex == 0, the first parms with the dev field set to NULL will 
>>>> be taken belonging or not to the right net.
>>> They should be taken. In the other case inetdev_event will fail for sure
>>> in the middle. You could check.
>>>
>>> These are ARP defaults and I do not see a problem for now to get them.
>> Because there is a parms default per namespace. So several instances of 
>> them per nd table. That was the initial approach with Eric's patchset.
>>
> 
> These changes are not in mainstream and I do not want to touch ARP as
> this is not a simple thing. In reality ARP will be needed only when
> we'll have a real device inside a namespace.
> 
> Right now I prefer to have minimal set of working changes to finish IP
> and upper layers.

core/neighbour.c is a common part between several protocols, especially 
ipv4 and ipv6. If you modify this function just to fit your need in the 
arp that will block me for ipv6 until you make parms default per 
namespace. So please, find another way to do that, perhaps just add a 
helper function.

I suggest you do parms default per namespace first, it is quite small 
and easy :)

Just let me the time to send the copy-parms-default function.

Is it ok ?

   -- Daniel



  -- Daniel

^ permalink raw reply

* Re: [PATCH][PPPOL2TP]: Fix SMP oops in pppol2tp driver
From: Jarek Poplawski @ 2008-02-19 10:36 UTC (permalink / raw)
  To: James Chapman; +Cc: David Miller, netdev
In-Reply-To: <20080219103047.GA3898@ff.dom.local>

On Tue, Feb 19, 2008 at 10:30:47AM +0000, Jarek Poplawski wrote:
...
> IMHO, just like I wrote earlier, the main problem is in ppp_generic(),

...or maybe ppp_generic.c? Whatever...

Jarek P.

^ permalink raw reply

* Re: [PATCH] net/8021q/vlan_dev.c - Use print_mac
From: Patrick McHardy @ 2008-02-19 11:48 UTC (permalink / raw)
  To: David Miller; +Cc: joe, bruno, netdev, jgarzik, linux-wireless, linville
In-Reply-To: <20080218.165036.218650084.davem@davemloft.net>

David Miller wrote:
> From: David Miller <davem@davemloft.net>
> Date: Mon, 18 Feb 2008 16:43:05 -0800 (PST)
> 
>> I think we can fix this easily by using __attribute_const_
>> on the print_mac() declaration.  Let me play with that.
> 
> Actually it seems the 'pure' attribute is more important
> here.  Although it's not semantically a perfect match,
> what we need to tell the compiler is basically that:
> 
> 1) the return value depends upon the inputs
> 2) if the input is not used, it's safe to avoid the call
> 
> and 'pure' accomplishes that without any unwanted side-effects.
> 
> I think this will not result in any unwanted over-optimization.
> Because if the inputs change in any way GCC has to emit the
> call.
> 
> Any objections?


This seems fine to me, thanks Dave.

^ permalink raw reply

* Re: [PATCH] cls_u32 u32_classify()
From: jamal @ 2008-02-19 11:54 UTC (permalink / raw)
  To: David Miller; +Cc: mahatma, mahatma, netdev
In-Reply-To: <20080218.214600.94322068.davem@davemloft.net>

On Mon, 2008-18-02 at 21:46 -0800, David Miller wrote:


> Can some u32 expert review this?

http://marc.info/?l=linux-netdev&m=120178638323045&w=2

cheers,
jamal


^ permalink raw reply

* [PATCH net-2.6.26 0/5][SYSCTL]: Make some sysctl RO in net namespaces.
From: Pavel Emelyanov @ 2008-02-19 11:54 UTC (permalink / raw)
  To: David Miller; +Cc: Linux Netdev List

Hi, David.

Some time ago, when I made the net.core.somaxconn ctl per-namespace,
you told that the approach I used to make some ctl tables read-only
in namespace was not very good and said to improve it. After looking
at other code, I decided, that many ctl variables will have to be 
read-only in namespace, so we need some generic way to do this.

So, here's the patchset, that allows to create ctl tables, that are
read-only in some namespace in general (and in some net namespace in 
particular). I tried to make it work the way not to consume extra
memory at run time.

This patchset is related to net namespaces only, but on the other hand
it affects the core sysctl engine. What is your opinion about this set:
should I send these patches (or some of them) to Andrew instead and wait 
till it appears in mainline (and sequentially in net tree) or will you
accept this one in net-2.6.26?

Thanks,
Pavel


^ permalink raw reply

* Re: [Bugme-new] [Bug 9920] New: kernel panic when using ebtables redirect target
From: Patrick McHardy @ 2008-02-19 11:56 UTC (permalink / raw)
  To: David Miller
  Cc: joonwpark81, akpm, netfilter-devel, netdev, bugme-daemon,
	mingching.tiew
In-Reply-To: <20080218.205317.62981417.davem@davemloft.net>

David Miller wrote:
> From: Joonwoo Park <joonwpark81@gmail.com>
> Date: Tue, 19 Feb 2008 11:53:24 +0900
> 
>> [PATCH] netfilter: fix incorrect use of skb_make_writable
>>
>> http://bugzilla.kernel.org/show_bug.cgi?id=9920
>> The function skb_make_writable returns true or false.
>>
>> Signed-off-by: Joonwoo Park <joonwpark81@gmail.com>
> 
> I'll let Patrick pull this in, thanks!


Applied, thanks.

^ permalink raw reply

* [PATCH net-2.6.26 1/5][SYSCTL]: Merge equal code in sysctl proc handlers.
From: Pavel Emelyanov @ 2008-02-19 11:56 UTC (permalink / raw)
  To: David Miller; +Cc: Linux Netdev List
In-Reply-To: <47BAC38F.10100@openvz.org>

The ->read and ->write callbacks act in a very similar way, so
merge these paths to reduce the number of places to patch later.

Signed-off-by: Pavel Emelyanov <xemul@openvz.org>

---
 fs/proc/proc_sysctl.c |   50 ++++++++++--------------------------------------
 1 files changed, 11 insertions(+), 39 deletions(-)

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 614c34b..5e31585 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -165,8 +165,8 @@ out:
 	return err;
 }
 
-static ssize_t proc_sys_read(struct file *filp, char __user *buf,
-				size_t count, loff_t *ppos)
+static ssize_t proc_sys_call_handler(struct file *filp, void __user *buf,
+		size_t count, loff_t *ppos, int write)
 {
 	struct dentry *dentry = filp->f_dentry;
 	struct ctl_table_header *head;
@@ -190,12 +190,12 @@ static ssize_t proc_sys_read(struct file *filp, char __user *buf,
 	 * and won't be until we finish.
 	 */
 	error = -EPERM;
-	if (sysctl_perm(table, MAY_READ))
+	if (sysctl_perm(table, write ? MAY_WRITE : MAY_READ))
 		goto out;
 
 	/* careful: calling conventions are nasty here */
 	res = count;
-	error = table->proc_handler(table, 0, filp, buf, &res, ppos);
+	error = table->proc_handler(table, write, filp, buf, &res, ppos);
 	if (!error)
 		error = res;
 out:
@@ -204,44 +204,16 @@ out:
 	return error;
 }
 
-static ssize_t proc_sys_write(struct file *filp, const char __user *buf,
+static ssize_t proc_sys_read(struct file *filp, char __user *buf,
 				size_t count, loff_t *ppos)
 {
-	struct dentry *dentry = filp->f_dentry;
-	struct ctl_table_header *head;
-	struct ctl_table *table;
-	ssize_t error;
-	size_t res;
-
-	table = do_proc_sys_lookup(dentry->d_parent, &dentry->d_name, &head);
-	/* Has the sysctl entry disappeared on us? */
-	error = -ENOENT;
-	if (!table)
-		goto out;
-
-	/* Has the sysctl entry been replaced by a directory? */
-	error = -EISDIR;
-	if (!table->proc_handler)
-		goto out;
-
-	/*
-	 * At this point we know that the sysctl was not unregistered
-	 * and won't be until we finish.
-	 */
-	error = -EPERM;
-	if (sysctl_perm(table, MAY_WRITE))
-		goto out;
-
-	/* careful: calling conventions are nasty here */
-	res = count;
-	error = table->proc_handler(table, 1, filp, (char __user *)buf,
-				    &res, ppos);
-	if (!error)
-		error = res;
-out:
-	sysctl_head_finish(head);
+	return proc_sys_call_handler(filp, (void __user *)buf, count, ppos, 0);
+}
 
-	return error;
+static ssize_t proc_sys_write(struct file *filp, const char __user *buf,
+				size_t count, loff_t *ppos)
+{
+	return proc_sys_call_handler(filp, (void __user *)buf, count, ppos, 1);
 }
 
 
-- 
1.5.3.4


^ permalink raw reply related

* [PATCH net-2.6.26 2/5][SYSCTL]: Clean sysctls from unneeded extern and forward declarations.
From: Pavel Emelyanov @ 2008-02-19 11:58 UTC (permalink / raw)
  To: David Miller; +Cc: Linux Netdev List
In-Reply-To: <47BAC38F.10100@openvz.org>

The do_sysctl_strategy can be static since it's used in 
kernel/sysctl.c only.

Besides, move it and parse_table above their callers and 
drop the forward declarations.

Signed-off-by: Pavel Emelyanov <xemul@openvz.org>

---
 include/linux/sysctl.h |    5 --
 kernel/sysctl.c        |  144 +++++++++++++++++++++++-------------------------
 2 files changed, 68 insertions(+), 81 deletions(-)

diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index 571f01d..8e50196 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -981,11 +981,6 @@ extern int do_sysctl (int __user *name, int nlen,
 		      void __user *oldval, size_t __user *oldlenp,
 		      void __user *newval, size_t newlen);
 
-extern int do_sysctl_strategy (struct ctl_table *table,
-			       int __user *name, int nlen,
-			       void __user *oldval, size_t __user *oldlenp,
-			       void __user *newval, size_t newlen);
-
 extern ctl_handler sysctl_data;
 extern ctl_handler sysctl_string;
 extern ctl_handler sysctl_intvec;
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 740e144..c224cc5 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -145,12 +145,6 @@ extern int no_unaligned_warning;
 extern int max_lock_depth;
 #endif
 
-#ifdef CONFIG_SYSCTL_SYSCALL
-static int parse_table(int __user *, int, void __user *, size_t __user *,
-		void __user *, size_t, struct ctl_table *);
-#endif
-
-
 #ifdef CONFIG_PROC_SYSCTL
 static int proc_do_cad_pid(struct ctl_table *table, int write, struct file *filp,
 		  void __user *buffer, size_t *lenp, loff_t *ppos);
@@ -1459,6 +1453,74 @@ void register_sysctl_root(struct ctl_table_root *root)
 }
 
 #ifdef CONFIG_SYSCTL_SYSCALL
+/* Perform the actual read/write of a sysctl table entry. */
+static int do_sysctl_strategy (struct ctl_table *table,
+			int __user *name, int nlen,
+			void __user *oldval, size_t __user *oldlenp,
+			void __user *newval, size_t newlen)
+{
+	int op = 0, rc;
+
+	if (oldval)
+		op |= 004;
+	if (newval) 
+		op |= 002;
+	if (sysctl_perm(table, op))
+		return -EPERM;
+
+	if (table->strategy) {
+		rc = table->strategy(table, name, nlen, oldval, oldlenp,
+				     newval, newlen);
+		if (rc < 0)
+			return rc;
+		if (rc > 0)
+			return 0;
+	}
+
+	/* If there is no strategy routine, or if the strategy returns
+	 * zero, proceed with automatic r/w */
+	if (table->data && table->maxlen) {
+		rc = sysctl_data(table, name, nlen, oldval, oldlenp,
+				 newval, newlen);
+		if (rc < 0)
+			return rc;
+	}
+	return 0;
+}
+
+static int parse_table(int __user *name, int nlen,
+		       void __user *oldval, size_t __user *oldlenp,
+		       void __user *newval, size_t newlen,
+		       struct ctl_table *table)
+{
+	int n;
+repeat:
+	if (!nlen)
+		return -ENOTDIR;
+	if (get_user(n, name))
+		return -EFAULT;
+	for ( ; table->ctl_name || table->procname; table++) {
+		if (!table->ctl_name)
+			continue;
+		if (n == table->ctl_name) {
+			int error;
+			if (table->child) {
+				if (sysctl_perm(table, 001))
+					return -EPERM;
+				name++;
+				nlen--;
+				table = table->child;
+				goto repeat;
+			}
+			error = do_sysctl_strategy(table, name, nlen,
+						   oldval, oldlenp,
+						   newval, newlen);
+			return error;
+		}
+	}
+	return -ENOTDIR;
+}
+
 int do_sysctl(int __user *name, int nlen, void __user *oldval, size_t __user *oldlenp,
 	       void __user *newval, size_t newlen)
 {
@@ -1531,76 +1593,6 @@ int sysctl_perm(struct ctl_table *table, int op)
 	return test_perm(table->mode, op);
 }
 
-#ifdef CONFIG_SYSCTL_SYSCALL
-static int parse_table(int __user *name, int nlen,
-		       void __user *oldval, size_t __user *oldlenp,
-		       void __user *newval, size_t newlen,
-		       struct ctl_table *table)
-{
-	int n;
-repeat:
-	if (!nlen)
-		return -ENOTDIR;
-	if (get_user(n, name))
-		return -EFAULT;
-	for ( ; table->ctl_name || table->procname; table++) {
-		if (!table->ctl_name)
-			continue;
-		if (n == table->ctl_name) {
-			int error;
-			if (table->child) {
-				if (sysctl_perm(table, 001))
-					return -EPERM;
-				name++;
-				nlen--;
-				table = table->child;
-				goto repeat;
-			}
-			error = do_sysctl_strategy(table, name, nlen,
-						   oldval, oldlenp,
-						   newval, newlen);
-			return error;
-		}
-	}
-	return -ENOTDIR;
-}
-
-/* Perform the actual read/write of a sysctl table entry. */
-int do_sysctl_strategy (struct ctl_table *table,
-			int __user *name, int nlen,
-			void __user *oldval, size_t __user *oldlenp,
-			void __user *newval, size_t newlen)
-{
-	int op = 0, rc;
-
-	if (oldval)
-		op |= 004;
-	if (newval) 
-		op |= 002;
-	if (sysctl_perm(table, op))
-		return -EPERM;
-
-	if (table->strategy) {
-		rc = table->strategy(table, name, nlen, oldval, oldlenp,
-				     newval, newlen);
-		if (rc < 0)
-			return rc;
-		if (rc > 0)
-			return 0;
-	}
-
-	/* If there is no strategy routine, or if the strategy returns
-	 * zero, proceed with automatic r/w */
-	if (table->data && table->maxlen) {
-		rc = sysctl_data(table, name, nlen, oldval, oldlenp,
-				 newval, newlen);
-		if (rc < 0)
-			return rc;
-	}
-	return 0;
-}
-#endif /* CONFIG_SYSCTL_SYSCALL */
-
 static void sysctl_set_parent(struct ctl_table *parent, struct ctl_table *table)
 {
 	for (; table->ctl_name || table->procname; table++) {
-- 
1.5.3.4


^ permalink raw reply related

* [PATCH net-2.6.26 3/5][SYSCTL]: Add the ->permissions callback on the ctl_table_root.
From: Pavel Emelyanov @ 2008-02-19 12:00 UTC (permalink / raw)
  To: David Miller; +Cc: Linux Netdev List
In-Reply-To: <47BAC38F.10100@openvz.org>

When the table came from some other root, this root may affect
the table's permissions, depending on who is working with the
table.

The core hunk is at the bottom of this patch. All the rest is
just pushing the ctl_table_root argument up to the sysctl_perm
function.

This will be mostly (only?) used in the net sysctls.

Signed-off-by: Pavel Emelyanov <xemul@openvz.org>

---
 fs/proc/proc_sysctl.c  |    4 ++--
 include/linux/sysctl.h |    7 ++++++-
 kernel/sysctl.c        |   25 ++++++++++++++++++-------
 3 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 5e31585..5acc001 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -190,7 +190,7 @@ static ssize_t proc_sys_call_handler(struct file *filp, void __user *buf,
 	 * and won't be until we finish.
 	 */
 	error = -EPERM;
-	if (sysctl_perm(table, write ? MAY_WRITE : MAY_READ))
+	if (sysctl_perm(head->root, table, write ? MAY_WRITE : MAY_READ))
 		goto out;
 
 	/* careful: calling conventions are nasty here */
@@ -388,7 +388,7 @@ static int proc_sys_permission(struct inode *inode, int mask, struct nameidata *
 		goto out;
 
 	/* Use the permissions on the sysctl table entry */
-	error = sysctl_perm(table, mask);
+	error = sysctl_perm(head->root, table, mask);
 out:
 	sysctl_head_finish(head);
 	return error;
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index 8e50196..3239561 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -945,11 +945,14 @@ enum
 /* For the /proc/sys support */
 struct ctl_table;
 struct nsproxy;
+struct ctl_table_root;
+
 extern struct ctl_table_header *sysctl_head_next(struct ctl_table_header *prev);
 extern struct ctl_table_header *__sysctl_head_next(struct nsproxy *namespaces,
 						struct ctl_table_header *prev);
 extern void sysctl_head_finish(struct ctl_table_header *prev);
-extern int sysctl_perm(struct ctl_table *table, int op);
+extern int sysctl_perm(struct ctl_table_root *root,
+		struct ctl_table *table, int op);
 
 typedef struct ctl_table ctl_table;
 
@@ -1049,6 +1052,8 @@ struct ctl_table_root {
 	struct list_head header_list;
 	struct list_head *(*lookup)(struct ctl_table_root *root,
 					   struct nsproxy *namespaces);
+	int (*permissions)(struct ctl_table_root *root,
+			struct nsproxy *namespaces, struct ctl_table *table);
 };
 
 /* struct ctl_table_header is used to maintain dynamic lists of
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index c224cc5..8b8b582 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1454,7 +1454,8 @@ void register_sysctl_root(struct ctl_table_root *root)
 
 #ifdef CONFIG_SYSCTL_SYSCALL
 /* Perform the actual read/write of a sysctl table entry. */
-static int do_sysctl_strategy (struct ctl_table *table,
+static int do_sysctl_strategy (struct ctl_table_root *root,
+			struct ctl_table *table,
 			int __user *name, int nlen,
 			void __user *oldval, size_t __user *oldlenp,
 			void __user *newval, size_t newlen)
@@ -1465,7 +1466,7 @@ static int do_sysctl_strategy (struct ctl_table *table,
 		op |= 004;
 	if (newval) 
 		op |= 002;
-	if (sysctl_perm(table, op))
+	if (sysctl_perm(root, table, op))
 		return -EPERM;
 
 	if (table->strategy) {
@@ -1491,6 +1492,7 @@ static int do_sysctl_strategy (struct ctl_table *table,
 static int parse_table(int __user *name, int nlen,
 		       void __user *oldval, size_t __user *oldlenp,
 		       void __user *newval, size_t newlen,
+		       struct ctl_table_root *root,
 		       struct ctl_table *table)
 {
 	int n;
@@ -1505,14 +1507,14 @@ repeat:
 		if (n == table->ctl_name) {
 			int error;
 			if (table->child) {
-				if (sysctl_perm(table, 001))
+				if (sysctl_perm(root, table, 001))
 					return -EPERM;
 				name++;
 				nlen--;
 				table = table->child;
 				goto repeat;
 			}
-			error = do_sysctl_strategy(table, name, nlen,
+			error = do_sysctl_strategy(root, table, name, nlen,
 						   oldval, oldlenp,
 						   newval, newlen);
 			return error;
@@ -1538,7 +1540,8 @@ int do_sysctl(int __user *name, int nlen, void __user *oldval, size_t __user *ol
 	for (head = sysctl_head_next(NULL); head;
 			head = sysctl_head_next(head)) {
 		error = parse_table(name, nlen, oldval, oldlenp, 
-					newval, newlen, head->ctl_table);
+					newval, newlen,
+					head->root, head->ctl_table);
 		if (error != -ENOTDIR) {
 			sysctl_head_finish(head);
 			break;
@@ -1584,13 +1587,21 @@ static int test_perm(int mode, int op)
 	return -EACCES;
 }
 
-int sysctl_perm(struct ctl_table *table, int op)
+int sysctl_perm(struct ctl_table_root *root, struct ctl_table *table, int op)
 {
 	int error;
+	int mode;
+
 	error = security_sysctl(table, op);
 	if (error)
 		return error;
-	return test_perm(table->mode, op);
+
+	if (root->permissions)
+		mode = root->permissions(root, current->nsproxy, table);
+	else
+		mode = table->mode;
+
+	return test_perm(mode, op);
 }
 
 static void sysctl_set_parent(struct ctl_table *parent, struct ctl_table *table)
-- 
1.5.3.4


^ permalink raw reply related

* [PATCH net-2.6.26 4/5][SYSCTL]: Create the net sysctl root for RO tables.
From: Pavel Emelyanov @ 2008-02-19 12:02 UTC (permalink / raw)
  To: David Miller; +Cc: Linux Netdev List
In-Reply-To: <47BAC38F.10100@openvz.org>

This root keeps ctl tables in one global list, but doesn't allow
for non-init namespaces to write into tables, stored in it.

Signed-off-by: Pavel Emelyanov <xemul@openvz.org>

---
 include/net/net_namespace.h |    2 ++
 net/sysctl_net.c            |   33 +++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 0 deletions(-)

diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
index 28738b7..2930ae3 100644
--- a/include/net/net_namespace.h
+++ b/include/net/net_namespace.h
@@ -173,6 +173,8 @@ struct ctl_table;
 struct ctl_table_header;
 extern struct ctl_table_header *register_net_sysctl_table(struct net *net,
 	const struct ctl_path *path, struct ctl_table *table);
+extern struct ctl_table_header *register_init_net_ctl_table(
+		struct ctl_path *path, struct ctl_table *table);
 extern void unregister_net_sysctl_table(struct ctl_table_header *header);
 
 #endif /* __NET_NET_NAMESPACE_H */
diff --git a/net/sysctl_net.c b/net/sysctl_net.c
index 665e856..42c99e6 100644
--- a/net/sysctl_net.c
+++ b/net/sysctl_net.c
@@ -40,6 +40,30 @@ static struct ctl_table_root net_sysctl_root = {
 	.lookup = net_ctl_header_lookup,
 };
 
+static LIST_HEAD(net_ro_headers);
+
+static struct list_head *net_ctl_ro_header_lookup(struct ctl_table_root *root,
+		struct nsproxy *namespaces)
+{
+	return &net_ro_headers;
+}
+
+static int net_ctl_ro_permissions(struct ctl_table_root *root,
+		struct nsproxy *ns, struct ctl_table *table)
+{
+	int mode;
+
+	mode = table->mode;
+	if (ns->net_ns != &init_net)
+		mode &= ~0222;
+	return mode;
+}
+
+static struct ctl_table_root net_sysctl_ro_root = {
+	.lookup = net_ctl_ro_header_lookup,
+	.permissions = net_ctl_ro_permissions,
+};
+
 static int sysctl_net_init(struct net *net)
 {
 	INIT_LIST_HEAD(&net->sysctl_table_headers);
@@ -64,6 +88,7 @@ static __init int sysctl_init(void)
 	if (ret)
 		goto out;
 	register_sysctl_root(&net_sysctl_root);
+	register_sysctl_root(&net_sysctl_ro_root);
 out:
 	return ret;
 }
@@ -80,6 +105,14 @@ struct ctl_table_header *register_net_sysctl_table(struct net *net,
 }
 EXPORT_SYMBOL_GPL(register_net_sysctl_table);
 
+struct ctl_table_header *register_init_net_ctl_table(struct ctl_path *path,
+		struct ctl_table *table)
+{
+	return __register_sysctl_paths(&net_sysctl_ro_root,
+			&init_nsproxy, path, table);
+}
+EXPORT_SYMBOL_GPL(register_net_ro_ctl_table);
+
 void unregister_net_sysctl_table(struct ctl_table_header *header)
 {
 	return unregister_sysctl_table(header);
-- 
1.5.3.4


^ permalink raw reply related

* [PATCH net-2.6.26 5/5][SYSCTL]: Move some net.core sysctls to RO root.
From: Pavel Emelyanov @ 2008-02-19 12:05 UTC (permalink / raw)
  To: David Miller; +Cc: Linux Netdev List
In-Reply-To: <47BAC38F.10100@openvz.org>

There are many tables in net/core/sysctl_net_core.c that are
to be read-only. Current implementation duplicates this array
for each namespace just to clear the "write" bits in the
permissions mask.

Keep the writable tables to per-net ctl root and move the others
to the read-only one. This saves some memory in run time and
removes the... ugly code, that prepared the tables.

Signed-off-by: Pavel Emelyanov <xemul@openvz.org>

---
 net/core/sysctl_net_core.c |   35 +++++++++++++++++------------------
 1 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/net/core/sysctl_net_core.c b/net/core/sysctl_net_core.c
index 130338f..4e530ce 100644
--- a/net/core/sysctl_net_core.c
+++ b/net/core/sysctl_net_core.c
@@ -125,14 +125,6 @@ static struct ctl_table net_core_table[] = {
 #endif /* CONFIG_XFRM */
 #endif /* CONFIG_NET */
 	{
-		.ctl_name	= NET_CORE_SOMAXCONN,
-		.procname	= "somaxconn",
-		.data		= &init_net.sysctl_somaxconn,
-		.maxlen		= sizeof(int),
-		.mode		= 0644,
-		.proc_handler	= &proc_dointvec
-	},
-	{
 		.ctl_name	= NET_CORE_BUDGET,
 		.procname	= "netdev_budget",
 		.data		= &netdev_budget,
@@ -151,6 +143,18 @@ static struct ctl_table net_core_table[] = {
 	{ .ctl_name = 0 }
 };
 
+static struct ctl_table netns_core_table[] = {
+	{
+		.ctl_name	= NET_CORE_SOMAXCONN,
+		.procname	= "somaxconn",
+		.data		= &init_net.sysctl_somaxconn,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= &proc_dointvec
+	},
+	{ .ctl_name = 0 }
+};
+
 static __net_initdata struct ctl_path net_core_path[] = {
 	{ .procname = "net", .ctl_name = CTL_NET, },
 	{ .procname = "core", .ctl_name = NET_CORE, },
@@ -159,23 +163,17 @@ static __net_initdata struct ctl_path net_core_path[] = {
 
 static __net_init int sysctl_core_net_init(struct net *net)
 {
-	struct ctl_table *tbl, *tmp;
+	struct ctl_table *tbl;
 
 	net->sysctl_somaxconn = SOMAXCONN;
 
-	tbl = net_core_table;
+	tbl = netns_core_table;
 	if (net != &init_net) {
-		tbl = kmemdup(tbl, sizeof(net_core_table), GFP_KERNEL);
+		tbl = kmemdup(tbl, sizeof(netns_core_table), GFP_KERNEL);
 		if (tbl == NULL)
 			goto err_dup;
 
-		for (tmp = tbl; tmp->procname; tmp++) {
-			if (tmp->data >= (void *)&init_net &&
-					tmp->data < (void *)(&init_net + 1))
-				tmp->data += (char *)net - (char *)&init_net;
-			else
-				tmp->mode &= ~0222;
-		}
+		tbl[0].data = &net->sysctl_somaxconn;
 	}
 
 	net->sysctl_core_hdr = register_net_sysctl_table(net,
@@ -209,6 +207,7 @@ static __net_initdata struct pernet_operations sysctl_core_ops = {
 
 static __init int sysctl_core_init(void)
 {
+	register_init_net_ctl_table(net_core_path, net_core_table);
 	return register_pernet_subsys(&sysctl_core_ops);
 }
 
-- 
1.5.3.4


^ permalink raw reply related

* On the Top of the World~『Textile Industry』
From: 『Taiwan News Express』 @ 2008-02-19 11:48 UTC (permalink / raw)
  To: netdev



 <http://www.taiwannews.com.tw/static/express/080219-01.pdf>
<http://www.taiwannews.com.tw/static/express/080219-02.pdf>
<http://www.taiwannews.com.tw/static/express/080219-03.pdf>
<http://www.taiwannews.com.tw/static/express/080219-04.pdf>



 


--
Powered by PHPlist, www.phplist.com --




^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox