xt_MARK useless line

xt_MARK useless line

[Jan Engelhardt]

Hi,


in linux-2.6.20-rc2/net/netfilter/xt_MARK.c one can find

        if((*pskb)->mark != markinfo->mark)
                (*pskb)->mark = markinfo->mark;

would not it be simpler just to set the mark? Or are comparison-jumps 
like these 'cheaper' than assignments? (I don't think so.)

	-`J'
-- 

Re: xt_MARK useless line

[Harald Welte]

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

On Fri, Dec 29, 2006 at 05:54:07PM +0100, Jan Engelhardt wrote:
> Hi,
> 
> 
> in linux-2.6.20-rc2/net/netfilter/xt_MARK.c one can find
> 
>         if((*pskb)->mark != markinfo->mark)
>                 (*pskb)->mark = markinfo->mark;
> 
> would not it be simpler just to set the mark? Or are comparison-jumps 
> like these 'cheaper' than assignments? (I don't think so.)

I don't really know. I think the intention might have been to prevent
writes to shared data structures, in order to reduce cache ping-pong on
SMP.  However, unlesss some really complex and rare event happens, this
skb will never be processed asynchronously and thus not be used on any
other CPU.

-- 
- Harald Welte <laforge@netfilter.org>                 http://netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: xt_MARK useless line

[Jan Engelhardt]

On Jan 7 2007 15:15, Harald Welte wrote:
>On Fri, Dec 29, 2006 at 05:54:07PM +0100, Jan Engelhardt wrote:
>> Hi,
>> 
>> in linux-2.6.20-rc2/net/netfilter/xt_MARK.c one can find
>> 
>>         if((*pskb)->mark != markinfo->mark)
>>                 (*pskb)->mark = markinfo->mark;
>> 
>> would not it be simpler just to set the mark? Or are comparison-jumps 
>> like these 'cheaper' than assignments? (I don't think so.)
>
>I don't really know. I think the intention might have been to prevent
>writes to shared data structures, in order to reduce cache ping-pong on
>SMP.  However, unlesss some really complex and rare event happens, this
>skb will never be processed asynchronously and thus not be used on any
>other CPU.

In other words, your conclusion is ... - (R)emove/(K)eep?
(For the record, it's anywhere where something is set, that is, MARK, CONNMARK,
SECMARK, TOS, ECN, and so on, everywhere you have XT_CONTINUE)


	-`J'
-- 

Re: xt_MARK useless line

[Philip Craig]

Jan Engelhardt wrote:
> On Jan 7 2007 15:15, Harald Welte wrote:
>> On Fri, Dec 29, 2006 at 05:54:07PM +0100, Jan Engelhardt wrote:
>>> Hi,
>>>
>>> in linux-2.6.20-rc2/net/netfilter/xt_MARK.c one can find
>>>
>>>         if((*pskb)->mark != markinfo->mark)
>>>                 (*pskb)->mark = markinfo->mark;
>>>
>>> would not it be simpler just to set the mark? Or are comparison-jumps 
>>> like these 'cheaper' than assignments? (I don't think so.)
>> I don't really know. I think the intention might have been to prevent
>> writes to shared data structures, in order to reduce cache ping-pong on
>> SMP.  However, unlesss some really complex and rare event happens, this
>> skb will never be processed asynchronously and thus not be used on any
>> other CPU.
> 
> In other words, your conclusion is ... - (R)emove/(K)eep?
> (For the record, it's anywhere where something is set, that is, MARK, CONNMARK,
> SECMARK, TOS, ECN, and so on, everywhere you have XT_CONTINUE)

Some of these may be leftover from when NFC_ALTERED existed?

Re: xt_MARK useless line

[Patrick McHardy]

Philip Craig wrote:
> Jan Engelhardt wrote:
> 
>>On Jan 7 2007 15:15, Harald Welte wrote:
>>
>>>On Fri, Dec 29, 2006 at 05:54:07PM +0100, Jan Engelhardt wrote:
>>>
>>>>Hi,
>>>>
>>>>in linux-2.6.20-rc2/net/netfilter/xt_MARK.c one can find
>>>>
>>>>        if((*pskb)->mark != markinfo->mark)
>>>>                (*pskb)->mark = markinfo->mark;
>>>>
>>>>would not it be simpler just to set the mark? Or are comparison-jumps 
>>>>like these 'cheaper' than assignments? (I don't think so.)
>>>
>>>I don't really know. I think the intention might have been to prevent
>>>writes to shared data structures, in order to reduce cache ping-pong on
>>>SMP.  However, unlesss some really complex and rare event happens, this
>>>skb will never be processed asynchronously and thus not be used on any
>>>other CPU.
>>
>>In other words, your conclusion is ... - (R)emove/(K)eep?
>>(For the record, it's anywhere where something is set, that is, MARK, CONNMARK,
>>SECMARK, TOS, ECN, and so on, everywhere you have XT_CONTINUE)
> 
> 
> Some of these may be leftover from when NFC_ALTERED existed?

That sounds like the most likely explanation. Looking at 2.4:

        if((*pskb)->nfmark != markinfo->mark) {
                (*pskb)->nfmark = markinfo->mark;
                (*pskb)->nfcache |= NFC_ALTERED;

        }

I vote for removing this (also in CONNMARK, CONNSECMARK, ...).

Re: xt_MARK useless line

[Jan Engelhardt]

On Jan 10 2007 06:53, Patrick McHardy wrote:
>> 
>> Some of these may be leftover from when NFC_ALTERED existed?
>
>That sounds like the most likely explanation. Looking at 2.4:
>
>        if((*pskb)->nfmark != markinfo->mark) {
>                (*pskb)->nfmark = markinfo->mark;
>                (*pskb)->nfcache |= NFC_ALTERED;
>
>        }
>
>I vote for removing this (also in CONNMARK, CONNSECMARK, ...).
>

I'll whack it up.


	-`J'
-- 

Re: xt_MARK useless line

[Jan Engelhardt]

On Jan 10 2007 06:53, Patrick McHardy wrote:
>> 
>> Some of these may be leftover from when NFC_ALTERED existed?
>
>That sounds like the most likely explanation. Looking at 2.4:
>
>        if((*pskb)->nfmark != markinfo->mark) {
>                (*pskb)->nfmark = markinfo->mark;
>                (*pskb)->nfcache |= NFC_ALTERED;
>
>        }
>
>I vote for removing this (also in CONNMARK, CONNSECMARK, ...).
>

Not everywhere, xt_CONNMARK has a place where:

newmark = (*ctmark & ~markinfo->mask) | markinfo->mark;
if (newmark != *ctmark) {
    *ctmark = newmark;
#if defined(CONFIG_IP_NF_CONNTRACK) || defined(CONFIG_IP_NF_CONNTRACK_MODULE)
    ip_conntrack_event_cache(IPCT_MARK, *pskb);
#else
    nf_conntrack_event_cache(IPCT_MARK, *pskb);
#endif
}

Would it matter to call these cache functions it even if the mark did
not really change?

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Remove unnecessary if() constructs before assignment.
Plus one spelling fix, one brace indent fix.

Signed-off-by: Jan Engelhardt <jengelh@gmx.de>

Index: linux-2.6.20-rc4/net/ipv6/netfilter/ip6t_HL.c
===================================================================
--- linux-2.6.20-rc4.orig/net/ipv6/netfilter/ip6t_HL.c
+++ linux-2.6.20-rc4/net/ipv6/netfilter/ip6t_HL.c
@@ -52,8 +52,7 @@ static unsigned int ip6t_hl_target(struc
 			break;
 	}
 
-	if (new_hl != ip6h->hop_limit)
-		ip6h->hop_limit = new_hl;
+	ip6h->hop_limit = new_hl;
 
 	return IP6T_CONTINUE;
 }
Index: linux-2.6.20-rc4/net/netfilter/xt_CLASSIFY.c
===================================================================
--- linux-2.6.20-rc4.orig/net/netfilter/xt_CLASSIFY.c
+++ linux-2.6.20-rc4/net/netfilter/xt_CLASSIFY.c
@@ -32,10 +32,7 @@ target(struct sk_buff **pskb,
        const void *targinfo)
 {
 	const struct xt_classify_target_info *clinfo = targinfo;
-
-	if ((*pskb)->priority != clinfo->priority)
-		(*pskb)->priority = clinfo->priority;
-
+	(*pskb)->priority = clinfo->priority;
 	return XT_CONTINUE;
 }
 
Index: linux-2.6.20-rc4/net/netfilter/xt_CONNMARK.c
===================================================================
--- linux-2.6.20-rc4.orig/net/netfilter/xt_CONNMARK.c
+++ linux-2.6.20-rc4/net/netfilter/xt_CONNMARK.c
@@ -61,7 +61,7 @@ target(struct sk_buff **pskb,
 #else
 				nf_conntrack_event_cache(IPCT_MARK, *pskb);
 #endif
-		}
+			}
 			break;
 		case XT_CONNMARK_SAVE:
 			newmark = (*ctmark & ~markinfo->mask) |
@@ -78,8 +78,7 @@ target(struct sk_buff **pskb,
 		case XT_CONNMARK_RESTORE:
 			mark = (*pskb)->mark;
 			diff = (*ctmark ^ mark) & markinfo->mask;
-			if (diff != 0)
-				(*pskb)->mark = mark ^ diff;
+			(*pskb)->mark = mark ^ diff;
 			break;
 		}
 	}
Index: linux-2.6.20-rc4/net/netfilter/xt_CONNSECMARK.c
===================================================================
--- linux-2.6.20-rc4.orig/net/netfilter/xt_CONNSECMARK.c
+++ linux-2.6.20-rc4/net/netfilter/xt_CONNSECMARK.c
@@ -41,8 +41,7 @@ static void secmark_save(struct sk_buff 
 
 		connsecmark = nf_ct_get_secmark(skb, &ctinfo);
 		if (connsecmark && !*connsecmark)
-			if (*connsecmark != skb->secmark)
-				*connsecmark = skb->secmark;
+			*connsecmark = skb->secmark;
 	}
 }
 
@@ -58,8 +57,7 @@ static void secmark_restore(struct sk_bu
 
 		connsecmark = nf_ct_get_secmark(skb, &ctinfo);
 		if (connsecmark && *connsecmark)
-			if (skb->secmark != *connsecmark)
-				skb->secmark = *connsecmark;
+			skb->secmark = *connsecmark;
 	}
 }
 
Index: linux-2.6.20-rc4/net/netfilter/xt_MARK.c
===================================================================
--- linux-2.6.20-rc4.orig/net/netfilter/xt_MARK.c
+++ linux-2.6.20-rc4/net/netfilter/xt_MARK.c
@@ -30,10 +30,7 @@ target_v0(struct sk_buff **pskb,
 	  const void *targinfo)
 {
 	const struct xt_mark_target_info *markinfo = targinfo;
-
-	if((*pskb)->mark != markinfo->mark)
-		(*pskb)->mark = markinfo->mark;
-
+	(*pskb)->mark = markinfo->mark;
 	return XT_CONTINUE;
 }
 
@@ -62,9 +59,7 @@ target_v1(struct sk_buff **pskb,
 		break;
 	}
 
-	if((*pskb)->mark != mark)
-		(*pskb)->mark = mark;
-
+	(*pskb)->mark = mark;
 	return XT_CONTINUE;
 }
 
Index: linux-2.6.20-rc4/net/netfilter/xt_NOTRACK.c
===================================================================
--- linux-2.6.20-rc4.orig/net/netfilter/xt_NOTRACK.c
+++ linux-2.6.20-rc4/net/netfilter/xt_NOTRACK.c
@@ -24,7 +24,7 @@ target(struct sk_buff **pskb,
 
 	/* Attach fake conntrack entry. 
 	   If there is a real ct entry correspondig to this packet, 
-	   it'll hang aroun till timing out. We don't deal with it
+	   it'll hang around till timing out. We don't deal with it
 	   for performance reasons. JK */
 	nf_ct_untrack(*pskb);
 	(*pskb)->nfctinfo = IP_CT_NEW;
Index: linux-2.6.20-rc4/net/netfilter/xt_SECMARK.c
===================================================================
--- linux-2.6.20-rc4.orig/net/netfilter/xt_SECMARK.c
+++ linux-2.6.20-rc4/net/netfilter/xt_SECMARK.c
@@ -47,9 +47,7 @@ static unsigned int target(struct sk_buf
 		BUG();
 	}
 
-	if ((*pskb)->secmark != secmark)
-		(*pskb)->secmark = secmark;
-
+	(*pskb)->secmark = secmark;
 	return XT_CONTINUE;
 }
 

Re: xt_MARK useless line

[Patrick McHardy]

Jan Engelhardt wrote:
> On Jan 10 2007 06:53, Patrick McHardy wrote:
> 
>>I vote for removing this (also in CONNMARK, CONNSECMARK, ...).
>>
> 
> 
> Not everywhere, xt_CONNMARK has a place where:
> 
> newmark = (*ctmark & ~markinfo->mask) | markinfo->mark;
> if (newmark != *ctmark) {
>     *ctmark = newmark;
> #if defined(CONFIG_IP_NF_CONNTRACK) || defined(CONFIG_IP_NF_CONNTRACK_MODULE)
>     ip_conntrack_event_cache(IPCT_MARK, *pskb);
> #else
>     nf_conntrack_event_cache(IPCT_MARK, *pskb);
> #endif
> }
> 
> Would it matter to call these cache functions it even if the mark did
> not really change?

Yes.

> 
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Remove unnecessary if() constructs before assignment.
> Plus one spelling fix, one brace indent fix.

Queued for 2.6.21 (without the spelling fix since that touched
a totally unrelated file), thanks.