Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH 3/4] [NEIGH] neighbour table configuration and statistics via rtnetlink
From: jamal @ 2005-05-27 15:56 UTC (permalink / raw)
  To: Thomas Graf; +Cc: David S. Miller, netdev
In-Reply-To: <20050527151608.GZ15391@postel.suug.ch>

On Fri, 2005-27-05 at 17:16 +0200, Thomas Graf wrote:
> * jamal <1117205822.6383.68.camel@localhost.localdomain> 2005-05-27 10:57
> > The thresholds etc are there - just not netlink accessible. Example, the
> > default values for V4 are settable or queriable via:
> > 
> > /proc/sys/net/ipv4/neigh/default/gc_thresh3
> > /proc/sys/net/ipv4/neigh/default/gc_thresh2
> > /proc/sys/net/ipv4/neigh/default/gc_thresh1
> > /proc/sys/net/ipv4/neigh/default/gc_interval
> 
> Sorry but this is just one big hack:

It maybe in the user space representation, but not in the kernel 
abstraction which is what i was refering to.
In other words look at struct in_device;

> a) they do not belong there, the first implication a user does when
>    he sees default/, dev1/, dev2/, devN is that default/ covers
>    the same set of parameters as devX/. Therefore he expects all
>    parameters in default/ to be also in devX/.
> 

The deafult can be overriden by devX. So they dont need to sync.
But this is a separate topic

> b)	struct neigh_parms	parms;
> 	/* HACK. gc_* shoul follow parms without a gap! */
> 	int			gc_interval;
> 	int			gc_thresh1;
> 	int			gc_thresh2;
> 	int			gc_thresh3;
> 
> ...
> 
> 	if (dev) {
> 		dev_name_source = dev->name;
> 		t->neigh_dev[0].ctl_name = dev->ifindex;
> 		t->neigh_vars[12].procname = NULL; <- gc_interval
> 		t->neigh_vars[13].procname = NULL; <- gc_thresh1
> 		t->neigh_vars[14].procname = NULL; <- gc_thresh2
> 		t->neigh_vars[15].procname = NULL; <- gc_thresh3
> 	} else {
>  		dev_name_source = t->neigh_dev[0].procname;
> 		t->neigh_vars[12].data = (int *)(p + 1);
> 		t->neigh_vars[13].data = (int *)(p + 1) + 1;
> 		t->neigh_vars[14].data = (int *)(p + 1) + 2;
> 		t->neigh_vars[15].data = (int *)(p + 1) + 3;
> 	}
> 
>   I will not push along these hacks into my code ;->
> 

I am afraid you are looking at this from the wrong angle (user space),
Thomas ;-> 
The abstraction in the kernel is proper.

To redraw that model again with the exact structure names:

netdevice ->
  L2 config stuff
  config stuff
  more config stuff
  ..
  ..
  netdevice protocol config:
   -> v4 specific (struct in_device)
   ----> IPV4 addresses (ifa_list), ARP params(arp_parms),etc
   -> v6 specific (struct inet6_dev)
   ----> IPV6 addresses(addr_list), ndisc params (nd_parms), etc
 
There are a few more items but i am leaving them out for brevity.
I hope it makes more sense now.

> Some more notes on the devconfig idea: I'm not sure if you are still
> implying devconfig as devinet but if so: although we only have neighbour
> tables on devinets so far there might be neighbour table on other
> interfaces for whatever reason. 

in_devxxx types; sorry, I created the confusion by mentioning devconf in
the sweep.

> A protocol might allocate a neighbour
> parameter set on any netdevice which makes me think that the netlink
> neighbour code should be open for this as well. It depends on how we
> do the devconfig thing, so what I suggest it so leave this as-is for
> the moment and make a decision once we know more about how the devconfig
> thing will look like. Reasonable?

Take a look at what i said above and see if it makes sense. 
Note, I am not objecting to the patch like i said - I think some of
those items are must put in that patch. Whatever simplifies life for
you. I am not exactly a purist but at times i like getting it right from
the beggining.

cheers,
jamal

^ permalink raw reply

* Re: [PATCH] baycom_epp.c: Fix compilation with gcc-4.1.0-20050522
From: Stephen Hemminger @ 2005-05-27 15:53 UTC (permalink / raw)
  To: Alexey Dobriyan; +Cc: netdev, Thomas Sailer
In-Reply-To: <200505271808.32032.adobriyan@gmail.com>

On Fri, 27 May 2005 18:08:31 +0400
Alexey Dobriyan <adobriyan@gmail.com> wrote:

> drivers/net/hamradio/baycom_epp.c:432: error: jump into statement expression
> 	...
> 
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> 
> --- linux-vanilla/drivers/net/hamradio/baycom_epp.c	2005-05-27 12:58:48.000000000 +0400
> +++ linux-gcc4/drivers/net/hamradio/baycom_epp.c	2005-05-27 17:54:14.000000000 +0400
> @@ -380,22 +380,18 @@ static inline void do_kiss_params(struct
>   */
>  
>  #define ENCODEITERA(j)                         \
> -({                                             \
>          if (!(notbitstream & (0x1f0 << j)))    \
>                  goto stuff##j;                 \
> -  encodeend##j:    	;                      \
> -})
> +  encodeend##j:    	;

Ick... Please just kill this kind of macro crap

^ permalink raw reply

* Re: Network driver test suite (part 2)
From: Stephen Hemminger @ 2005-05-27 15:52 UTC (permalink / raw)
  To: Michael Renzmann; +Cc: netdev
In-Reply-To: <4296BACF.7030202@otaku42.de>

On Fri, 27 May 2005 08:14:39 +0200
Michael Renzmann <netdev@nospam.otaku42.de> wrote:

> Hi all.
> 
> I vaguely remembered there was a discussion about a test suite which 
> allows to check whether a network driver works as intended. After 
> digging my local message archives I found the thread with the above 
> title, but it seems the thread ceased without any result (apart from a 
> wishlist for such a test suite).
> 
> Hence my question: is there such a test suite available now? If not, did 
> anyone at least start working on such a project? What do you others 
> usually use to test-drive a network driver?

I know of know such suite, an engineer at OSDL had started pulling pieces and
ideas together, but unfortunately he was laid off. Let me know what you find out.

^ permalink raw reply

* Re: RFC: NAPI packet weighting patch
From: Stephen Hemminger @ 2005-05-27 15:50 UTC (permalink / raw)
  To: Mitch Williams; +Cc: netdev, john.ronciak, ganesh.venkatesan, jesse.brandeburg
In-Reply-To: <Pine.CYG.4.58.0505261406210.2364@mawilli1-desk2.amr.corp.intel.com>

On Thu, 26 May 2005 14:36:22 -0700
Mitch Williams <mitch.a.williams@intel.com> wrote:

> The following patch (which applies to 2.6.12rc4) adds a new sysctl
> parameter called 'netdev_packet_weight'.  This parameter controls how many
> backlog work units each RX packet is worth.
> 
> With the parameter set to 0 (the default), NAPI polling works exactly as
> it does today:  each packet is worth one backlog work unit, and the
> maximum number of received packets that will be processed in any given
> softirq is controlled by the 'netdev_max_backlog' parameter.
> 
> By setting the netdev_packet_weight to a nonzero value, we make each
> packet worth more than one backlog work unit.  Since it's a shift value, a
> setting of 1 makes each packet worth 2 work units, a setting of 2 makes
> each packet worth 4 units, etc.  Under normal circumstances you would
> never use a value higher than 3, though 4 might work for Gigabit and 10
> Gigabit networks.
> 
> By increasing the packet weight, we accomplish two things:  first, we
> cause the individual NAPI RX loops in each driver to process fewer
> packets.  This means that they will free up RX resources to the hardware
> more often, which reduces the possibility of dropped packets.  Second, it
> shortens the total time spent in the NAPI softirq, which can free the CPU
> to handle other tasks more often, thus reducing overall latency.

Rather than weighting each packet differently, why not just reduce the upper
bound on number of packets. There are several patches is in my 2.6.12-rc5-tcp3
that make this easier:
----
http://developer.osdl.org/shemminger/patches/2.6.12-rc5-tcp3/patches/bigger-backlog.patch
Separate out the two uses of netdev_max_backlog. One controls the upper
bound on packets processed per softirq, the new name for this is netdev_max_weight;
the other controls the limit on packets queued via netif_rx

Signed-off-by: Stephen Hemminger <shemminger@osdl.org>


Index: linux-2.6.12-rc4-tcp2/net/core/sysctl_net_core.c
===================================================================
--- linux-2.6.12-rc4-tcp2.orig/net/core/sysctl_net_core.c
+++ linux-2.6.12-rc4-tcp2/net/core/sysctl_net_core.c
@@ -13,6 +13,7 @@
 #ifdef CONFIG_SYSCTL
 
 extern int netdev_max_backlog;
+extern int netdev_max_weight;
 extern int weight_p;
 extern int net_msg_cost;
 extern int net_msg_burst;
@@ -137,6 +138,14 @@ ctl_table core_table[] = {
 		.mode		= 0644,
 		.proc_handler	= &proc_dointvec
 	},
+	{
+		.ctl_name	= NET_CORE_MAX_WEIGHT,
+		.procname	= "netdev_max_weight",
+		.data		= &netdev_max_weight,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= &proc_dointvec
+	},
 	{ .ctl_name = 0 }
 };
 
Index: linux-2.6.12-rc4-tcp2/net/core/dev.c
===================================================================
--- linux-2.6.12-rc4-tcp2.orig/net/core/dev.c
+++ linux-2.6.12-rc4-tcp2/net/core/dev.c
@@ -1334,7 +1334,8 @@ out:
 			Receiver routines
   =======================================================================*/
 
-int netdev_max_backlog = 300;
+int netdev_max_backlog = 10000;
+int netdev_max_weight  = 500;
 int weight_p = 64;            /* old backlog weight */
 
 DEFINE_PER_CPU(struct netif_rx_stats, netdev_rx_stat) = { 0, };
@@ -1682,8 +1683,7 @@ static void net_rx_action(struct softirq
 {
 	struct softnet_data *queue = &__get_cpu_var(softnet_data);
 	unsigned long start_time = jiffies;
-	int budget = netdev_max_backlog;
-
+	int budget = netdev_max_weight;
 	
 	local_irq_disable();
 
Index: linux-2.6.12-rc4-tcp2/include/linux/sysctl.h
===================================================================
--- linux-2.6.12-rc4-tcp2.orig/include/linux/sysctl.h
+++ linux-2.6.12-rc4-tcp2/include/linux/sysctl.h
@@ -242,6 +242,7 @@ enum
 	NET_CORE_MOD_CONG=16,
 	NET_CORE_DEV_WEIGHT=17,
 	NET_CORE_SOMAXCONN=18,
+	NET_CORE_MAX_WEIGHT=19,
 };
 
 /* /proc/sys/net/ethernet */

----
http://developer.osdl.org/shemminger/patches/2.6.12-rc5-tcp3/patches/fix-weightp.patch
Changing the dev_weight sysctl parameter has no effect because the weight
of the backlog devices is set during initialization and never changed.
Fix this by propogating changes.

Signed-off-by: Stephen Hemminger <shemminger@osdl.org>


Index: linux-2.6.12-rc4-tcp2/net/core/dev.c
===================================================================
--- linux-2.6.12-rc4-tcp2.orig/net/core/dev.c
+++ linux-2.6.12-rc4-tcp2/net/core/dev.c
@@ -1636,6 +1636,7 @@ static int process_backlog(struct net_de
 	struct softnet_data *queue = &__get_cpu_var(softnet_data);
 	unsigned long start_time = jiffies;
 
+	backlog_dev->weight = weight_p;
 	for (;;) {
 		struct sk_buff *skb;
 		struct net_device *dev;

^ permalink raw reply

* Re: PATCH: rtnetlink explicit flags setting
From: Thomas Graf @ 2005-05-27 15:19 UTC (permalink / raw)
  To: jamal; +Cc: David S. Miller, netdev
In-Reply-To: <1117206091.6383.73.camel@localhost.localdomain>

* jamal <1117206091.6383.73.camel@localhost.localdomain> 2005-05-27 11:01
> Dave please apply the patch i posted into your tree and then we can
> follow with this other stuff. 
> Actually Thomas, you can probably apply my patch on your side and then
> the other changes to use _NEW() on top of it. If you dont have time i
> could do it tommorow.

Sure, I can come up with a patchset today which will include:
 o the new NLMSG_NEW macro
 o your patch adopted to use the new NLMSG_NEW macro
 o small patch to rename NLMSG_PUT_ANSWER usages into NLMSG_NEW_ANSWER
 o a small fallout from the new neighbour table code which gcc didn't
   warn me about when it should.

OK?

^ permalink raw reply

* Re: [PATCH 3/4] [NEIGH] neighbour table configuration and statistics via rtnetlink
From: Thomas Graf @ 2005-05-27 15:16 UTC (permalink / raw)
  To: jamal; +Cc: David S. Miller, netdev
In-Reply-To: <1117205822.6383.68.camel@localhost.localdomain>

* jamal <1117205822.6383.68.camel@localhost.localdomain> 2005-05-27 10:57
> The thresholds etc are there - just not netlink accessible. Example, the
> default values for V4 are settable or queriable via:
> 
> /proc/sys/net/ipv4/neigh/default/gc_thresh3
> /proc/sys/net/ipv4/neigh/default/gc_thresh2
> /proc/sys/net/ipv4/neigh/default/gc_thresh1
> /proc/sys/net/ipv4/neigh/default/gc_interval

Sorry but this is just one big hack:

a) they do not belong there, the first implication a user does when
   he sees default/, dev1/, dev2/, devN is that default/ covers
   the same set of parameters as devX/. Therefore he expects all
   parameters in default/ to be also in devX/.

b)	struct neigh_parms	parms;
	/* HACK. gc_* shoul follow parms without a gap! */
	int			gc_interval;
	int			gc_thresh1;
	int			gc_thresh2;
	int			gc_thresh3;

...

	if (dev) {
		dev_name_source = dev->name;
		t->neigh_dev[0].ctl_name = dev->ifindex;
		t->neigh_vars[12].procname = NULL; <- gc_interval
		t->neigh_vars[13].procname = NULL; <- gc_thresh1
		t->neigh_vars[14].procname = NULL; <- gc_thresh2
		t->neigh_vars[15].procname = NULL; <- gc_thresh3
	} else {
 		dev_name_source = t->neigh_dev[0].procname;
		t->neigh_vars[12].data = (int *)(p + 1);
		t->neigh_vars[13].data = (int *)(p + 1) + 1;
		t->neigh_vars[14].data = (int *)(p + 1) + 2;
		t->neigh_vars[15].data = (int *)(p + 1) + 3;
	}

  I will not push along these hacks into my code ;->

Some more notes on the devconfig idea: I'm not sure if you are still
implying devconfig as devinet but if so: although we only have neighbour
tables on devinets so far there might be neighbour table on other
interfaces for whatever reason. A protocol might allocate a neighbour
parameter set on any netdevice which makes me think that the netlink
neighbour code should be open for this as well. It depends on how we
do the devconfig thing, so what I suggest it so leave this as-is for
the moment and make a decision once we know more about how the devconfig
thing will look like. Reasonable?

> I apologize i didnt comment on the patch earlier - sometimes when i am
> away from the list for sometime i skip threads which may require me to
> spend time looking at them to make any useful comments. This maybe the
> reason i didnt respond. To catchup i normally read emails addressed to
> me first then go over the list in a LIFO mode and stop once i get
> engaged in a discussion.

I should have CCed you, my fault.

^ permalink raw reply

* Re: PATCH: rtnetlink explicit flags setting
From: jamal @ 2005-05-27 15:01 UTC (permalink / raw)
  To: Thomas Graf; +Cc: David S. Miller, netdev
In-Reply-To: <20050527141320.GQ15391@postel.suug.ch>

On Fri, 2005-27-05 at 16:13 +0200, Thomas Graf wrote:

> That is what the patch does but the diff looks a bit irritating.
> 1) Introduce NLMSG_NEW to take flags
> 2) Make NLMSG_PUT call NLMSG_NEW with flags==0
> 3) Change __nlmsg_put to take flags argument
> 

Yikes - sorry i glossed over the patch.

> > The first change would be definetely on top of the current patch i sent
> > which is well deserved.
> > Lets have Dave swallow my patch then either you or i could make those
> > changes. Sounds reasonable?
> 
> Sure.


Dave please apply the patch i posted into your tree and then we can
follow with this other stuff. 
Actually Thomas, you can probably apply my patch on your side and then
the other changes to use _NEW() on top of it. If you dont have time i
could do it tommorow.

cheers,
jamal

^ permalink raw reply

* Re: [PATCH 3/4] [NEIGH] neighbour table configuration and statistics via rtnetlink
From: jamal @ 2005-05-27 14:57 UTC (permalink / raw)
  To: Thomas Graf; +Cc: David S. Miller, netdev
In-Reply-To: <20050527141023.GP15391@postel.suug.ch>

On Fri, 2005-27-05 at 16:10 +0200, Thomas Graf wrote:
> * jamal <1117201853.6383.29.camel@localhost.localdomain> 2005-05-27 09:50

> > In other words, if i was to draw a model diagram it would show:
> > --> netdevice 
> >   ----> v4 addresses
> >   ----> v4 netdevice configuration
> >   ----> v6 addresses
> >   ----> v6 netdevice configuration
> > 
> > i.e the best way to do it imo, especially since you are doing this from
> > scratch, is to maintain that model. Your patch breaks away from this.
> 
> I think this model is only valid for neighbour table parameter set
> and not for neighbour tables themselves.
> 

Yes, that's true. In other words, neighbor tables details are out of
scope of that model and are more appropriate to the v4/6 neighbor "box".
Anything else that is netdevice related or connected as in the above
model (as opposed to say the ARP "box" related detail) is better to
leave or add it to be part of devconfig if it doesnt exist.

To be precise there are things in your patch that are fine to leave
there; but the most it seems to me belong to the devconfig netlink path
(which doesnt exist today).

> > The only thing that is not devconfig queriable or settable at the moment
> > is the stats. That i can certainly see as being part of the standard
> > neighbor details for example. Infact this would apply elsewhere as well,
> > for example in the FIB where some stats are being displayed via /proc at
> > the moment.
> 
> This is not correct, struct ndt_config (read-only configuration, sort
> of like statistics), gc thresholds, gc interval, and the default parameter
> set are also not devconfig queriable. From a user point of view the
> most commonly changed values will be the gc thresholds, gc interval
> and parameters in the default set and if you forget about the device
> specific parameters for a second it would be pretty obvious to not put
> them into devconfig. So my point is that I don't want to put the most
> obvious and common use into something where it doesn't really fit.


The thresholds etc are there - just not netlink accessible. Example, the
default values for V4 are settable or queriable via:

/proc/sys/net/ipv4/neigh/default/gc_thresh3
/proc/sys/net/ipv4/neigh/default/gc_thresh2
/proc/sys/net/ipv4/neigh/default/gc_thresh1
/proc/sys/net/ipv4/neigh/default/gc_interval

> I do agree that the device specific parameters fit into a devconfig
> or whathever we'll call it but not the whole neighbour table
> configuration bits. 
> 

Agreed that some things are specific to the "ARP" or "NDISC" block
but not all that you have belongs to those boxes.

> > We actually did discuss this a while back and i had an incomplete patch
> > for doing it the way i suggest above which i unfortunately cant seem to
> > locate at the moment.
> 
> Can't remember to have seen such as mail or patch, do you remember
> the thread you posted this? I posted the patch 3 times and I just
> read through the archives again and can't find any such commments.

I never posted the patch unfortunately but mentioned it in the
discussions. What counts is we are having this discussion now;->

I apologize i didnt comment on the patch earlier - sometimes when i am
away from the list for sometime i skip threads which may require me to
spend time looking at them to make any useful comments. This maybe the
reason i didnt respond. To catchup i normally read emails addressed to
me first then go over the list in a LIFO mode and stop once i get
engaged in a discussion.

cheers,
jamal

^ permalink raw reply

* [PATCH 7/7] [PKT_SCHED] Logic simplifications and codingstyle/whitespace cleanups
From: Thomas Graf @ 2005-05-27 14:56 UTC (permalink / raw)
  To: David S. Miller; +Cc: werner, netdev
In-Reply-To: <20050527145002.GR15391@postel.suug.ch>


Signed-off-by: Thomas Graf <tgraf@suug.ch>

---
commit fde09d4989b6c7b5183ffb3ec2076ff53dd6fa78
tree b032d0d440d93aac252e656bd41df32ff5461e3a
parent 36b5fe1b0094ab6d6ba2589076bec1975d5769ed
author Thomas Graf <tgraf@suug.ch> Fri, 27 May 2005 16:25:41 +0200
committer Thomas Graf <tgraf@suug.ch> Fri, 27 May 2005 16:25:41 +0200

 net/sched/sch_dsmark.c |  174 ++++++++++++++++++++++++-------------------------
 1 files changed, 88 insertions(+), 86 deletions(-)

Index: net/sched/sch_dsmark.c
===================================================================
--- ff95a30a27c52aef9561f2ed04e9950d0afcae6a/net/sched/sch_dsmark.c  (mode:100644)
+++ b032d0d440d93aac252e656bd41df32ff5461e3a/net/sched/sch_dsmark.c  (mode:100644)
@@ -31,7 +31,7 @@
 #endif
 
 
-#define PRIV(sch) qdisc_priv(sch)
+#define PRIV(sch) ((struct dsmark_qdisc_data *) qdisc_priv(sch))
 
 
 /*
@@ -55,10 +55,10 @@
 struct dsmark_qdisc_data {
 	struct Qdisc		*q;
 	struct tcf_proto	*filter_list;
-	__u8			*mask;	/* "owns" the array */
-	__u8			*value;
-	__u16			indices;
-	__u32			default_index;	/* index range is 0...0xffff */
+	u8			*mask;	/* "owns" the array */
+	u8			*value;
+	u16			indices;
+	u32			default_index;	/* index range is 0...0xffff */
 	int			set_tc_index;
 };
 
@@ -80,14 +80,13 @@
 
 /* ------------------------- Class/flow operations ------------------------- */
 
-
-static int dsmark_graft(struct Qdisc *sch,unsigned long arg,
-    struct Qdisc *new,struct Qdisc **old)
+static int dsmark_graft(struct Qdisc *sch, unsigned long arg,
+			struct Qdisc *new, struct Qdisc **old)
 {
 	struct dsmark_qdisc_data *p = PRIV(sch);
 
-	DPRINTK("dsmark_graft(sch %p,[qdisc %p],new %p,old %p)\n",sch,p,new,
-	    old);
+	DPRINTK("dsmark_graft(sch %p,[qdisc %p],new %p,old %p)\n",
+		sch, p, new, old);
 
 	if (new == NULL) {
 		new = qdisc_create_dflt(sch->dev, &pfifo_qdisc_ops);
@@ -96,44 +95,37 @@
 	}
 
 	sch_tree_lock(sch);
-	*old = xchg(&p->q,new);
-	if (*old)
-		qdisc_reset(*old);
+	*old = xchg(&p->q, new);
+	qdisc_reset(*old);
 	sch->q.qlen = 0;
-	sch_tree_unlock(sch); /* @@@ move up ? */
+	sch_tree_unlock(sch);
+
         return 0;
 }
 
-
 static struct Qdisc *dsmark_leaf(struct Qdisc *sch, unsigned long arg)
 {
-	struct dsmark_qdisc_data *p = PRIV(sch);
-
-	return p->q;
+	return PRIV(sch)->q;
 }
 
-
-static unsigned long dsmark_get(struct Qdisc *sch,u32 classid)
+static unsigned long dsmark_get(struct Qdisc *sch, u32 classid)
 {
-	struct dsmark_qdisc_data *p __attribute__((unused)) = PRIV(sch);
+	DPRINTK("dsmark_get(sch %p,[qdisc %p],classid %x)\n",
+		sch, PRIV(sch), classid);
 
-	DPRINTK("dsmark_get(sch %p,[qdisc %p],classid %x)\n",sch,p,classid);
-	return TC_H_MIN(classid)+1;
+	return TC_H_MIN(classid) + 1;
 }
 
-
 static unsigned long dsmark_bind_filter(struct Qdisc *sch,
-    unsigned long parent, u32 classid)
+					unsigned long parent, u32 classid)
 {
-	return dsmark_get(sch,classid);
+	return dsmark_get(sch, classid);
 }
 
-
 static void dsmark_put(struct Qdisc *sch, unsigned long cl)
 {
 }
 
-
 static int dsmark_change(struct Qdisc *sch, u32 classid, u32 parent,
 			 struct rtattr **tca, unsigned long *arg)
 {
@@ -169,26 +161,29 @@
 	return err;
 }
 
-static int dsmark_delete(struct Qdisc *sch,unsigned long arg)
+static int dsmark_delete(struct Qdisc *sch, unsigned long arg)
 {
 	struct dsmark_qdisc_data *p = PRIV(sch);
 
-	if (!arg || arg > p->indices)
+	if (!dsmark_valid_index(p, arg))
 		return -EINVAL;
+	
 	p->mask[arg-1] = 0xff;
 	p->value[arg-1] = 0;
+
 	return 0;
 }
 
-
 static void dsmark_walk(struct Qdisc *sch,struct qdisc_walker *walker)
 {
 	struct dsmark_qdisc_data *p = PRIV(sch);
 	int i;
 
-	DPRINTK("dsmark_walk(sch %p,[qdisc %p],walker %p)\n",sch,p,walker);
+	DPRINTK("dsmark_walk(sch %p,[qdisc %p],walker %p)\n", sch, p, walker);
+
 	if (walker->stop)
 		return;
+
 	for (i = 0; i < p->indices; i++) {
 		if (p->mask[i] == 0xff && !p->value[i])
 			goto ignore;
@@ -203,26 +198,20 @@
         }
 }
 
-
 static struct tcf_proto **dsmark_find_tcf(struct Qdisc *sch,unsigned long cl)
 {
-	struct dsmark_qdisc_data *p = PRIV(sch);
-
-	return &p->filter_list;
+	return &PRIV(sch)->filter_list;
 }
 
-
 /* --------------------------- Qdisc operations ---------------------------- */
 
-
 static int dsmark_enqueue(struct sk_buff *skb,struct Qdisc *sch)
 {
 	struct dsmark_qdisc_data *p = PRIV(sch);
-	struct tcf_result res;
-	int result;
-	int ret = NET_XMIT_POLICED;
+	int err;
+
+	D2PRINTK("dsmark_enqueue(skb %p,sch %p,[qdisc %p])\n", skb, sch, p);
 
-	D2PRINTK("dsmark_enqueue(skb %p,sch %p,[qdisc %p])\n",skb,sch,p);
 	if (p->set_tc_index) {
 		/* FIXME: Safe with non-linear skbs? --RR */
 		switch (skb->protocol) {
@@ -239,17 +228,21 @@
 				break;
 		};
 	}
-	result = TC_POLICE_OK; /* be nice to gcc */
-	if (TC_H_MAJ(skb->priority) == sch->handle) {
+
+	if (TC_H_MAJ(skb->priority) == sch->handle)
 		skb->tc_index = TC_H_MIN(skb->priority);
-	} else {
-		result = tc_classify(skb,p->filter_list,&res);
-		D2PRINTK("result %d class 0x%04x\n",result,res.classid);
+	else {
+		struct tcf_result res;
+		int result = tc_classify(skb, p->filter_list, &res);
+
+		D2PRINTK("result %d class 0x%04x\n", result, res.classid);
+
 		switch (result) {
 #ifdef CONFIG_NET_CLS_POLICE
 			case TC_POLICE_SHOT:
 				kfree_skb(skb);
-				break;
+				sch->qstats.drops++;
+				return NET_XMIT_POLICED;
 #if 0
 			case TC_POLICE_RECLASSIFY:
 				/* FIXME: what to do here ??? */
@@ -266,43 +259,45 @@
 				break;
 		};
 	}
-	if (
-#ifdef CONFIG_NET_CLS_POLICE
-	    result == TC_POLICE_SHOT ||
-#endif
 
-	    ((ret = p->q->enqueue(skb,p->q)) != 0)) {
+	err = p->q->enqueue(skb,p->q);
+	if (err != NET_XMIT_SUCCESS) {
 		sch->qstats.drops++;
-		return ret;
+		return err;
 	}
+
 	sch->bstats.bytes += skb->len;
 	sch->bstats.packets++;
 	sch->q.qlen++;
-	return ret;
-}
 
+	return NET_XMIT_SUCCESS;
+}
 
 static struct sk_buff *dsmark_dequeue(struct Qdisc *sch)
 {
 	struct dsmark_qdisc_data *p = PRIV(sch);
 	struct sk_buff *skb;
-	int index;
+	u32 index;
+
+	D2PRINTK("dsmark_dequeue(sch %p,[qdisc %p])\n", sch, p);
 
-	D2PRINTK("dsmark_dequeue(sch %p,[qdisc %p])\n",sch,p);
 	skb = p->q->ops->dequeue(p->q);
-	if (!skb)
+	if (skb == NULL)
 		return NULL;
+
 	sch->q.qlen--;
-	index = skb->tc_index & (p->indices-1);
-	D2PRINTK("index %d->%d\n",skb->tc_index,index);
+
+	index = skb->tc_index & (p->indices - 1);
+	D2PRINTK("index %d->%d\n", skb->tc_index, index);
+
 	switch (skb->protocol) {
 		case __constant_htons(ETH_P_IP):
-			ipv4_change_dsfield(skb->nh.iph,
-			    p->mask[index],p->value[index]);
+			ipv4_change_dsfield(skb->nh.iph, p->mask[index],
+					    p->value[index]);
 			break;
 		case __constant_htons(ETH_P_IPV6):
-			ipv6_change_dsfield(skb->nh.ipv6h,
-			    p->mask[index],p->value[index]);
+			ipv6_change_dsfield(skb->nh.ipv6h, p->mask[index],
+					    p->value[index]);
 			break;
 		default:
 			/*
@@ -316,41 +311,46 @@
 				       htons(skb->protocol));
 			break;
 	};
+
 	return skb;
 }
 
-
 static int dsmark_requeue(struct sk_buff *skb,struct Qdisc *sch)
 {
-	int ret;
 	struct dsmark_qdisc_data *p = PRIV(sch);
+	int err;
 
-	D2PRINTK("dsmark_requeue(skb %p,sch %p,[qdisc %p])\n",skb,sch,p);
-        if ((ret = p->q->ops->requeue(skb, p->q)) == 0) {
-		sch->q.qlen++;
-		sch->qstats.requeues++;
-		return 0;
+	D2PRINTK("dsmark_requeue(skb %p,sch %p,[qdisc %p])\n", skb, sch, p);
+
+	err = p->q->ops->requeue(skb, p->q);
+	if (err != NET_XMIT_SUCCESS) {
+		sch->qstats.drops++;
+		return err;
 	}
-	sch->qstats.drops++;
-	return ret;
-}
 
+	sch->q.qlen++;
+	sch->qstats.requeues++;
+
+	return NET_XMIT_SUCCESS;
+}
 
 static unsigned int dsmark_drop(struct Qdisc *sch)
 {
 	struct dsmark_qdisc_data *p = PRIV(sch);
 	unsigned int len;
 	
-	DPRINTK("dsmark_reset(sch %p,[qdisc %p])\n",sch,p);
-	if (!p->q->ops->drop)
-		return 0;
-	if (!(len = p->q->ops->drop(p->q)))
+	DPRINTK("dsmark_reset(sch %p,[qdisc %p])\n", sch, p);
+
+	if (p->q->ops->drop == NULL)
 		return 0;
-	sch->q.qlen--;
+
+	len = p->q->ops->drop(p->q);
+	if (len)
+		sch->q.qlen--;
+
 	return len;
 }
 
-
 static int dsmark_init(struct Qdisc *sch, struct rtattr *opt)
 {
 	struct dsmark_qdisc_data *p = PRIV(sch);
@@ -400,33 +400,32 @@
 	return err;
 }
 
-
 static void dsmark_reset(struct Qdisc *sch)
 {
 	struct dsmark_qdisc_data *p = PRIV(sch);
 
-	DPRINTK("dsmark_reset(sch %p,[qdisc %p])\n",sch,p);
+	DPRINTK("dsmark_reset(sch %p,[qdisc %p])\n", sch, p);
 	qdisc_reset(p->q);
 	sch->q.qlen = 0;
 }
 
-
 static void dsmark_destroy(struct Qdisc *sch)
 {
 	struct dsmark_qdisc_data *p = PRIV(sch);
 	struct tcf_proto *tp;
 
-	DPRINTK("dsmark_destroy(sch %p,[qdisc %p])\n",sch,p);
+	DPRINTK("dsmark_destroy(sch %p,[qdisc %p])\n", sch, p);
+
 	while (p->filter_list) {
 		tp = p->filter_list;
 		p->filter_list = tp->next;
 		tcf_destroy(tp);
 	}
+
 	qdisc_destroy(p->q);
 	kfree(p->mask);
 }
 
-
 static int dsmark_dump_class(struct Qdisc *sch, unsigned long cl,
 			     struct sk_buff *skb, struct tcmsg *tcm)
 {
@@ -505,10 +504,13 @@
 {
 	return register_qdisc(&dsmark_qdisc_ops);
 }
+
 static void __exit dsmark_module_exit(void) 
 {
 	unregister_qdisc(&dsmark_qdisc_ops);
 }
+
 module_init(dsmark_module_init)
 module_exit(dsmark_module_exit)
+
 MODULE_LICENSE("GPL");

^ permalink raw reply

* [PATCH 6/7] [PKT_SCHED] Make dsmark use the new dumping macros
From: Thomas Graf @ 2005-05-27 14:54 UTC (permalink / raw)
  To: David S. Miller; +Cc: werner, netdev
In-Reply-To: <20050527145002.GR15391@postel.suug.ch>


Signed-off-by: Thomas Graf <tgraf@suug.ch>

---
commit 36b5fe1b0094ab6d6ba2589076bec1975d5769ed
tree ff95a30a27c52aef9561f2ed04e9950d0afcae6a
parent 76061832cc84456bd970769c0dfcdfefc20d22fa
author Thomas Graf <tgraf@suug.ch> Fri, 27 May 2005 15:22:44 +0200
committer Thomas Graf <tgraf@suug.ch> Fri, 27 May 2005 15:22:44 +0200

 net/sched/sch_dsmark.c |   52 ++++++++++++++++++++++---------------------------
 1 files changed, 24 insertions(+), 28 deletions(-)

Index: net/sched/sch_dsmark.c
===================================================================
--- ffa2efa3cf35b0a3c8c983060731be7b4d3330db/net/sched/sch_dsmark.c  (mode:100644)
+++ ff95a30a27c52aef9561f2ed04e9950d0afcae6a/net/sched/sch_dsmark.c  (mode:100644)
@@ -428,50 +428,46 @@
 
 
 static int dsmark_dump_class(struct Qdisc *sch, unsigned long cl,
-    struct sk_buff *skb, struct tcmsg *tcm)
+			     struct sk_buff *skb, struct tcmsg *tcm)
 {
 	struct dsmark_qdisc_data *p = PRIV(sch);
-	unsigned char *b = skb->tail;
-	struct rtattr *rta;
+	struct rtattr *opts = NULL;
 
-	DPRINTK("dsmark_dump_class(sch %p,[qdisc %p],class %ld\n",sch,p,cl);
-	if (!cl || cl > p->indices)
+	DPRINTK("dsmark_dump_class(sch %p,[qdisc %p],class %ld\n", sch, p, cl);
+
+	if (!dsmark_valid_index(p, cl))
 		return -EINVAL;
-	tcm->tcm_handle = TC_H_MAKE(TC_H_MAJ(sch->handle),cl-1);
-	rta = (struct rtattr *) b;
-	RTA_PUT(skb,TCA_OPTIONS,0,NULL);
-	RTA_PUT(skb,TCA_DSMARK_MASK,1,&p->mask[cl-1]);
-	RTA_PUT(skb,TCA_DSMARK_VALUE,1,&p->value[cl-1]);
-	rta->rta_len = skb->tail-b;
-	return skb->len;
+
+	tcm->tcm_handle = TC_H_MAKE(TC_H_MAJ(sch->handle), cl-1);
+
+	opts = RTA_NEST(skb, TCA_OPTIONS);
+	RTA_PUT_U8(skb,TCA_DSMARK_MASK, p->mask[cl-1]);
+	RTA_PUT_U8(skb,TCA_DSMARK_VALUE, p->value[cl-1]);
+
+	return RTA_NEST_END(skb, opts);
 
 rtattr_failure:
-	skb_trim(skb,b-skb->data);
-	return -1;
+	return RTA_NEST_CANCEL(skb, opts);
 }
 
 static int dsmark_dump(struct Qdisc *sch, struct sk_buff *skb)
 {
 	struct dsmark_qdisc_data *p = PRIV(sch);
-	unsigned char *b = skb->tail;
-	struct rtattr *rta;
+	struct rtattr *opts = NULL;
+
+	opts = RTA_NEST(skb, TCA_OPTIONS);
+	RTA_PUT_U16(skb, TCA_DSMARK_INDICES, p->indices);
 
-	rta = (struct rtattr *) b;
-	RTA_PUT(skb,TCA_OPTIONS,0,NULL);
-	RTA_PUT(skb,TCA_DSMARK_INDICES,sizeof(__u16),&p->indices);
-	if (p->default_index != NO_DEFAULT_INDEX) {
-		__u16 tmp = p->default_index;
+	if (p->default_index != NO_DEFAULT_INDEX)
+		RTA_PUT_U16(skb, TCA_DSMARK_DEFAULT_INDEX, p->default_index);
 
-		RTA_PUT(skb,TCA_DSMARK_DEFAULT_INDEX, sizeof(__u16), &tmp);
-	}
 	if (p->set_tc_index)
-		RTA_PUT(skb, TCA_DSMARK_SET_TC_INDEX, 0, NULL);
-	rta->rta_len = skb->tail-b;
-	return skb->len;
+		RTA_PUT_FLAG(skb, TCA_DSMARK_SET_TC_INDEX);
+
+	return RTA_NEST_END(skb, opts);
 
 rtattr_failure:
-	skb_trim(skb,b-skb->data);
-	return -1;
+	return RTA_NEST_CANCEL(skb, opts);
 }
 
 static struct Qdisc_class_ops dsmark_class_ops = {

^ permalink raw reply

* [PATCH 5/7] [PKT_SCHED] Fix dsmark to apply changes consistent
From: Thomas Graf @ 2005-05-27 14:53 UTC (permalink / raw)
  To: David S. Miller; +Cc: werner, netdev
In-Reply-To: <20050527145002.GR15391@postel.suug.ch>

Fixes dsmark to do all configuration sanity checks first and
only apply the changes if all of them can be applied without
any errors. Also fixes the weak sanity checks for DSMARK_VALUE
and DSMASK_MASK.

Signed-off-by: Thomas Graf <tgraf@suug.ch>

---
commit 76061832cc84456bd970769c0dfcdfefc20d22fa
tree ffa2efa3cf35b0a3c8c983060731be7b4d3330db
parent c49bbdd22621352b9b96097ffdb9e5c14b4ed95e
author Thomas Graf <tgraf@suug.ch> Fri, 27 May 2005 15:15:26 +0200
committer Thomas Graf <tgraf@suug.ch> Fri, 27 May 2005 15:15:26 +0200

 net/sched/sch_dsmark.c |  129 ++++++++++++++++++++++++++++++-------------------
 1 files changed, 81 insertions(+), 48 deletions(-)

Index: net/sched/sch_dsmark.c
===================================================================
--- 6204e59b7aa59cf79632003c621e521db1297b1b/net/sched/sch_dsmark.c  (mode:100644)
+++ ffa2efa3cf35b0a3c8c983060731be7b4d3330db/net/sched/sch_dsmark.c  (mode:100644)
@@ -62,6 +62,21 @@
 	int			set_tc_index;
 };
 
+static inline int dsmark_valid_indices(u16 indices)
+{
+	while (indices != 1) {
+		if (indices & 1)
+			return 0;
+		indices >>= 1;
+	}
+ 
+	return 1;
+}
+
+static inline int dsmark_valid_index(struct dsmark_qdisc_data *p, u16 index)
+{
+	return (index <= p->indices && index > 0);
+}
 
 /* ------------------------- Class/flow operations ------------------------- */
 
@@ -120,31 +135,39 @@
 
 
 static int dsmark_change(struct Qdisc *sch, u32 classid, u32 parent,
-    struct rtattr **tca, unsigned long *arg)
+			 struct rtattr **tca, unsigned long *arg)
 {
 	struct dsmark_qdisc_data *p = PRIV(sch);
 	struct rtattr *opt = tca[TCA_OPTIONS-1];
 	struct rtattr *tb[TCA_DSMARK_MAX];
+	int err = -EINVAL;
+	u8 mask = 0;
 
 	DPRINTK("dsmark_change(sch %p,[qdisc %p],classid %x,parent %x),"
-	    "arg 0x%lx\n",sch,p,classid,parent,*arg);
-	if (*arg > p->indices)
-		return -ENOENT;
-	if (!opt || rtattr_parse_nested(tb, TCA_DSMARK_MAX, opt))
-		return -EINVAL;
-	if (tb[TCA_DSMARK_MASK-1]) {
-		if (!RTA_PAYLOAD(tb[TCA_DSMARK_MASK-1]))
-			return -EINVAL;
-		p->mask[*arg-1] = *(__u8 *) RTA_DATA(tb[TCA_DSMARK_MASK-1]);
-	}
-	if (tb[TCA_DSMARK_VALUE-1]) {
-		if (!RTA_PAYLOAD(tb[TCA_DSMARK_VALUE-1]))
-			return -EINVAL;
-		p->value[*arg-1] = *(__u8 *) RTA_DATA(tb[TCA_DSMARK_VALUE-1]);
+		"arg 0x%lx\n", sch, p, classid, parent, *arg);
+
+	if (!dsmark_valid_index(p, *arg)) {
+		err = -ENOENT;
+		goto rtattr_failure;
 	}
-	return 0;
-}
 
+	if (!opt || rtattr_parse_nested(tb, TCA_DSMARK_MAX, opt))
+		goto rtattr_failure;
+
+	if (tb[TCA_DSMARK_MASK-1])
+		mask = RTA_GET_U8(tb[TCA_DSMARK_MASK-1]);
+
+	if (tb[TCA_DSMARK_VALUE-1])
+		p->value[*arg-1] = RTA_GET_U8(tb[TCA_DSMARK_VALUE-1]);
+		
+	if (tb[TCA_DSMARK_MASK-1])
+		p->mask[*arg-1] = mask;
+
+	err = 0;
+
+rtattr_failure:
+	return err;
+}
 
 static int dsmark_delete(struct Qdisc *sch,unsigned long arg)
 {
@@ -328,43 +351,53 @@
 }
 
 
-static int dsmark_init(struct Qdisc *sch,struct rtattr *opt)
+static int dsmark_init(struct Qdisc *sch, struct rtattr *opt)
 {
 	struct dsmark_qdisc_data *p = PRIV(sch);
 	struct rtattr *tb[TCA_DSMARK_MAX];
-	__u16 tmp;
+	int err = -EINVAL;
+	u32 default_index = NO_DEFAULT_INDEX;
+	u16 indices;
+	u8 *mask;
 
-	DPRINTK("dsmark_init(sch %p,[qdisc %p],opt %p)\n",sch,p,opt);
-	if (!opt ||
-	    rtattr_parse(tb,TCA_DSMARK_MAX,RTA_DATA(opt),RTA_PAYLOAD(opt)) < 0 ||
-	    !tb[TCA_DSMARK_INDICES-1] ||
-	    RTA_PAYLOAD(tb[TCA_DSMARK_INDICES-1]) < sizeof(__u16))
-                return -EINVAL;
-	p->indices = *(__u16 *) RTA_DATA(tb[TCA_DSMARK_INDICES-1]);
-	if (!p->indices)
-		return -EINVAL;
-	for (tmp = p->indices; tmp != 1; tmp >>= 1) {
-		if (tmp & 1)
-			return -EINVAL;
-	}
-	p->default_index = NO_DEFAULT_INDEX;
-	if (tb[TCA_DSMARK_DEFAULT_INDEX-1]) {
-		if (RTA_PAYLOAD(tb[TCA_DSMARK_DEFAULT_INDEX-1]) < sizeof(__u16))
-			return -EINVAL;
-		p->default_index =
-		    *(__u16 *) RTA_DATA(tb[TCA_DSMARK_DEFAULT_INDEX-1]);
+	DPRINTK("dsmark_init(sch %p,[qdisc %p],opt %p)\n", sch, p, opt);
+
+	if (!opt || rtattr_parse_nested(tb, TCA_DSMARK_MAX, opt) < 0)
+		goto errout;
+
+	indices = RTA_GET_U16(tb[TCA_DSMARK_INDICES-1]);
+	if (!indices || !dsmark_valid_indices(indices))
+		goto errout;
+
+	if (tb[TCA_DSMARK_DEFAULT_INDEX-1])
+		default_index = RTA_GET_U16(tb[TCA_DSMARK_DEFAULT_INDEX-1]);
+
+	mask = kmalloc(indices * 2, GFP_KERNEL);
+	if (mask == NULL) {
+		err = -ENOMEM;
+		goto errout;
 	}
-	p->set_tc_index = !!tb[TCA_DSMARK_SET_TC_INDEX-1];
-	p->mask = kmalloc(p->indices*2,GFP_KERNEL);
-	if (!p->mask)
-		return -ENOMEM;
-	p->value = p->mask+p->indices;
-	memset(p->mask,0xff,p->indices);
-	memset(p->value,0,p->indices);
-	if (!(p->q = qdisc_create_dflt(sch->dev, &pfifo_qdisc_ops)))
+
+	p->mask = mask;
+	memset(p->mask, 0xff, indices);
+
+	p->value = p->mask + indices;
+	memset(p->value, 0, indices);
+
+	p->indices = indices;
+	p->default_index = default_index;
+	p->set_tc_index = RTA_GET_FLAG(tb[TCA_DSMARK_SET_TC_INDEX-1]);
+
+	p->q = qdisc_create_dflt(sch->dev, &pfifo_qdisc_ops);
+	if (p->q == NULL)
 		p->q = &noop_qdisc;
-	DPRINTK("dsmark_init: qdisc %p\n",&p->q);
-	return 0;
+
+	DPRINTK("dsmark_init: qdisc %p\n", p->q);
+
+	err = 0;
+errout:
+rtattr_failure:
+	return err;
 }
 
 

^ permalink raw reply

* [PATCH 4/7] [RTNETLINK] Add RTA_(PUT|GET) shortcuts for u8, u16, and flag
From: Thomas Graf @ 2005-05-27 14:52 UTC (permalink / raw)
  To: David S. Miller; +Cc: werner, netdev
In-Reply-To: <20050527145002.GR15391@postel.suug.ch>


Signed-off-by: Thomas Graf <tgraf@suug.ch>

---
commit c49bbdd22621352b9b96097ffdb9e5c14b4ed95e
tree 6204e59b7aa59cf79632003c621e521db1297b1b
parent 0ecf7991c4f8532a5afc903e175e2bec80585fea
author Thomas Graf <tgraf@suug.ch> Fri, 27 May 2005 14:36:11 +0200
committer Thomas Graf <tgraf@suug.ch> Fri, 27 May 2005 14:36:11 +0200

 include/linux/rtnetlink.h |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+)

Index: include/linux/rtnetlink.h
===================================================================
--- 07e28bf36ae4052536820a75bbefc6ac1c7a9098/include/linux/rtnetlink.h  (mode:100644)
+++ 6204e59b7aa59cf79632003c621e521db1297b1b/include/linux/rtnetlink.h  (mode:100644)
@@ -897,6 +897,14 @@
 		goto rtattr_failure; \
 	memcpy(skb_put(skb, RTA_ALIGN(attrlen)), data, attrlen); })
 
+#define RTA_PUT_U8(skb, attrtype, value) \
+({	u8 _tmp = (value); \
+	RTA_PUT(skb, attrtype, sizeof(u8), &_tmp); })
+
+#define RTA_PUT_U16(skb, attrtype, value) \
+({	u16 _tmp = (value); \
+	RTA_PUT(skb, attrtype, sizeof(u16), &_tmp); })
+
 #define RTA_PUT_U32(skb, attrtype, value) \
 ({	u32 _tmp = (value); \
 	RTA_PUT(skb, attrtype, sizeof(u32), &_tmp); })
@@ -914,6 +922,9 @@
 #define RTA_PUT_STRING(skb, attrtype, value) \
 	RTA_PUT(skb, attrtype, strlen(value) + 1, value)
 
+#define RTA_PUT_FLAG(skb, attrtype) \
+	RTA_PUT(skb, attrtype, 0, NULL);
+
 #define RTA_NEST(skb, type) \
 ({	struct rtattr *__start = (struct rtattr *) (skb)->tail; \
 	RTA_PUT(skb, type, 0, NULL); \
@@ -928,6 +939,16 @@
 		skb_trim(skb, (unsigned char *) (start) - (skb)->data); \
 	-1; })
 
+#define RTA_GET_U8(rta) \
+({	if (!rta || RTA_PAYLOAD(rta) < sizeof(u8)) \
+		goto rtattr_failure; \
+	*(u8 *) RTA_DATA(rta); })
+
+#define RTA_GET_U16(rta) \
+({	if (!rta || RTA_PAYLOAD(rta) < sizeof(u16)) \
+		goto rtattr_failure; \
+	*(u16 *) RTA_DATA(rta); })
+
 #define RTA_GET_U32(rta) \
 ({	if (!rta || RTA_PAYLOAD(rta) < sizeof(u32)) \
 		goto rtattr_failure; \
@@ -940,6 +961,8 @@
 	memcpy(&_tmp, RTA_DATA(rta), sizeof(_tmp)); \
 	_tmp; })
 
+#define RTA_GET_FLAG(rta) (!!(rta))
+
 #define RTA_GET_SECS(rta) ((unsigned long) RTA_GET_U64(rta) * HZ)
 #define RTA_GET_MSECS(rta) (msecs_to_jiffies((unsigned long) RTA_GET_U64(rta)))
 		

^ permalink raw reply

* [PATCH 3/7] [PKT_SCHED] Disable dsmark debugging messages by default
From: Thomas Graf @ 2005-05-27 14:52 UTC (permalink / raw)
  To: David S. Miller; +Cc: werner, netdev
In-Reply-To: <20050527145002.GR15391@postel.suug.ch>


Signed-off-by: Thomas Graf <tgraf@suug.ch>

---
commit 0ecf7991c4f8532a5afc903e175e2bec80585fea
tree 07e28bf36ae4052536820a75bbefc6ac1c7a9098
parent 0297af53b691b2c14b09f65be7b9e32a43a34319
author Thomas Graf <tgraf@suug.ch> Fri, 27 May 2005 14:35:44 +0200
committer Thomas Graf <tgraf@suug.ch> Fri, 27 May 2005 14:35:44 +0200

 net/sched/sch_dsmark.c |    2 +-
 1 files changed, 1 insertion(+), 1 deletion(-)

Index: net/sched/sch_dsmark.c
===================================================================
--- 686339fe0d5a70e263d261324365e65f53cc9880/net/sched/sch_dsmark.c  (mode:100644)
+++ 07e28bf36ae4052536820a75bbefc6ac1c7a9098/net/sched/sch_dsmark.c  (mode:100644)
@@ -18,7 +18,7 @@
 #include <asm/byteorder.h>
 
 
-#if 1 /* control */
+#if 0 /* control */
 #define DPRINTK(format,args...) printk(KERN_DEBUG format,##args)
 #else
 #define DPRINTK(format,args...)

^ permalink raw reply

* [PATCH 2/7] [PKT_SCHED] make dsmark try using pfifo instead of noop while grafting
From: Thomas Graf @ 2005-05-27 14:51 UTC (permalink / raw)
  To: David S. Miller; +Cc: werner, netdev
In-Reply-To: <20050527145002.GR15391@postel.suug.ch>


Signed-off-by: Thomas Graf <tgraf@suug.ch>

---
commit 0297af53b691b2c14b09f65be7b9e32a43a34319
tree 686339fe0d5a70e263d261324365e65f53cc9880
parent 99ed2eb92cc035b223e2d383446f7f183600ba27
author Thomas Graf <tgraf@suug.ch> Fri, 27 May 2005 14:30:37 +0200
committer Thomas Graf <tgraf@suug.ch> Fri, 27 May 2005 14:30:37 +0200

 net/sched/sch_dsmark.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

Index: net/sched/sch_dsmark.c
===================================================================
--- 127116412eeedf62fcda0acc882500cd981cbab1/net/sched/sch_dsmark.c  (mode:100644)
+++ 686339fe0d5a70e263d261324365e65f53cc9880/net/sched/sch_dsmark.c  (mode:100644)
@@ -73,8 +73,13 @@
 
 	DPRINTK("dsmark_graft(sch %p,[qdisc %p],new %p,old %p)\n",sch,p,new,
 	    old);
-	if (!new)
-		new = &noop_qdisc;
+
+	if (new == NULL) {
+		new = qdisc_create_dflt(sch->dev, &pfifo_qdisc_ops);
+		if (new == NULL)
+			new = &noop_qdisc;
+	}
+
 	sch_tree_lock(sch);
 	*old = xchg(&p->q,new);
 	if (*old)

^ permalink raw reply

* [PATCH 1/7] [PKT_SCHED] Fix dsmark to count ignored indices while walking
From: Thomas Graf @ 2005-05-27 14:50 UTC (permalink / raw)
  To: David S. Miller; +Cc: werner, netdev
In-Reply-To: <20050527145002.GR15391@postel.suug.ch>

Unused indices which are ignored while walking must still
be counted to avoid dumping the same index twice.

Signed-off-by: Thomas Graf <tgraf@suug.ch>

---
commit 99ed2eb92cc035b223e2d383446f7f183600ba27
tree 127116412eeedf62fcda0acc882500cd981cbab1
parent e66646bb6686bb1be24e25b3743028f762544de2
author Thomas Graf <tgraf@suug.ch> Fri, 27 May 2005 14:29:52 +0200
committer Thomas Graf <tgraf@suug.ch> Fri, 27 May 2005 14:29:52 +0200

 net/sched/sch_dsmark.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

Index: net/sched/sch_dsmark.c
===================================================================
--- 735126283c38e22995f5da71b2e90a9d71ac4b1e/net/sched/sch_dsmark.c  (mode:100644)
+++ 127116412eeedf62fcda0acc882500cd981cbab1/net/sched/sch_dsmark.c  (mode:100644)
@@ -163,14 +163,15 @@
 		return;
 	for (i = 0; i < p->indices; i++) {
 		if (p->mask[i] == 0xff && !p->value[i])
-			continue;
+			goto ignore;
 		if (walker->count >= walker->skip) {
 			if (walker->fn(sch, i+1, walker) < 0) {
 				walker->stop = 1;
 				break;
 			}
 		}
-                walker->count++;
+ignore:		
+		walker->count++;
         }
 }
 

^ permalink raw reply

* [PATCHSET] dsmark fixes & cleanups
From: Thomas Graf @ 2005-05-27 14:50 UTC (permalink / raw)
  To: David S. Miller; +Cc: werner, netdev

Dave,

The following patchset fixes various smallish bugs in dsmark
and does a general cleanup. The lower the patch number,
the less intrusive and more serious the bug is.

Patches tested using tc-testsuite (see below) and some
manual testing.

---
dsmark: dsmark root qdisc creation succeeded
dsmark: dsmark class 1 creation succeeded
dsmark: dsmark class 2 creation succeeded
dsmark: dsmark dump qdisc succeeded with output:
qdisc dsmark 10: indices 0x0040 default_index 0x0001 set_tc_index 
dsmark: dsmark dump class succeeded with output:
class dsmark 10:12 parent 10: mask 0xff value 0x02 
class dsmark 10:13 parent 10: mask 0xfc value 0x04 
dsmark: generic qdisc tree deletion succeeded
---

^ permalink raw reply

* Re: PATCH: rtnetlink explicit flags setting
From: Thomas Graf @ 2005-05-27 14:13 UTC (permalink / raw)
  To: jamal; +Cc: David S. Miller, netdev
In-Reply-To: <1117202331.6383.39.camel@localhost.localdomain>

* jamal <1117202331.6383.39.camel@localhost.localdomain> 2005-05-27 09:58
> On Fri, 2005-27-05 at 14:50 +0200, Thomas Graf wrote:
> 
> 
> >  
> > -#define NLMSG_PUT(skb, pid, seq, type, len) \
> > +#define NLMSG_NEW(skb, pid, seq, type, len, flags) \
> 
> How about introducing NLMSG_NEW and leave NLMSG_PUT where it is?
> This way we could do incremental updates. If there are not too many of
> those NLMSG_PUT around, then it shouldnt be an issue.

That is what the patch does but the diff looks a bit irritating.
1) Introduce NLMSG_NEW to take flags
2) Make NLMSG_PUT call NLMSG_NEW with flags==0
3) Change __nlmsg_put to take flags argument

> The first change would be definetely on top of the current patch i sent
> which is well deserved.
> Lets have Dave swallow my patch then either you or i could make those
> changes. Sounds reasonable?

Sure.

^ permalink raw reply

* [PATCH] baycom_epp.c: Fix compilation with gcc-4.1.0-20050522
From: Alexey Dobriyan @ 2005-05-27 14:08 UTC (permalink / raw)
  To: netdev; +Cc: Thomas Sailer

drivers/net/hamradio/baycom_epp.c:432: error: jump into statement expression
	...

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>

--- linux-vanilla/drivers/net/hamradio/baycom_epp.c	2005-05-27 12:58:48.000000000 +0400
+++ linux-gcc4/drivers/net/hamradio/baycom_epp.c	2005-05-27 17:54:14.000000000 +0400
@@ -380,22 +380,18 @@ static inline void do_kiss_params(struct
  */
 
 #define ENCODEITERA(j)                         \
-({                                             \
         if (!(notbitstream & (0x1f0 << j)))    \
                 goto stuff##j;                 \
-  encodeend##j:    	;                      \
-})
+  encodeend##j:    	;
 
 #define ENCODEITERB(j)                                          \
-({                                                              \
   stuff##j:                                                     \
         bitstream &= ~(0x100 << j);                             \
         bitbuf = (bitbuf & (((2 << j) << numbit) - 1)) |        \
                 ((bitbuf & ~(((2 << j) << numbit) - 1)) << 1);  \
         numbit++;                                               \
         notbitstream = ~bitstream;                              \
-        goto encodeend##j;                                      \
-})
+        goto encodeend##j;
 
 
 static void encode_hdlc(struct baycom_state *bc)
@@ -611,16 +607,13 @@ static void do_rxpacket(struct net_devic
 }
 
 #define DECODEITERA(j)                                                        \
-({                                                                            \
         if (!(notbitstream & (0x0fc << j)))              /* flag or abort */  \
                 goto flgabrt##j;                                              \
         if ((bitstream & (0x1f8 << j)) == (0xf8 << j))   /* stuffed bit */    \
                 goto stuff##j;                                                \
-  enditer##j:      ;                                                           \
-})
+  enditer##j:      ;
 
 #define DECODEITERB(j)                                                                 \
-({                                                                                     \
   flgabrt##j:                                                                          \
         if (!(notbitstream & (0x1fc << j))) {              /* abort received */        \
                 state = 0;                                                             \
@@ -638,8 +631,7 @@ static void do_rxpacket(struct net_devic
   stuff##j:                                                                            \
         numbits--;                                                                     \
         bitbuf = (bitbuf & ((~0xff) << j)) | ((bitbuf & ~((~0xff) << j)) << 1);        \
-        goto enditer##j;                                                               \
-})
+        goto enditer##j;
         
 static int receive(struct net_device *dev, int cnt)
 {

^ permalink raw reply

* Re: PATCH: rtnetlink explicit flags setting
From: jamal @ 2005-05-27 13:58 UTC (permalink / raw)
  To: Thomas Graf; +Cc: David S. Miller, netdev
In-Reply-To: <20050527125010.GO15391@postel.suug.ch>

On Fri, 2005-27-05 at 14:50 +0200, Thomas Graf wrote:


>  
> -#define NLMSG_PUT(skb, pid, seq, type, len) \
> +#define NLMSG_NEW(skb, pid, seq, type, len, flags) \

How about introducing NLMSG_NEW and leave NLMSG_PUT where it is?
This way we could do incremental updates. If there are not too many of
those NLMSG_PUT around, then it shouldnt be an issue.

The first change would be definetely on top of the current patch i sent
which is well deserved.
Lets have Dave swallow my patch then either you or i could make those
changes. Sounds reasonable?

cheers,
jamal

^ permalink raw reply

* Re: [PATCH 3/4] [NEIGH] neighbour table configuration and statistics via rtnetlink
From: jamal @ 2005-05-27 13:50 UTC (permalink / raw)
  To: Thomas Graf; +Cc: David S. Miller, netdev
In-Reply-To: <20050527121503.GN15391@postel.suug.ch>

On Fri, 2005-27-05 at 14:15 +0200, Thomas Graf wrote:
> * jamal <1117192464.6688.3.camel@localhost.localdomain> 2005-05-27 07:14
> > It would have been better, IMO, to just work on a generic idev/devinet
> > retrieval and setting instead of one component (as in ARP in this
> > case). 
> 
> I thought about adding a devinet component to allow retrieving/setting
> device specific parameters (along with other settings) but dropped the
> idea because I did not want to implement the data structures twice.
>
> I think this is cleaner, it allows one request to be sent to see
> the complete arp/ndisc configuration rather than sending one to
> retrieve the global configuration/statistics and another one
> to retrieve device specific parameters.
> 
> Nevertheless, I'm open for changes so once I've written the devinet
> component to change rp_filter, forwarding, arp_filter, etc. we can
> still move the device specific NDTA_PARMS to the devinet component
> if we still think it would be better. For now, the neightbl component
> is clean, small, well defined and probably never needs to be touched
> again.
> 
> Is that acceptable for you?
> 


I think depends on where we are going. My view is that the current
abstraction model is fine for this specific bit.

i.e from an abstraction point of view these parameters are per device
per protocol (v4/v6/etc). 

In other words, if i was to draw a model diagram it would show:
--> netdevice 
  ----> v4 addresses
  ----> v4 netdevice configuration
  ----> v6 addresses
  ----> v6 netdevice configuration

i.e the best way to do it imo, especially since you are doing this from
scratch, is to maintain that model. Your patch breaks away from this.
The only thing that is not devconfig queriable or settable at the moment
is the stats. That i can certainly see as being part of the standard
neighbor details for example. Infact this would apply elsewhere as well,
for example in the FIB where some stats are being displayed via /proc at
the moment.

We actually did discuss this a while back and i had an incomplete patch
for doing it the way i suggest above which i unfortunately cant seem to
locate at the moment. 

cheers,
jamal

^ permalink raw reply

* Re: PATCH: rtnetlink explicit flags setting
From: Thomas Graf @ 2005-05-27 12:50 UTC (permalink / raw)
  To: jamal; +Cc: David S. Miller, netdev
In-Reply-To: <1117197157.6688.24.camel@localhost.localdomain>

* jamal <1117197157.6688.24.camel@localhost.localdomain> 2005-05-27 08:32
> 
> I think i may have missed a few spots.
> 
> Thomas it would be worth creating a macro which takes flags as input as
> well to replace things along the lines of:
>         nlh = NLMSG_PUT(skb, pid, seq, event, sizeof(*ifm));
>         nlh->nlmsg_flags = flags;
> Since theres a sufficient number of them.

Right, proposal:

--- 6204e59b7aa59cf79632003c621e521db1297b1b/include/linux/netlink.h  (mode:100644)
+++ uncommitted/include/linux/netlink.h  (mode:100644)
@@ -156,7 +156,7 @@
 };
 
 static __inline__ struct nlmsghdr *
-__nlmsg_put(struct sk_buff *skb, u32 pid, u32 seq, int type, int len)
+__nlmsg_put(struct sk_buff *skb, u32 pid, u32 seq, int type, int len, int flags)
 {
        struct nlmsghdr *nlh;
        int size = NLMSG_LENGTH(len);
@@ -164,20 +164,23 @@
        nlh = (struct nlmsghdr*)skb_put(skb, NLMSG_ALIGN(size));
        nlh->nlmsg_type = type;
        nlh->nlmsg_len = size;
-       nlh->nlmsg_flags = 0;
+       nlh->nlmsg_flags = flags;
        nlh->nlmsg_pid = pid;
        nlh->nlmsg_seq = seq;
        return nlh;
 }
 
-#define NLMSG_PUT(skb, pid, seq, type, len) \
+#define NLMSG_NEW(skb, pid, seq, type, len, flags) \
 ({     if (skb_tailroom(skb) < (int)NLMSG_SPACE(len)) \
                goto nlmsg_failure; \
-       __nlmsg_put(skb, pid, seq, type, len); })
+       __nlmsg_put(skb, pid, seq, type, len, flags); })
+
+#define NLMSG_PUT(skb, pid, seq, type, len) \
+       NLMSG_NEW(skb, pid, seq, type, len, 0)
 
-#define NLMSG_PUT_ANSWER(skb, cb, type, len) \
-       NLMSG_PUT(skb, NETLINK_CB((cb)->skb).pid, \
-                 (cb)->nlh->nlmsg_seq, type, len)
+#define NLMSG_NEW_ANSWER(skb, cb, type, len, flags) \
+       NLMSG_NEW(skb, NETLINK_CB((cb)->skb).pid, \
+                 (cb)->nlh->nlmsg_seq, type, len, flags)
 
 #define NLMSG_END(skb, nlh) \
 ({     (nlh)->nlmsg_len = (skb)->tail - (unsigned char *) (nlh); \


I'd be especially happy with this because __nlmsg_put keeps control
over the flags ;->

^ permalink raw reply

* PATCH: rtnetlink explicit flags setting
From: jamal @ 2005-05-27 12:32 UTC (permalink / raw)
  To: David S. Miller; +Cc: Thomas Graf, netdev

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


I think i may have missed a few spots.

Thomas it would be worth creating a macro which takes flags as input as
well to replace things along the lines of:
        nlh = NLMSG_PUT(skb, pid, seq, event, sizeof(*ifm));
        nlh->nlmsg_flags = flags;
Since theres a sufficient number of them.

I will work next on fixing ioctl derived events to contain the proper
pid of the process that caused the event

cheers,
jamal
--


This patch rectifies some rtnetlink message builders that derive the
flags from the pid. It is now explicit like the other cases
which get it right.

Signed-off-by: Jamal Hadi Salim <hadi@cyberus.ca>




[-- Attachment #2: rtnet_info_p --]
[-- Type: text/plain, Size: 12095 bytes --]

net/core/neighbour.c: needs update
net/core/rtnetlink.c: needs update
net/decnet/dn_route.c: needs update
net/ipv4/devinet.c: needs update
net/ipv4/route.c: needs update
net/ipv6/addrconf.c: needs update
Index: net/core/neighbour.c
===================================================================
--- 2cbffac3afc093b729781fdc58acfb43acc98bc8/net/core/neighbour.c  (mode:100644)
+++ uncommitted/net/core/neighbour.c  (mode:100644)
@@ -1548,7 +1548,7 @@
 
 
 static int neigh_fill_info(struct sk_buff *skb, struct neighbour *n,
-			   u32 pid, u32 seq, int event)
+			   u32 pid, u32 seq, int event, unsigned flags)
 {
 	unsigned long now = jiffies;
 	unsigned char *b = skb->tail;
@@ -1559,7 +1559,7 @@
 					 sizeof(struct ndmsg));
 	struct ndmsg *ndm = NLMSG_DATA(nlh);
 
-	nlh->nlmsg_flags = pid ? NLM_F_MULTI : 0;
+	nlh->nlmsg_flags = flags;
 	ndm->ndm_family	 = n->ops->family;
 	ndm->ndm_flags	 = n->flags;
 	ndm->ndm_type	 = n->type;
@@ -1609,7 +1609,8 @@
 				continue;
 			if (neigh_fill_info(skb, n, NETLINK_CB(cb->skb).pid,
 					    cb->nlh->nlmsg_seq,
-					    RTM_NEWNEIGH) <= 0) {
+					    RTM_NEWNEIGH,
+					    NLM_F_MULTI) <= 0) {
 				read_unlock_bh(&tbl->lock);
 				rc = -1;
 				goto out;
@@ -2018,7 +2019,7 @@
 	if (!skb)
 		return;
 
-	if (neigh_fill_info(skb, n, 0, 0, RTM_GETNEIGH) < 0) {
+	if (neigh_fill_info(skb, n, 0, 0, RTM_GETNEIGH, 0) < 0) {
 		kfree_skb(skb);
 		return;
 	}
@@ -2037,7 +2038,7 @@
 	if (!skb)
 		return;
 
-	if (neigh_fill_info(skb, n, 0, 0, RTM_NEWNEIGH) < 0) {
+	if (neigh_fill_info(skb, n, 0, 0, RTM_NEWNEIGH, 0) < 0) {
 		kfree_skb(skb);
 		return;
 	}
Index: net/core/rtnetlink.c
===================================================================
--- 2cbffac3afc093b729781fdc58acfb43acc98bc8/net/core/rtnetlink.c  (mode:100644)
+++ uncommitted/net/core/rtnetlink.c  (mode:100644)
@@ -176,14 +176,15 @@
 
 
 static int rtnetlink_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
-				 int type, u32 pid, u32 seq, u32 change)
+				 int type, u32 pid, u32 seq, u32 change, 
+				 unsigned flags)
 {
 	struct ifinfomsg *r;
 	struct nlmsghdr  *nlh;
 	unsigned char	 *b = skb->tail;
 
 	nlh = NLMSG_PUT(skb, pid, seq, type, sizeof(*r));
-	if (pid) nlh->nlmsg_flags |= NLM_F_MULTI;
+	nlh->nlmsg_flags = flags;
 	r = NLMSG_DATA(nlh);
 	r->ifi_family = AF_UNSPEC;
 	r->ifi_type = dev->type;
@@ -273,7 +274,7 @@
 	for (dev=dev_base, idx=0; dev; dev = dev->next, idx++) {
 		if (idx < s_idx)
 			continue;
-		if (rtnetlink_fill_ifinfo(skb, dev, RTM_NEWLINK, NETLINK_CB(cb->skb).pid, cb->nlh->nlmsg_seq, 0) <= 0)
+		if (rtnetlink_fill_ifinfo(skb, dev, RTM_NEWLINK, NETLINK_CB(cb->skb).pid, cb->nlh->nlmsg_seq, 0, NLM_F_MULTI) <= 0)
 			break;
 	}
 	read_unlock(&dev_base_lock);
@@ -447,7 +448,7 @@
 	if (!skb)
 		return;
 
-	if (rtnetlink_fill_ifinfo(skb, dev, type, 0, 0, change) < 0) {
+	if (rtnetlink_fill_ifinfo(skb, dev, type, 0, 0, change, 0) < 0) {
 		kfree_skb(skb);
 		return;
 	}
Index: net/decnet/dn_route.c
===================================================================
--- 2cbffac3afc093b729781fdc58acfb43acc98bc8/net/decnet/dn_route.c  (mode:100644)
+++ uncommitted/net/decnet/dn_route.c  (mode:100644)
@@ -1465,7 +1465,7 @@
 	return dn_route_input_slow(skb);
 }
 
-static int dn_rt_fill_info(struct sk_buff *skb, u32 pid, u32 seq, int event, int nowait)
+static int dn_rt_fill_info(struct sk_buff *skb, u32 pid, u32 seq, int event, int nowait, unsigned flags)
 {
 	struct dn_route *rt = (struct dn_route *)skb->dst;
 	struct rtmsg *r;
@@ -1475,7 +1475,7 @@
 
 	nlh = NLMSG_PUT(skb, pid, seq, event, sizeof(*r));
 	r = NLMSG_DATA(nlh);
-	nlh->nlmsg_flags = (nowait && pid) ? NLM_F_MULTI : 0;
+	nlh->nlmsg_flags = flags;
 	r->rtm_family = AF_DECnet;
 	r->rtm_dst_len = 16;
 	r->rtm_src_len = 0;
@@ -1596,7 +1596,7 @@
 
 	NETLINK_CB(skb).dst_pid = NETLINK_CB(in_skb).pid;
 
-	err = dn_rt_fill_info(skb, NETLINK_CB(in_skb).pid, nlh->nlmsg_seq, RTM_NEWROUTE, 0);
+	err = dn_rt_fill_info(skb, NETLINK_CB(in_skb).pid, nlh->nlmsg_seq, RTM_NEWROUTE, 0, 0);
 
 	if (err == 0)
 		goto out_free;
@@ -1644,7 +1644,8 @@
 				continue;
 			skb->dst = dst_clone(&rt->u.dst);
 			if (dn_rt_fill_info(skb, NETLINK_CB(cb->skb).pid,
-					cb->nlh->nlmsg_seq, RTM_NEWROUTE, 1) <= 0) {
+					cb->nlh->nlmsg_seq, RTM_NEWROUTE, 
+					1, NLM_F_MULTI) <= 0) {
 				dst_release(xchg(&skb->dst, NULL));
 				rcu_read_unlock_bh();
 				goto done;
Index: net/ipv4/devinet.c
===================================================================
--- 2cbffac3afc093b729781fdc58acfb43acc98bc8/net/ipv4/devinet.c  (mode:100644)
+++ uncommitted/net/ipv4/devinet.c  (mode:100644)
@@ -1015,14 +1015,14 @@
 };
 
 static int inet_fill_ifaddr(struct sk_buff *skb, struct in_ifaddr *ifa,
-			    u32 pid, u32 seq, int event)
+			    u32 pid, u32 seq, int event, unsigned flags)
 {
 	struct ifaddrmsg *ifm;
 	struct nlmsghdr  *nlh;
 	unsigned char	 *b = skb->tail;
 
 	nlh = NLMSG_PUT(skb, pid, seq, event, sizeof(*ifm));
-	if (pid) nlh->nlmsg_flags |= NLM_F_MULTI;
+	nlh->nlmsg_flags = flags;
 	ifm = NLMSG_DATA(nlh);
 	ifm->ifa_family = AF_INET;
 	ifm->ifa_prefixlen = ifa->ifa_prefixlen;
@@ -1075,7 +1075,7 @@
 				continue;
 			if (inet_fill_ifaddr(skb, ifa, NETLINK_CB(cb->skb).pid,
 					     cb->nlh->nlmsg_seq,
-					     RTM_NEWADDR) <= 0) {
+					     RTM_NEWADDR, NLM_F_MULTI) <= 0) {
 				rcu_read_unlock();
 				goto done;
 			}
@@ -1098,7 +1098,7 @@
 
 	if (!skb)
 		netlink_set_err(rtnl, 0, RTMGRP_IPV4_IFADDR, ENOBUFS);
-	else if (inet_fill_ifaddr(skb, ifa, 0, 0, event) < 0) {
+	else if (inet_fill_ifaddr(skb, ifa, 0, 0, event, 0) < 0) {
 		kfree_skb(skb);
 		netlink_set_err(rtnl, 0, RTMGRP_IPV4_IFADDR, EINVAL);
 	} else {
Index: net/ipv4/route.c
===================================================================
--- 2cbffac3afc093b729781fdc58acfb43acc98bc8/net/ipv4/route.c  (mode:100644)
+++ uncommitted/net/ipv4/route.c  (mode:100644)
@@ -2581,7 +2581,7 @@
 }
 
 static int rt_fill_info(struct sk_buff *skb, u32 pid, u32 seq, int event,
-			int nowait)
+			int nowait, unsigned flags)
 {
 	struct rtable *rt = (struct rtable*)skb->dst;
 	struct rtmsg *r;
@@ -2593,7 +2593,7 @@
 #endif
 	nlh = NLMSG_PUT(skb, pid, seq, event, sizeof(*r));
 	r = NLMSG_DATA(nlh);
-	nlh->nlmsg_flags = (nowait && pid) ? NLM_F_MULTI : 0;
+	nlh->nlmsg_flags = flags;
 	r->rtm_family	 = AF_INET;
 	r->rtm_dst_len	= 32;
 	r->rtm_src_len	= 0;
@@ -2744,7 +2744,7 @@
 	NETLINK_CB(skb).dst_pid = NETLINK_CB(in_skb).pid;
 
 	err = rt_fill_info(skb, NETLINK_CB(in_skb).pid, nlh->nlmsg_seq,
-				RTM_NEWROUTE, 0);
+				RTM_NEWROUTE, 0, 0);
 	if (!err)
 		goto out_free;
 	if (err < 0) {
@@ -2781,8 +2781,8 @@
 				continue;
 			skb->dst = dst_clone(&rt->u.dst);
 			if (rt_fill_info(skb, NETLINK_CB(cb->skb).pid,
-					 cb->nlh->nlmsg_seq,
-					 RTM_NEWROUTE, 1) <= 0) {
+					 cb->nlh->nlmsg_seq, RTM_NEWROUTE, 
+					 1, NLM_F_MULTI) <= 0) {
 				dst_release(xchg(&skb->dst, NULL));
 				rcu_read_unlock_bh();
 				goto done;
Index: net/ipv6/addrconf.c
===================================================================
--- 2cbffac3afc093b729781fdc58acfb43acc98bc8/net/ipv6/addrconf.c  (mode:100644)
+++ uncommitted/net/ipv6/addrconf.c  (mode:100644)
@@ -2621,7 +2621,7 @@
 }
 
 static int inet6_fill_ifaddr(struct sk_buff *skb, struct inet6_ifaddr *ifa,
-			     u32 pid, u32 seq, int event)
+			     u32 pid, u32 seq, int event, unsigned flags)
 {
 	struct ifaddrmsg *ifm;
 	struct nlmsghdr  *nlh;
@@ -2629,7 +2629,7 @@
 	unsigned char	 *b = skb->tail;
 
 	nlh = NLMSG_PUT(skb, pid, seq, event, sizeof(*ifm));
-	if (pid) nlh->nlmsg_flags |= NLM_F_MULTI;
+	nlh->nlmsg_flags = flags;
 	ifm = NLMSG_DATA(nlh);
 	ifm->ifa_family = AF_INET6;
 	ifm->ifa_prefixlen = ifa->prefix_len;
@@ -2671,7 +2671,7 @@
 }
 
 static int inet6_fill_ifmcaddr(struct sk_buff *skb, struct ifmcaddr6 *ifmca,
-				u32 pid, u32 seq, int event)
+				u32 pid, u32 seq, int event, unsigned flags)
 {
 	struct ifaddrmsg *ifm;
 	struct nlmsghdr  *nlh;
@@ -2679,7 +2679,7 @@
 	unsigned char	 *b = skb->tail;
 
 	nlh = NLMSG_PUT(skb, pid, seq, event, sizeof(*ifm));
-	if (pid) nlh->nlmsg_flags |= NLM_F_MULTI;
+	nlh->nlmsg_flags = flags;
 	ifm = NLMSG_DATA(nlh);
 	ifm->ifa_family = AF_INET6;	
 	ifm->ifa_prefixlen = 128;
@@ -2708,7 +2708,7 @@
 }
 
 static int inet6_fill_ifacaddr(struct sk_buff *skb, struct ifacaddr6 *ifaca,
-				u32 pid, u32 seq, int event)
+				u32 pid, u32 seq, int event, unsigned flags)
 {
 	struct ifaddrmsg *ifm;
 	struct nlmsghdr  *nlh;
@@ -2716,7 +2716,7 @@
 	unsigned char	 *b = skb->tail;
 
 	nlh = NLMSG_PUT(skb, pid, seq, event, sizeof(*ifm));
-	if (pid) nlh->nlmsg_flags |= NLM_F_MULTI;
+	nlh->nlmsg_flags = flags;
 	ifm = NLMSG_DATA(nlh);
 	ifm->ifa_family = AF_INET6;	
 	ifm->ifa_prefixlen = 128;
@@ -2785,7 +2785,8 @@
 					continue;
 				if ((err = inet6_fill_ifaddr(skb, ifa, 
 				    NETLINK_CB(cb->skb).pid, 
-				    cb->nlh->nlmsg_seq, RTM_NEWADDR)) <= 0)
+				    cb->nlh->nlmsg_seq, RTM_NEWADDR,
+				    NLM_F_MULTI)) <= 0)
 					goto done;
 			}
 			/* temp addr */
@@ -2796,7 +2797,8 @@
 					continue;
 				if ((err = inet6_fill_ifaddr(skb, ifa, 
 				    NETLINK_CB(cb->skb).pid, 
-				    cb->nlh->nlmsg_seq, RTM_NEWADDR)) <= 0) 
+				    cb->nlh->nlmsg_seq, RTM_NEWADDR,
+				    NLM_F_MULTI)) <= 0) 
 					goto done;
 			}
 #endif
@@ -2809,7 +2811,8 @@
 					continue;
 				if ((err = inet6_fill_ifmcaddr(skb, ifmca, 
 				    NETLINK_CB(cb->skb).pid, 
-				    cb->nlh->nlmsg_seq, RTM_GETMULTICAST)) <= 0)
+				    cb->nlh->nlmsg_seq, RTM_GETMULTICAST,
+				    NLM_F_MULTI)) <= 0)
 					goto done;
 			}
 			break;
@@ -2821,7 +2824,8 @@
 					continue;
 				if ((err = inet6_fill_ifacaddr(skb, ifaca, 
 				    NETLINK_CB(cb->skb).pid, 
-				    cb->nlh->nlmsg_seq, RTM_GETANYCAST)) <= 0) 
+				    cb->nlh->nlmsg_seq, RTM_GETANYCAST,
+				    NLM_F_MULTI)) <= 0) 
 					goto done;
 			}
 			break;
@@ -2871,7 +2875,7 @@
 		netlink_set_err(rtnl, 0, RTMGRP_IPV6_IFADDR, ENOBUFS);
 		return;
 	}
-	if (inet6_fill_ifaddr(skb, ifa, 0, 0, event) < 0) {
+	if (inet6_fill_ifaddr(skb, ifa, 0, 0, event, 0) < 0) {
 		kfree_skb(skb);
 		netlink_set_err(rtnl, 0, RTMGRP_IPV6_IFADDR, EINVAL);
 		return;
@@ -2906,7 +2910,7 @@
 }
 
 static int inet6_fill_ifinfo(struct sk_buff *skb, struct inet6_dev *idev, 
-			     u32 pid, u32 seq, int event)
+			     u32 pid, u32 seq, int event, unsigned flags)
 {
 	struct net_device	*dev = idev->dev;
 	__s32			*array = NULL;
@@ -2918,7 +2922,7 @@
 	struct ifla_cacheinfo	ci;
 
 	nlh = NLMSG_PUT(skb, pid, seq, event, sizeof(*r));
-	if (pid) nlh->nlmsg_flags |= NLM_F_MULTI;
+	nlh->nlmsg_flags = flags;
 	r = NLMSG_DATA(nlh);
 	r->ifi_family = AF_INET6;
 	r->ifi_type = dev->type;
@@ -2985,7 +2989,7 @@
 		if ((idev = in6_dev_get(dev)) == NULL)
 			continue;
 		err = inet6_fill_ifinfo(skb, idev, NETLINK_CB(cb->skb).pid, 
-				cb->nlh->nlmsg_seq, RTM_NEWLINK);
+				cb->nlh->nlmsg_seq, RTM_NEWLINK, NLM_F_MULTI);
 		in6_dev_put(idev);
 		if (err <= 0)
 			break;
@@ -3007,7 +3011,7 @@
 		netlink_set_err(rtnl, 0, RTMGRP_IPV6_IFINFO, ENOBUFS);
 		return;
 	}
-	if (inet6_fill_ifinfo(skb, idev, 0, 0, event) < 0) {
+	if (inet6_fill_ifinfo(skb, idev, 0, 0, event, 0) < 0) {
 		kfree_skb(skb);
 		netlink_set_err(rtnl, 0, RTMGRP_IPV6_IFINFO, EINVAL);
 		return;
@@ -3017,7 +3021,8 @@
 }
 
 static int inet6_fill_prefix(struct sk_buff *skb, struct inet6_dev *idev,
-			struct prefix_info *pinfo, u32 pid, u32 seq, int event)
+			struct prefix_info *pinfo, u32 pid, u32 seq, 
+			int event, unsigned flags)
 {
 	struct prefixmsg	*pmsg;
 	struct nlmsghdr 	*nlh;
@@ -3025,9 +3030,7 @@
 	struct prefix_cacheinfo	ci;
 
 	nlh = NLMSG_PUT(skb, pid, seq, event, sizeof(*pmsg));
-	
-	if (pid) 
-		nlh->nlmsg_flags |= NLM_F_MULTI;
+	nlh->nlmsg_flags = flags;
 	
 	pmsg = NLMSG_DATA(nlh);
 	pmsg->prefix_family = AF_INET6;
@@ -3067,7 +3070,7 @@
 		netlink_set_err(rtnl, 0, RTMGRP_IPV6_PREFIX, ENOBUFS);
 		return;
 	}
-	if (inet6_fill_prefix(skb, idev, pinfo, 0, 0, event) < 0) {
+	if (inet6_fill_prefix(skb, idev, pinfo, 0, 0, event, 0) < 0) {
 		kfree_skb(skb);
 		netlink_set_err(rtnl, 0, RTMGRP_IPV6_PREFIX, EINVAL);
 		return;

^ permalink raw reply

* Re: [PATCH 3/4] [NEIGH] neighbour table configuration and statistics via rtnetlink
From: Thomas Graf @ 2005-05-27 12:15 UTC (permalink / raw)
  To: jamal; +Cc: David S. Miller, netdev
In-Reply-To: <1117192464.6688.3.camel@localhost.localdomain>

* jamal <1117192464.6688.3.camel@localhost.localdomain> 2005-05-27 07:14
> It would have been better, IMO, to just work on a generic idev/devinet
> retrieval and setting instead of one component (as in ARP in this
> case). 

I thought about adding a devinet component to allow retrieving/setting
device specific parameters (along with other settings) but dropped the
idea because I did not want to implement the data structures twice.

I think this is cleaner, it allows one request to be sent to see
the complete arp/ndisc configuration rather than sending one to
retrieve the global configuration/statistics and another one
to retrieve device specific parameters.

Nevertheless, I'm open for changes so once I've written the devinet
component to change rp_filter, forwarding, arp_filter, etc. we can
still move the device specific NDTA_PARMS to the devinet component
if we still think it would be better. For now, the neightbl component
is clean, small, well defined and probably never needs to be touched
again.

Is that acceptable for you?

^ permalink raw reply

* [PATCH] shaper.c: fix locking
From: Christoph Hellwig @ 2005-05-27 11:54 UTC (permalink / raw)
  To: davem; +Cc: netdev

 o use a semaphore instead of an opencoded and racy lock
 o move locking out of shaper_kick and into the callers - most just
   released the lock before calling shaper_kick
 o remove in_interrupt() tests.  from ->close we can always block, from
   ->hard_start_xmit and timer context never


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/include/linux/if_shaper.h
===================================================================
--- linux-2.6.orig/include/linux/if_shaper.h	2005-04-30 10:17:19.000000000 +0200
+++ linux-2.6/include/linux/if_shaper.h	2005-05-27 13:47:47.000000000 +0200
@@ -23,7 +23,7 @@
 	__u32 shapeclock;
 	unsigned long recovery;	/* Time we can next clock a packet out on
 				   an empty queue */
-        unsigned long locked;
+	struct semaphore sem;
         struct net_device_stats stats;
 	struct net_device *dev;
 	int  (*hard_start_xmit) (struct sk_buff *skb,
@@ -38,7 +38,6 @@
 	int (*hard_header_cache)(struct neighbour *neigh, struct hh_cache *hh);
 	void (*header_cache_update)(struct hh_cache *hh, struct net_device *dev, unsigned char *  haddr);
 	struct net_device_stats* (*get_stats)(struct net_device *dev);
-	wait_queue_head_t  wait_queue;
 	struct timer_list timer;
 };
 
Index: linux-2.6/drivers/net/shaper.c
===================================================================
--- linux-2.6.orig/drivers/net/shaper.c	2005-04-30 10:17:15.000000000 +0200
+++ linux-2.6/drivers/net/shaper.c	2005-05-27 13:50:41.000000000 +0200
@@ -100,35 +100,8 @@
 
 #define SHAPER_BANNER	"CymruNet Traffic Shaper BETA 0.04 for Linux 2.1\n"
 
-/*
- *	Locking
- */
- 
-static int shaper_lock(struct shaper *sh)
-{
-	/*
-	 *	Lock in an interrupt must fail
-	 */
-	while (test_and_set_bit(0, &sh->locked))
-	{
-		if (!in_interrupt())
-			sleep_on(&sh->wait_queue);
-		else
-			return 0;
-			
-	}
-	return 1;
-}
-
 static void shaper_kick(struct shaper *sh);
 
-static void shaper_unlock(struct shaper *sh)
-{
-	clear_bit(0, &sh->locked);
-	wake_up(&sh->wait_queue);
-	shaper_kick(sh);
-}
-
 /*
  *	Compute clocks on a buffer
  */
@@ -157,17 +130,15 @@
  *	Throw a frame at a shaper.
  */
   
-static int shaper_qframe(struct shaper *shaper, struct sk_buff *skb)
+
+static int shaper_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
+	struct shaper *shaper = dev->priv;
  	struct sk_buff *ptr;
    
- 	/*
- 	 *	Get ready to work on this shaper. Lock may fail if its
- 	 *	an interrupt and locked.
- 	 */
- 	 
- 	if(!shaper_lock(shaper))
- 		return -1;
+	if (down_trylock(&shaper->sem))
+		return -1;
+
  	ptr=shaper->sendq.prev;
  	
  	/*
@@ -260,7 +231,8 @@
                 dev_kfree_skb(ptr);
                 shaper->stats.collisions++;
  	}
- 	shaper_unlock(shaper);
+	shaper_kick(shaper);
+	up(&shaper->sem);
  	return 0;
 }
 
@@ -297,8 +269,13 @@
  
 static void shaper_timer(unsigned long data)
 {
-	struct shaper *sh=(struct shaper *)data;
-	shaper_kick(sh);
+	struct shaper *shaper = (struct shaper *)data;
+
+	if (!down_trylock(&shaper->sem)) {
+		shaper_kick(shaper);
+		up(&shaper->sem);
+	} else
+		mod_timer(&shaper->timer, jiffies);
 }
 
 /*
@@ -311,19 +288,6 @@
 	struct sk_buff *skb;
 	
 	/*
-	 *	Shaper unlock will kick
-	 */
-	 
-	if (test_and_set_bit(0, &shaper->locked))
-	{
-		if(sh_debug)
-			printk("Shaper locked.\n");
-		mod_timer(&shaper->timer, jiffies);
-		return;
-	}
-
-		
-	/*
 	 *	Walk the list (may be empty)
 	 */
 	 
@@ -364,8 +328,6 @@
 	 
 	if(skb!=NULL)
 		mod_timer(&shaper->timer, SHAPERCB(skb)->shapeclock);
-
-	clear_bit(0, &shaper->locked);
 }
 
 
@@ -376,14 +338,12 @@
 static void shaper_flush(struct shaper *shaper)
 {
 	struct sk_buff *skb;
- 	if(!shaper_lock(shaper))
-	{
-		printk(KERN_ERR "shaper: shaper_flush() called by an irq!\n");
- 		return;
-	}
+
+	down(&shaper->sem);
 	while((skb=skb_dequeue(&shaper->sendq))!=NULL)
 		dev_kfree_skb(skb);
-	shaper_unlock(shaper);
+	shaper_kick(shaper);
+	up(&shaper->sem);
 }
 
 /*
@@ -426,13 +386,6 @@
  *	ARP and other resolutions and not before.
  */
 
-
-static int shaper_start_xmit(struct sk_buff *skb, struct net_device *dev)
-{
-	struct shaper *sh=dev->priv;
-	return shaper_qframe(sh, skb);
-}
-
 static struct net_device_stats *shaper_get_stats(struct net_device *dev)
 {
      	struct shaper *sh=dev->priv;
@@ -623,7 +576,6 @@
 	init_timer(&sh->timer);
 	sh->timer.function=shaper_timer;
 	sh->timer.data=(unsigned long)sh;
-	init_waitqueue_head(&sh->wait_queue);
 }
 
 /*

^ permalink raw reply

* [9/10] Not quite...
From: Herbert Xu @ 2005-05-27 11:28 UTC (permalink / raw)
  To: David S. Miller, jamal, Patrick McHardy, netdev
In-Reply-To: <20050527111534.GH4545@gondor.apana.org.au>

Sorry, I incorrectly counted a patch that had been reverted.  So that's it
for now :)
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ 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