Received: with ECARTIS (v1.0.0; list netdev); Thu, 30 Dec 2004 16:54:24 -0800 (PST)
Received: from kaber.coreworks.de ([62.206.217.67])
	by oss.sgi.com (8.13.0/8.13.0) with ESMTP id iBV0ruOP011014
	for <netdev@oss.sgi.com>; Thu, 30 Dec 2004 16:54:17 -0800
Received: from eru.coreworks.de ([172.16.0.2] helo=trash.net)
	by kaber.coreworks.de with esmtp (Exim 4.34)
	id 1CkBBF-0004zD-PL; Fri, 31 Dec 2004 02:02:14 +0100
Message-ID: <41D4A4D2.4000109@trash.net>
Date: Fri, 31 Dec 2004 02:01:06 +0100
From: Patrick McHardy <kaber@trash.net>
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040413 Debian/1.6-5
X-Accept-Language: en
MIME-Version: 1.0
To: Thomas Graf <tgraf@suug.ch>
CC: "David S. Miller" <davem@davemloft.net>,
        Jamal Hadi Salim <hadi@cyberus.ca>, netdev@oss.sgi.com
Subject: Re: [PATCH 2/9] PKT_SCHED: tc filter extension API
References: <20041230122652.GM32419@postel.suug.ch> <20041230123023.GO32419@postel.suug.ch>
In-Reply-To: <20041230123023.GO32419@postel.suug.ch>
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Transfer-Encoding: 7bit
X-Virus-Scanned: ClamAV 0.80/645/Mon Dec 27 14:56:20 2004
	clamav-milter version 0.80j
	on 127.0.0.1
X-Virus-Status: Clean
X-archive-position: 13263
X-ecartis-version: Ecartis v1.0.0
Sender: netdev-bounce@oss.sgi.com
Errors-to: netdev-bounce@oss.sgi.com
X-original-sender: kaber@trash.net
Precedence: bulk
X-list: netdev

Thomas Graf wrote:

> +int
> +tcf_exts_validate(struct tcf_proto *tp, struct rtattr **tb,
> +	          struct rtattr *rate_tlv, struct tcf_exts *exts,
> +	          struct tcf_ext_map *map)
> +{
> +	memset(exts, 0, sizeof(*exts));
> +	
> +#ifdef CONFIG_NET_CLS_ACT
> +	int err;
> +	struct tc_action *act;
> +
> +	if (map->police && tb[map->police-1] && rate_tlv) {
> +		act = tcf_action_init_1(tb[map->police-1], rate_tlv, "police",
> +			TCA_ACT_NOREPLACE, TCA_ACT_BIND, &err);
> +		if (NULL == act)

Please use act == NULL


> +void
> +tcf_exts_change(struct tcf_proto *tp, struct tcf_exts *dst,
> +	        struct tcf_exts *src)
> +{
> +#ifdef CONFIG_NET_CLS_ACT
> +	if (src->action) {
> +		if (dst->action) {
> +			struct tc_action *act;
> +
> +			tcf_tree_lock(tp);
> +			act = xchg(&dst->action, src->action);
> +			tcf_tree_unlock(tp);
> +
> +			tcf_action_destroy(act, TCA_ACT_UNBIND);
> +		} else
> +			dst->action = src->action;

This isn't right (its also wrong in the current code). If the
CPU reorders stores and another CPU looks at dst->action at
the wrong time it might see an inconsistent structure. So at
least smp_wmb is required for the unlocked case, but I think
you should just remove it completely. I also wonder if anyone
actually knows why we need the xchg (here and in all the other
places), it looks totally useless.

Regards
Patrick