Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH] dcache: better name hash function
From: Stephen Hemminger @ 2009-10-27  3:53 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Al Viro, Andrew Morton, Linus Torvalds, Octavian Purdila, netdev,
	linux-kernel
In-Reply-To: <4AE65EDE.8080605@gmail.com>

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

On Tue, 27 Oct 2009 03:45:50 +0100
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> Stephen Hemminger <shemminger@vyatta.com>, Al Viro a écrit :
> > --- a/include/linux/dcache.h	2009-10-26 14:58:45.220347300 -0700
> > +++ b/include/linux/dcache.h	2009-10-26 15:12:15.004160122 -0700
> > @@ -45,15 +45,28 @@ struct dentry_stat_t {
> >  };
> >  extern struct dentry_stat_t dentry_stat;
> >  
> > -/* Name hashing routines. Initial hash value */
> > -/* Hash courtesy of the R5 hash in reiserfs modulo sign bits */
> > -#define init_name_hash()		0
> > +/*
> > + * Fowler / Noll / Vo (FNV) Hash
> > + * see: http://www.isthe.com/chongo/tech/comp/fnv/
> > + */
> > +#ifdef CONFIG_64BIT
> > +#define FNV_PRIME  1099511628211ull
> > +#define FNV1_INIT  14695981039346656037ull
> > +#else
> > +#define FNV_PRIME  16777619u
> > +#define FNV1_INIT  2166136261u
> > +#endif
> > +
> > +#define init_name_hash()	FNV1_INIT
> >  
> > -/* partial hash update function. Assume roughly 4 bits per character */
> > +/* partial hash update function. */
> >  static inline unsigned long
> > -partial_name_hash(unsigned long c, unsigned long prevhash)
> > +partial_name_hash(unsigned char c, unsigned long prevhash)
> >  {
> > -	return (prevhash + (c << 4) + (c >> 4)) * 11;
> > +	prevhash ^= c;
> > +	prevhash *= FNV_PRIME;
> > +
> > +	return prevhash;
> >  }
> >  
> >  /*
> 
> OK, but thats strlen(name) X (long,long) multiplies.
> 
> I suspect you tested on recent x86_64 cpu.
> 
> Some arches might have slow multiplies, no ?
> 
> jhash() (and others) are optimized by compiler to use basic and fast operations.
> jhash operates on blocs of 12 chars per round, so it might be a pretty good choice once
> out-of-line (because its pretty large and full_name_hash() is now used by
> a lot of call sites)
> 
> Please provide your test program source, so that other can test on various arches.
> 
> Thanks

long on i386 is 32 bits so it is 32 bit multiply.  There is also an optimization
that uses shift and adds.




-- 

[-- Attachment #2: hashtest.tar.bz2 --]
[-- Type: application/x-bzip, Size: 7585 bytes --]

^ permalink raw reply

* RE: [PATCH] TI DaVinci EMAC: Minor macro related updates
From: Chaithrika U S @ 2009-10-27  3:57 UTC (permalink / raw)
  To: 'Jean-Christophe PLAGNIOL-VILLARD'
  Cc: netdev, davem, davinci-linux-open-source
In-Reply-To: <20091026220722.GA23415@game.jcrosoft.org>

On Tue, Oct 27, 2009 at 03:37:22, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 16:25 Thu 01 Oct     , Chaithrika U S wrote:
> > Use BIT for macro definitions wherever possible, remove
> > unused and redundant macros.
> > 
> > Signed-off-by: Chaithrika U S <chaithrika@ti.com>
> > ---
> > Applies to Linus' kernel tree
> do you plan to send a new version soon?
> 
> as the current DaVinci EMAC does not build on the v2.6.32-rc5
> 
> Best Regards,
> J.
> 

DaVinci EMAC builds and work fine on Linus' tree and DaVinci GIT tree.
Can you please provide more info regarding the errors you are seeing?

Regards, 
Chaithrika



^ permalink raw reply

* Re: [PATCH v2 8/8] Document future removal of sysctl_tcp_* options
From: Bill Fink @ 2009-10-27  5:09 UTC (permalink / raw)
  To: Gilad Ben-Yossef
  Cc: Eric Dumazet, William Allen Simpson, netdev, Ilpo Järvinen
In-Reply-To: <4AE5C579.2070104@codefidence.com>

On Mon, 26 Oct 2009, Gilad Ben-Yossef wrote:

> Bill Fink wrote:
> 
> >> OK. It really sounds like we should go with my first suggestion: global 
> >> sysctl based kill switches, just as we have now and in addition, the 
> >> ability to kill TCP options per route. The TCP option will be used if 
> >> and only if both kill switches (global and per route) are not set.
> >>     
> >
> > This wording is confusing.  The global kill switch not being set
> > really means that the sysctl is set.  And this assumes the per-route
> > default is not set.  Correct?
> >   
> Now it is my turn to get confused, because I didn't understand your 
> question :-)

My question was just if I understood you correctly, which apparently
I did.

> What I suggest is to leave the sysctl exactly as they are now:
> 
> - You leave them be (value of 1), the respective TCP option is 
> supported. This is the default.
> - You turn them off (write 0), the respective TCP option is not supported.
> 
> What I suggest to *add* is the following ability:
> 
> - If you have the TCP option support turned on (default, value of one), 
> you can turn support for the option for a specific route using a ip 
> route option.

Should be "turn" -> "turn off" above.

> Hope that made it clearer.

Yes.

> >> What we achieve is:
> >>
> >> 1. Global kill switches work exactly as they do now, whether you use the 
> >> new per route options or not, so backwards compatible.
> >>
> >> 2. In addition, if the global kill switch is not in effect, you can also 
> >> kill the options on a per route basis.
> >>
> >> I'm going to send third version of the patch to this effect, minus the 
> >> new remote DoS possibility that Ilpo pointed out and leaving the global 
> >> sysctl kill switches be.
> >>
> >> If you like it, please ACK ;-)
> >>     
> >
> > IIUC this doesn't seem right to me.  I believe the global setting
> > should be a default and the route specific an override.  Your scheme
> > would mean that if I set a global option to disable timestamps, then
> > I couldn't enable timestamps on specific routes using the per route
> > setting.
> >   
> Yes. You understand my intention perfectly.
> 
> Let me try to explain why I believe this is the correct behavior to 
> implement:
> 
> 1. This is the closest thing to what we have now. Today you write 0 to 
> the sysctl and that TCP option is turned off globally. Period.  My 
> suggestion leaves this behavior as is now regardless if you've used per 
> route settings. The other way make a very subtle change in the meaning 
> of writing 0 to the sysctl.

Continuing to support existing behavior is the most important
consideration for me, so your intention for the per route settings
doesn't cause me any major grief.  I initially thought the per route
setting as an override to a global default setting was more intuitive
than your method of it being a disable switch only, but my thinking
has since evolved (see below).

> I believe very subtle changes to meaning of long established interfaces 
> is bad way to go. It's better to change interfaces on users, but it is 
> even worse to maek something that they have long used do something just 
> slightly different.

I didn't see how my suggestion changed existing behavior.  If you
didn't use the per route settings everything remained as it was
(or so I initially thought).  And if you were using a new feature
then you needed to be aware of its semantics.

> 2. If the per route options needs to be "default, of or off" instead of 
> "on or off", we'd need to move from 1 bit to store the option to, well 
> 2s bit in theory, but probably 32 bits in practice, since we can't use 
> RTAX_FEATURES any longer.
> 
> Yes, we can invent RTAX_FEATURES_TWO_BITS or some such, but I'd say that 
> is ugly :-)

I only intended a single bit for the global default and per route
settings.  The global default would just be enabled (1) or disabled(0).
The per route setting would be inherited from the global default setting,
and also would be just enabled(1) or disabled(0).

Explicitly setting a per route setting to enabled(1) or disabled(0)
would override the initial default setting.

However I didn't fully work through all the ramifications of this
idea, and I now realize this would entail some changes to the existing
behavior of the global setting, which I agree is unacceptable.

I therefore now believe your latest proposal is a better approach
for maintaining backward compatibility and avoiding any nasty
unexpected surprises to existing working configurations.

> 3. I believe that the scenario of needing to set the support of a TCP 
> option globally off and just turn it on for a specific route is not very 
> likely to be needed and losing it is a small price to pay for 1 + 2.

I agree this is probably an uncommon scenario.

> > And it also doesn't seem to address Eric's scenario.  If I understand
> > his concern correctly, what seems to be needed is a third global
> > reset value (not calling it a setting since the actual global setting
> > wouldn't be changed), which would reset any per-route override settings
> > to the global default setting.
> >
> >   
> Well, I do not believe this is what Eric meant (Eric?) but if it is then 
> I fail to see why
> to require from the per route TCP options switches what is not required 
> of any other
> route specific option already existing, since AFAIK we don't have a 
> "reset to default values" to the other options already supported.

I won't try and speak for Eric anymore.

> Having said all that, I have no issue with re-spinning the patch with 
> your suggestion.
> I don't feel all that much which is the correct way- I just want to get 
> as much feedback as possible
> since I'm suggesting to add a new user interface options and we all know 
> it is very hard to back peddle
> on those, so I'm trying to make sure to get enough feedback to do it 
> right the firs time.
> 
> So any feedback from anyone regarding favorite interface? it seems each 
> person fancy a different one :-)

I haven't checked the details of your patch, but I am now fine with
the concepts of the patch as you most recently presented them.

						-Bill

^ permalink raw reply

* Re: [PATCH] dcache: better name hash function
From: Stephen Hemminger @ 2009-10-27  5:19 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Andrew Morton, Linus Torvalds, Octavian Purdila, netdev,
	linux-kernel, Al Viro
In-Reply-To: <9986527.24561256620662709.JavaMail.root@tahiti.vyatta.com>

One of the root causes of slowness in network usage
was my original choice of power of 2 for hash size, to avoid
a mod operation. It turns out if size is not a power of 2
the original algorithm works fairly well.

On slow cpu; with 10million entries and 211 hash size

Algorithm             Time       Ratio       Max   StdDev
string10             1.271871       1.00     47397   0.01
djb2                 1.406322       1.00     47452   0.12
SuperFastHash        1.422348       1.00     48400   1.99
string_hash31        1.424079       1.00     47437   0.08
jhash_string         1.459232       1.00     47954   1.01
sdbm                 1.499209       1.00     47499   0.22
fnv32                1.539341       1.00     47728   0.75
full_name_hash       1.556792       1.00     47412   0.04
string_hash17        1.719039       1.00     47413   0.05
pjw                  1.827365       1.00     47441   0.09
elf                  2.033545       1.00     47441   0.09
fnv64                2.199533       1.00     47666   0.53
crc                  5.705784       1.00     47913   0.95
md5_string           10.308376       1.00     47946   1.00
fletcher             1.418866       1.01     53189  18.65
adler32              2.842117       1.01     53255  18.79
kr_hash              1.175678       6.43    468517 507.44
xor                  1.114692      11.02    583189 688.96
lastchar             0.795316      21.10   1000000 976.02

How important is saving the one division, versus getting better
distribution.

^ permalink raw reply

* Re: [PATCH] dcache: better name hash function
From: David Miller @ 2009-10-27  5:24 UTC (permalink / raw)
  To: stephen.hemminger
  Cc: eric.dumazet, akpm, torvalds, opurdila, netdev, linux-kernel,
	viro
In-Reply-To: <19864844.24581256620784317.JavaMail.root@tahiti.vyatta.com>

From: Stephen Hemminger <stephen.hemminger@vyatta.com>
Date: Mon, 26 Oct 2009 22:19:44 -0700 (PDT)

> How important is saving the one division, versus getting better
> distribution.

80 cpu cycles or more on some processors.  Cheaper to use
jenkins with a power-of-2 sized hash.

^ permalink raw reply

* Re: [patch next 3/4] netxen: fix bonding support
From: Eric W. Biederman @ 2009-10-27  5:50 UTC (permalink / raw)
  To: Dhananjay Phadke; +Cc: netdev@vger.kernel.org
In-Reply-To: <7608421F3572AB4292BB2532AE89D5658B0B6E4F74@AVEXMB1.qlogic.org>

Dhananjay Phadke <dhananjay.phadke@qlogic.com> writes:

>> Yes.  That should prevent the null pointer deference.  Will it also
>> allow setting the mac address when the NIC is down?
>> 
>> Eric
>
> Yes, we do save new address in netdev->dev_addr.
> This is later on programmed in hardware when interface is brought up.

Yep. Thanks.  I just tested it and confirmed the crash is no longer there.

Eric

^ permalink raw reply

* Re: [PATCH] dcache: better name hash function
From: Eric Dumazet @ 2009-10-27  6:07 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: Andrew Morton, Linus Torvalds, Octavian Purdila, netdev,
	linux-kernel, Al Viro
In-Reply-To: <19864844.24581256620784317.JavaMail.root@tahiti.vyatta.com>

Stephen Hemminger a écrit :
> One of the root causes of slowness in network usage
> was my original choice of power of 2 for hash size, to avoid
> a mod operation. It turns out if size is not a power of 2
> the original algorithm works fairly well.

Interesting, but I suspect all users have power of 2 tables :(

> 
> On slow cpu; with 10million entries and 211 hash size
> 
>
> 
> How important is saving the one division, versus getting better
> distribution.


unsigned int fold1(unsigned hash)
{
	return hash % 211;
}

Compiler uses a reciprocal divide because of 211 being a constant.

And you also could try following that contains one multiply only,
and check if hash distribution properties are still OK

unsigned int fold2(unsigned hash)
{
	return ((unsigned long long)hash * 211) >> 32;
}

fold1:
        movl    4(%esp), %ecx
        movl    $-1689489505, %edx
        movl    %ecx, %eax
        mull    %edx
        shrl    $7, %edx
        imull   $211, %edx, %edx
        subl    %edx, %ecx
        movl    %ecx, %eax
        ret

fold2:
        movl    $211, %eax
        mull    4(%esp)
        movl    %edx, %eax
        ret



^ permalink raw reply

* RE: [patch next 3/4] netxen: fix bonding support
From: Dhananjay Phadke @ 2009-10-27  6:06 UTC (permalink / raw)
  To: Eric W. Biederman; +Cc: netdev@vger.kernel.org
In-Reply-To: <m1aazdl713.fsf@fess.ebiederm.org>

Eric,

Thanks for reporting and verifying.

-Dhananjay
________________________________________
From: Eric W. Biederman [ebiederm@xmission.com]
Sent: Monday, October 26, 2009 10:50 PM
To: Dhananjay Phadke
Cc: netdev@vger.kernel.org
Subject: Re: [patch next 3/4] netxen: fix bonding support

Dhananjay Phadke <dhananjay.phadke@qlogic.com> writes:

>> Yes.  That should prevent the null pointer deference.  Will it also
>> allow setting the mac address when the NIC is down?
>>
>> Eric
>
> Yes, we do save new address in netdev->dev_addr.
> This is later on programmed in hardware when interface is brought up.

Yep. Thanks.  I just tested it and confirmed the crash is no longer there.

Eric

^ permalink raw reply

* Re: [PATCH] dcache: better name hash function
From: Eric Dumazet @ 2009-10-27  6:50 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: Andrew Morton, Linus Torvalds, Octavian Purdila, netdev,
	linux-kernel, Al Viro
In-Reply-To: <4AE68E23.20205@gmail.com>

Eric Dumazet a écrit :
> unsigned int fold2(unsigned hash)
> {
> 	return ((unsigned long long)hash * 211) >> 32;
> }
> 

I tried this reciprocal thing with 511 and 1023 values and got on a PIII 550 MHz, gcc-3.3.2 :

# ./hashtest 100000 511 
jhash_string         0.033123       1.01       234   1.06
fnv32                0.033911       1.02       254   1.38
# ./hashtest 1000000 511
jhash_string         0.331155       1.00      2109   1.10
fnv32                0.359346       1.00      2151   1.65
# ./hashtest 10000000 511
jhash_string         3.383340       1.00     19985   1.03
fnv32                3.849359       1.00     20198   1.53

# ./hashtest 100000 1023
jhash_string         0.033123       1.03       134   1.01
fnv32                0.034260       1.03       142   1.32
# ./hashtest 1000000 1023
jhash_string         0.332329       1.00      1075   1.06
fnv32                0.422035       1.00      1121   1.59
# ./hashtest 10000000 1023
jhash_string         3.417559       1.00     10107   1.01
fnv32                3.747563       1.00     10223   1.35


511 value on 64bit, and 1023 on 32bit arches are nice because
hashsz * sizeof(pointer) <= 4096, wasting space for one pointer only.

Conclusion : jhash and 511/1023 hashsize for netdevices,
no divides, only one multiply for the fold.

^ permalink raw reply

* Re: [PATCH] net: Adjust softirq raising in __napi_schedule
From: Johannes Berg @ 2009-10-27  7:01 UTC (permalink / raw)
  To: Tilman Schmidt
  Cc: Jarek Poplawski, David Miller,
	hidave.darkstar-Re5JQEeQqe8AvxtiuMwx3w,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, tglx-hfZtesqFncYOwBW4kG4KsQ,
	linux-wireless-u79uwXL29TY76Z2rM5mHXA,
	linux-ppp-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA,
	paulus-eUNUBHrolfbYtjvyW6yDsg, Michael Buesch, Oliver Hartkopp
In-Reply-To: <4AE64441.7060008-ZTO5kqT2PaM@public.gmane.org>

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

On Tue, 2009-10-27 at 01:52 +0100, Tilman Schmidt wrote:

> > Any code (say ISDN code) that calls netif_rx() is clearly assuming to
> > always be running in (soft)irq context, otherwise it couldn't call
> > netif_rx() unconditionally. Agree so far?
> 
> Well, in fact I'm not sure. :-) All I know is that in the ISDN case, no
> such assumption is explicitly stated anywhere. (The code in question is
> called from the rcvcallb_skb() callback method which the hardware driver
> calls when data has been received, and the description of that method in
> Documentation/isdn/INTERFACE does not say anything about the context in
> which it may be called.) The relevant code in drivers/isdn/i4l/isdn_ppp.c
> is rather old, perhaps even older than softirqs and the netif_rx() /
> netif_rx_ni() split. (Bear in mind that we are talking about the old
> ISDN4Linux subsystem which initially didn't even make it into the 2.6
> series because it was considered obsolete.) It seems quite possible to me
> that just no one ever thought about that question.

Heh :)

> > So now if you change the ISDN code to call netif_rx_ni(), you've changed
> > the assumption that the ISDN code makes -- that it is running in
> > (soft)irq context. Therefore, you need to verify that this is actually a
> > correct change, which is what I tried to say.
> 
> Understood. However, the fact that the local_softirq_pending message is
> appearing would seem to indicate that this assumption was wrong to
> begin with, wouldn't it?

I thought it only recently started appearing with a new driver or
something, but I may have misunderstood you. Anyway, I think that sums
up the issue from my POV.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply

* Re: [PATCH] dcache: better name hash function
From: Eric Dumazet @ 2009-10-27  7:29 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: Andrew Morton, Linus Torvalds, Octavian Purdila, netdev,
	linux-kernel, Al Viro
In-Reply-To: <4AE69829.9070207@gmail.com>

Eric Dumazet a écrit :
> 
> 
> 511 value on 64bit, and 1023 on 32bit arches are nice because
> hashsz * sizeof(pointer) <= 4096, wasting space for one pointer only.
> 
> Conclusion : jhash and 511/1023 hashsize for netdevices,
> no divides, only one multiply for the fold.

Just forget about 511 & 1023, as power of two works too.

-> 512 & 1024 + jhash

Guess what, David already said this :)

^ permalink raw reply

* Re: Increasing TCP initial cwnd
From: Ilpo Järvinen @ 2009-10-27  7:38 UTC (permalink / raw)
  To: Yair Gottdenker; +Cc: Netdev
In-Reply-To: <fa3cddca0910260807q714a24fdp1ffae601bb6f67ba@mail.gmail.com>

On Mon, 26 Oct 2009, Yair Gottdenker wrote:

> I already had a look in it. Their changes that are related to changing
> the initial cwnd  (snd_cwnd) are in several places in the patch:

Right. I know what I've modified in that patch... ;-)

> 1. tcp_ipv4.c in function tcp_init_cwnd. since currently I am working
> with no metric this is irrelevant.

You managed to mix up this filename twice in a row in a different way :-). 
...That function resides in tcp_input.c instead. And changes in 
tcp_init_metrics are not completely irrelevant, even with no metrics it 
is executed as "no metrics" works a bit differenctly than you seem to 
think.

> 2. tcp_ipv4.c: in function tcp_v4_init_sock, which I applied
> 3. tcp_minisocks.c - which I applied
> . They patched kernel 2.6.18.8 which is very old and there were a lot
> of changes since that in the kernel tcp layer. I didn't test their
> patch but applying the same logic to 2.6.31.3 doesn't seems to change
> initial window size.
> 
> It is clear that in order to correctly make the change to the initial
> cwnd, one should apply changes to many more scenarios like setting the
> initial cwnd after idle time and more. I want to proceed step by step,
> first the naive changes to make it work.

Hmm, it now rings a bell... You cannot directly set initial window to a 
large value (or you can but that doesn't do anything on the wire) because 
you hit first the limit of receiver advertised window that applies 
auto-tuning (with a starting value was four IIRC).

-- 
 i.

^ permalink raw reply

* next tree for October 27:  znet build failure
From: Sachin Sant @ 2009-10-27  9:35 UTC (permalink / raw)
  To: linux-next; +Cc: Stephen Rothwell, netdev, David Miller
In-Reply-To: <20091027190850.9694fb39.sfr@canb.auug.org.au>

Fails to build on x86_32 with lots of errors related to znet

drivers/net/znet.c:107:29: error: wireless/i82593.h: No such file or directory
drivers/net/znet.c:133: error: field 'i593_init' has incomplete type
drivers/net/znet.c: In function 'znet_set_multicast_list':
drivers/net/znet.c:235: error: invalid application of 'sizeof' to incomplete type 'struct i82593_conf_block'
drivers/net/znet.c:247: error: dereferencing pointer to incomplete type
.....

Commit 879e9304... moved wireless/i82593.h to drivers/staging/wavelan/i82593.h

Thanks
-Sachin


-- 

---------------------------------
Sachin Sant
IBM Linux Technology Center
India Systems and Technology Labs
Bangalore, India
---------------------------------

^ permalink raw reply

* Re: performance regression in virtio-net in 2.6.32-rc4
From: Avi Kivity @ 2009-10-27  9:41 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: Rusty Russell, virtualization, kvm, netdev
In-Reply-To: <20091026184835.GB26473@redhat.com>

On 10/26/2009 08:48 PM, Michael S. Tsirkin wrote:
> Hi!
> I noticed a performance regression in virtio net: going from
> 2.6.31 to 2.6.32-rc4 I see this, for guest to host communication:
>
> Any tips on debugging this?
>    

Lacking better advice, a bisect can help as a last resort.  'git bisect 
start -- drivers/net drivers/virtio' will probably find it fastest.

-- 
error compiling committee.c: too many arguments to function


^ permalink raw reply

* Re: [PATCH] vlan: allow VLAN ID 0 to be used
From: Benny Amorsen @ 2009-10-27  9:52 UTC (permalink / raw)
  To: Eric Dumazet
In-Reply-To: <4AE5CAC6.4000604@gmail.com>

Eric Dumazet <eric.dumazet@gmail.com> writes:

> Here is the patch I cooked that permitted VLAN 0 to be used with tg3
> (and other HW accelerated vlan nics I suppose)
>
> [PATCH] vlan: allow VLAN ID 0 to be used
>
> We currently use a 16 bit field (vlan_tci) to store VLAN ID on a skb.
>
> 0 value is used a special value, meaning VLAN ID not set.
> This forbids use of VLAN ID 0

Are you sure you actually want to do this?

VLAN 0 IS special. Frames received on VLAN 0 should be treated just as
if they had no VLAN tag at all, except that they have an 802.1p value.

Sending frames with VLAN 0 should have something to do with whether
the sender wants to use 802.1p, which doesn't really have much to do
with VLAN's at all...

It would be nice if the unsuspecting user was at least warned that their
use of VLAN 0 is non-standard and may cause surprising results like
leakage into the "native" VLAN. That could be done in /sbin/ip or
/sbin/vconfig, of course.


/Benny


^ permalink raw reply

* Re: [PATCH] vlan: allow VLAN ID 0 to be used
From: Eric Dumazet @ 2009-10-27 10:02 UTC (permalink / raw)
  To: Benny Amorsen
  Cc: Gertjan Hofman, Matt Carlson, netdev@vger.kernel.org,
	Patrick McHardy, David S. Miller
In-Reply-To: <m3aazdb1ue.fsf@ursa.amorsen.dk>

Benny Amorsen a écrit :
> Eric Dumazet <eric.dumazet@gmail.com> writes:
> 
>> Here is the patch I cooked that permitted VLAN 0 to be used with tg3
>> (and other HW accelerated vlan nics I suppose)
>>
>> [PATCH] vlan: allow VLAN ID 0 to be used
>>
>> We currently use a 16 bit field (vlan_tci) to store VLAN ID on a skb.
>>
>> 0 value is used a special value, meaning VLAN ID not set.
>> This forbids use of VLAN ID 0
> 
> Are you sure you actually want to do this?
> 
> VLAN 0 IS special. Frames received on VLAN 0 should be treated just as
> if they had no VLAN tag at all, except that they have an 802.1p value.
> 
> Sending frames with VLAN 0 should have something to do with whether
> the sender wants to use 802.1p, which doesn't really have much to do
> with VLAN's at all...
> 
> It would be nice if the unsuspecting user was at least warned that their
> use of VLAN 0 is non-standard and may cause surprising results like
> leakage into the "native" VLAN. That could be done in /sbin/ip or
> /sbin/vconfig, of course.
> 

Quotting http://en.wikipedia.org/wiki/IEEE_802.1Q

VLAN Identifier (VID): a 12-bit field specifying the VLAN to which the frame belongs.
 A value of 0 means that the frame doesn't belong to any VLAN; in this case the 802.1Q
 tag specifies only a priority and is referred to as a priority tag.
A value of hex FFF is reserved for implementation use.
All other values may be used as VLAN identifiers, allowing up to 4094 VLANs


So we expect to generate a 802.1Q frame, even with a VID=0 field.
Before patch, device sends a non 802.1Q frame, which is not what was wanted by user.
(Maybe he wants to check its device/network is able to transport 1522 bytes frames, who knows...)

To use non tagged frames, user selects eth0 device, and to send tagged frames, he selects eth0.0


Now, maybe eth0 and eth0.0 should share same IP addresses, because incoming frame
with ID=0 tag should be received by eth0 device, but I am not sure standard requires this.


^ permalink raw reply

* Re: [PATCH 6/9] ser_gigaset: checkpatch cleanup
From: Karsten Keil @ 2009-10-27 10:20 UTC (permalink / raw)
  To: isdn4linux
  Cc: Tilman Schmidt, Joe Perches, netdev, linux-kernel, i4ldeveloper,
	Hansjoerg Lipp, David Miller
In-Reply-To: <4AE637D8.60809@imap.cc>

On Dienstag, 27. Oktober 2009 00:59:20 Tilman Schmidt wrote:
> Am 26.10.2009 01:54 schrieb Joe Perches:
> > On Sun, 2009-10-25 at 20:30 +0100, Tilman Schmidt wrote:
> >> Duly uglified as demanded by checkpatch.pl.
> >> diff --git a/drivers/isdn/gigaset/ser-gigaset.c
> >> b/drivers/isdn/gigaset/ser-gigaset.c index 3071a52..ac3409e 100644
> >> --- a/drivers/isdn/gigaset/ser-gigaset.c
> >> +++ b/drivers/isdn/gigaset/ser-gigaset.c
> >> @@ -164,9 +164,15 @@ static void gigaset_modem_fill(unsigned long data)
> >>  {
> >>  	struct cardstate *cs = (struct cardstate *) data;
> >>  	struct bc_state *bcs;
> >> +	struct sk_buff *nextskb;
> >>  	int sent = 0;
> >>
> >> -	if (!cs || !(bcs = cs->bcs)) {
> >> +	if (!cs) {
> >> +		gig_dbg(DEBUG_OUTPUT, "%s: no cardstate", __func__);
> >> +		return;
> >> +	}
> >> +	bcs = cs->bcs;
> >> +	if (!bcs) {
> >>  		gig_dbg(DEBUG_OUTPUT, "%s: no cardstate", __func__);
> >>  		return;
> >> 	}
> >
> > perhaps:
> > 	if (!cs || !cs->bcs) {
> > 		gig_dbg(DEBUG_OUTPUT, "%s: no cardstate", __func__);
> > 		return;
> > 	}
> > 	bcs = cs->bcs;
>
> That would evaluate cs->bcs twice, and is also, in my experience,

gcc should handle this subsequent  double evaluation  well enough.

> significantly more prone to easily overlooked typos which result in
> checking a different pointer in the if statement than the one that's
> actually used in the subsequent assignment.
>

Yes this may happen, but more often a = in if statements  should be a ==.

The kernel code style says only one statement per line, which implies  no 
assignments in if statements, so we should follow this.
The checkpatch.pl script  complain about these issues.

Yes sometimes in the past (while preparing some old code for kernel submit)
I was not very happy about all these rules, it take lot  time to reach zero 
reports from checkpatch.
But it make lot of sense in the long term, currently I have to debug code 
written without any code style, it is really, really painful and I need 5-10 
times  more time for simple understanding the basic function of the code.

Karsten

^ permalink raw reply

* Re: [PATCH 5/5] ONLY-APPLY-IF-STILL-FAILING Revert 373c0a7e, 8aa7e847: Fix congestion_wait() sync/async vs read/write confusion
From: Frans Pop @ 2009-10-27 10:29 UTC (permalink / raw)
  To: KOSAKI Motohiro
  Cc: Mel Gorman, Jiri Kosina, Sven Geggus, Karol Lewandowski,
	Tobias Oetiker, Rafael J. Wysocki, David Miller, Reinette Chatre,
	Kalle Valo, David Rientjes, Mohamed Abbas, Jens Axboe,
	John W. Linville, Pekka Enberg, Bartlomiej Zolnierkiewicz,
	Greg Kroah-Hartman, Stephan von Krawczynski, Kernel Testers List,
	netdev, linux-kernel, linux-mm@kvack.org
In-Reply-To: <20091026235628.2F7B.A69D9226@jp.fujitsu.com>

On Tuesday 27 October 2009, KOSAKI Motohiro wrote:
> Oops. no, please no.
> 8aa7e847 is regression fixing commit. this revert indicate the
> regression occur again.
> if we really need to revert it, we need to revert 1faa16d2287 too.
> however, I doubt this commit really cause regression to iwlan. IOW,
> I agree Jens.

This is not intended as a patch for mainline, but just as a test to see if 
it improves things. It may be a regression fix, but it also creates a 
significant change in behavior during swapping in my test case.
If a fix is needed, it will probably by different from this revert.
Please read: http://lkml.org/lkml/2009/10/26/510.

This mail has some data: http://lkml.org/lkml/2009/10/26/455.

> I hope to try reproduce this problem on my test environment. Can anyone
> please explain reproduce way?

Please see my mails in this thread for bug #14141: 
http://thread.gmane.org/gmane.linux.kernel/896714

You will probably need to read some of them to understand the context of 
the two mails linked above.

The most relevant ones are (all from the same thread; not sure why gmane 
gives such weird links):
http://article.gmane.org/gmane.linux.kernel.mm/39909
http://article.gmane.org/gmane.linux.kernel.kernel-testers/7228
http://article.gmane.org/gmane.linux.kernel.kernel-testers/7165

> Is special hardware necessary?

Not special hardware, but you may need an encrypted partition and NFS; the 
test may need to be modified according to the amount of memory you have.
I think it should be possible to reproduce the freezes I see while ignoring 
the SKB allocation errors as IMO those are just a symptom, not the cause.
So you should not need wireless.

The severity of the freezes during my test often increases if the test is 
repeated (without rebooting).

Cheers,
FJP

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply

* Re: [PATCH 0/5] Candidate fix for increased number of GFP_ATOMIC failures V2
From: Mel Gorman @ 2009-10-27 10:40 UTC (permalink / raw)
  To: reinette chatre
  Cc: Frans Pop, Jiri Kosina, Sven Geggus, Karol Lewandowski,
	Tobias Oetiker, Rafael J. Wysocki, David Miller, Kalle Valo,
	David Rientjes, KOSAKI Motohiro, Abbas, Mohamed, Jens Axboe,
	John W. Linville, Pekka Enberg, Bartlomiej Zolnierkiewicz,
	Greg Kroah-Hartman, Stephan von Krawczynski, Kernel Testers List,
	netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-mm-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org
In-Reply-To: <1256226219.21134.1493.camel@rc-desk>

On Thu, Oct 22, 2009 at 08:43:38AM -0700, reinette chatre wrote:
> On Thu, 2009-10-22 at 07:22 -0700, Mel Gorman wrote:
> > [Bug #14141] order 2 page allocation failures in iwlagn
> > 	Commit 4752c93c30441f98f7ed723001b1a5e3e5619829 introduced GFP_ATOMIC
> > 	allocations within the wireless driver. This has caused large numbers
> > 	of failure reports to occur as reported by Frans Pop. Fixing this
> > 	requires changes to the driver if it wants to use GFP_ATOMIC which
> > 	is in the hands of Mohamed Abbas and Reinette Chatre. However,
> > 	it is very likely that it has being compounded by core mm changes
> > 	that this series is aimed at.
> 
> Driver has been changed to allocate paged skb for its receive buffers.
> This reduces amount of memory needed from order-2 to order-1. This work
> is significant and will thus be in 2.6.33. 
> 

What do you want to do for -stable in 2.6.31?

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

^ permalink raw reply

* phy_connect_direct problem
From: Kristoffer Glembo @ 2009-10-27 10:28 UTC (permalink / raw)
  To: netdev

Hi,

I'm in the process of converting an ethernet driver to using the PHY 
layer, but ran into a problem where my adjust_state handler was never 
called and the link did not go up ...

I use phy_connect to connect the PHY device with my ethernet driver. In 
phy_connect_direct I found the following code: 	

phy_prepare_link(phydev, handler);
phy_start_machine(phydev, NULL);

So first the adjust_state is set to handler by phy_prepare_link and then 
it is overwritten with the NULL parameter passed to phy_start_machine.

Maybe I'm abusing the PHY layer somehow since there are plenty of other 
drivers using this interface. However the patch below solved the issue 
for me.

Best regards,
Kristoffer Glembo


---
  drivers/net/phy/phy_device.c |    3 +--
  1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index b10fedd..d27ca80 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -311,8 +311,7 @@ int phy_connect_direct(struct net_device *dev, 
struct phy_device *phydev,
  	if (rc)
  		return rc;

-	phy_prepare_link(phydev, handler);
-	phy_start_machine(phydev, NULL);
+	phy_start_machine(phydev, handler);
  	if (phydev->irq > 0)
  		phy_start_interrupts(phydev);

-- 
1.5.2.2

^ permalink raw reply related

* netlink: symantics for NLM_F_MULTI and NLMSG_DONE ?
From: Roger Willcocks @ 2009-10-27 11:43 UTC (permalink / raw)
  To: netdev

The netlink rfc (3549) says that in multi-part messages (messages split
over multiple netlink packets), the first and all following headers 
have the NLM_F_MULTI flag set except for the last header which has the
header type NLMSG_DONE.

afaict NLMSG_DONE is redundant, since one could simply not set the
NLM_F_MULTI flag for the final header; and if a multi-part message
can't cross a datagram boundary - which is assumed by e.g. the reading
example in
http://www.kernel.org/doc/man-pages/online/pages/man7/netlink.7.html
- NLMSG_DONE is redundant too.

And the rfc doesn't say what NLMSG_DONE /and/ NLM_F_MULTI should mean,
although net/netlink/af_netlink.c always sets both.

So, what are the symantics for NLM_F_MULTI and NLMSG_DONE?



^ permalink raw reply

* [net-next-2.6 v4 0/4] TCPCT part1: cookie option exchange
From: William Allen Simpson @ 2009-10-27 12:11 UTC (permalink / raw)
  To: Linux Kernel Network Developers

Updated to patch cleanly with recent commits.

Changes from previous version:
* Strongly type otherwise void parameter with empty struct.
* Combined two shorter patches (b + c), dropping update of 2 drivers,
   as nobody has gotten back to me about testing results.
* Added a little Documentation for new sysctl.

^ permalink raw reply

* [net-next-2.6 PATCH v4 1/3] TCPCT part 1a: add request_values parameter for sending SYNACK
From: William Allen Simpson @ 2009-10-27 12:15 UTC (permalink / raw)
  To: Linux Kernel Network Developers
In-Reply-To: <4AE6E35C.2050101@gmail.com>

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

Add optional function parameters associated with sending SYNACK.
These parameters are not needed after sending SYNACK, and are not
used for retransmission.  Avoids extending struct tcp_request_sock,
and avoids allocating kernel memory.

Also affects DCCP as it uses common struct request_sock_ops,
but this parameter is currently reserved for future use.

Signed-off-by: William.Allen.Simpson@gmail.com
---
  include/net/request_sock.h      |    8 +++++++-
  include/net/tcp.h               |    1 +
  net/dccp/ipv4.c                 |    5 +++--
  net/dccp/ipv6.c                 |    5 +++--
  net/dccp/minisocks.c            |    2 +-
  net/ipv4/inet_connection_sock.c |    2 +-
  net/ipv4/tcp_ipv4.c             |   11 +++++++----
  net/ipv4/tcp_minisocks.c        |    2 +-
  net/ipv4/tcp_output.c           |    1 +
  net/ipv6/tcp_ipv6.c             |   14 +++++++-------
  10 files changed, 32 insertions(+), 19 deletions(-)


[-- Attachment #2: TCPCT+1a4.patch --]
[-- Type: text/plain, Size: 7357 bytes --]

diff --git a/include/net/request_sock.h b/include/net/request_sock.h
index c719084..c9b50eb 100644
--- a/include/net/request_sock.h
+++ b/include/net/request_sock.h
@@ -27,13 +27,19 @@ struct sk_buff;
 struct dst_entry;
 struct proto;
 
+/* empty to "strongly type" an otherwise void parameter.
+ */
+struct request_values {
+};
+
 struct request_sock_ops {
 	int		family;
 	int		obj_size;
 	struct kmem_cache	*slab;
 	char		*slab_name;
 	int		(*rtx_syn_ack)(struct sock *sk,
-				       struct request_sock *req);
+				       struct request_sock *req,
+				       struct request_values *rvp);
 	void		(*send_ack)(struct sock *sk, struct sk_buff *skb,
 				    struct request_sock *req);
 	void		(*send_reset)(struct sock *sk,
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 03a49c7..4abdb4d 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -443,6 +443,7 @@ extern int			tcp_connect(struct sock *sk);
 
 extern struct sk_buff *		tcp_make_synack(struct sock *sk,
 						struct dst_entry *dst,
+						struct request_values *rvp,
 						struct request_sock *req);
 
 extern int			tcp_disconnect(struct sock *sk, int flags);
diff --git a/net/dccp/ipv4.c b/net/dccp/ipv4.c
index 00028d4..8d4a88a 100644
--- a/net/dccp/ipv4.c
+++ b/net/dccp/ipv4.c
@@ -477,7 +477,8 @@ static struct dst_entry* dccp_v4_route_skb(struct net *net, struct sock *sk,
 	return &rt->u.dst;
 }
 
-static int dccp_v4_send_response(struct sock *sk, struct request_sock *req)
+static int dccp_v4_send_response(struct sock *sk, struct request_sock *req,
+				 struct request_values *rv_unused)
 {
 	int err = -1;
 	struct sk_buff *skb;
@@ -626,7 +627,7 @@ int dccp_v4_conn_request(struct sock *sk, struct sk_buff *skb)
 	dreq->dreq_iss	   = dccp_v4_init_sequence(skb);
 	dreq->dreq_service = service;
 
-	if (dccp_v4_send_response(sk, req))
+	if (dccp_v4_send_response(sk, req, NULL))
 		goto drop_and_free;
 
 	inet_csk_reqsk_queue_hash_add(sk, req, DCCP_TIMEOUT_INIT);
diff --git a/net/dccp/ipv6.c b/net/dccp/ipv6.c
index 6d89f9f..f64981e 100644
--- a/net/dccp/ipv6.c
+++ b/net/dccp/ipv6.c
@@ -241,7 +241,8 @@ out:
 }
 
 
-static int dccp_v6_send_response(struct sock *sk, struct request_sock *req)
+static int dccp_v6_send_response(struct sock *sk, struct request_sock *req,
+				 struct request_values *rv_unused)
 {
 	struct inet6_request_sock *ireq6 = inet6_rsk(req);
 	struct ipv6_pinfo *np = inet6_sk(sk);
@@ -468,7 +469,7 @@ static int dccp_v6_conn_request(struct sock *sk, struct sk_buff *skb)
 	dreq->dreq_iss	   = dccp_v6_init_sequence(skb);
 	dreq->dreq_service = service;
 
-	if (dccp_v6_send_response(sk, req))
+	if (dccp_v6_send_response(sk, req, NULL))
 		goto drop_and_free;
 
 	inet6_csk_reqsk_queue_hash_add(sk, req, DCCP_TIMEOUT_INIT);
diff --git a/net/dccp/minisocks.c b/net/dccp/minisocks.c
index 5ca49ce..af226a0 100644
--- a/net/dccp/minisocks.c
+++ b/net/dccp/minisocks.c
@@ -184,7 +184,7 @@ struct sock *dccp_check_req(struct sock *sk, struct sk_buff *skb,
 			 * counter (backoff, monitored by dccp_response_timer).
 			 */
 			req->retrans++;
-			req->rsk_ops->rtx_syn_ack(sk, req);
+			req->rsk_ops->rtx_syn_ack(sk, req, NULL);
 		}
 		/* Network Duplicate, discard packet */
 		return NULL;
diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index f6a0af7..b81d3c9 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -504,7 +504,7 @@ void inet_csk_reqsk_queue_prune(struct sock *parent,
 			if (time_after_eq(now, req->expires)) {
 				if ((req->retrans < thresh ||
 				     (inet_rsk(req)->acked && req->retrans < max_retries))
-				    && !req->rsk_ops->rtx_syn_ack(parent, req)) {
+				    && !req->rsk_ops->rtx_syn_ack(parent, req, NULL)) {
 					unsigned long timeo;
 
 					if (req->retrans++ == 0)
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index a4a3390..6edc5e2 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -743,6 +743,7 @@ static void tcp_v4_reqsk_send_ack(struct sock *sk, struct sk_buff *skb,
  *	socket.
  */
 static int __tcp_v4_send_synack(struct sock *sk, struct request_sock *req,
+				struct request_values *rvp,
 				struct dst_entry *dst)
 {
 	const struct inet_request_sock *ireq = inet_rsk(req);
@@ -753,7 +754,7 @@ static int __tcp_v4_send_synack(struct sock *sk, struct request_sock *req,
 	if (!dst && (dst = inet_csk_route_req(sk, req)) == NULL)
 		return -1;
 
-	skb = tcp_make_synack(sk, dst, req);
+	skb = tcp_make_synack(sk, dst, rvp, req);
 
 	if (skb) {
 		struct tcphdr *th = tcp_hdr(skb);
@@ -774,9 +775,10 @@ static int __tcp_v4_send_synack(struct sock *sk, struct request_sock *req,
 	return err;
 }
 
-static int tcp_v4_send_synack(struct sock *sk, struct request_sock *req)
+static int tcp_v4_send_synack(struct sock *sk, struct request_sock *req,
+			      struct request_values *rvp)
 {
-	return __tcp_v4_send_synack(sk, req, NULL);
+	return __tcp_v4_send_synack(sk, req, rvp, NULL);
 }
 
 /*
@@ -1334,7 +1336,8 @@ int tcp_v4_conn_request(struct sock *sk, struct sk_buff *skb)
 	}
 	tcp_rsk(req)->snt_isn = isn;
 
-	if (__tcp_v4_send_synack(sk, req, dst) || want_cookie)
+	if (__tcp_v4_send_synack(sk, req, NULL, dst) ||
+	    want_cookie)
 		goto drop_and_free;
 
 	inet_csk_reqsk_queue_hash_add(sk, req, TCP_TIMEOUT_INIT);
diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index e320afe..8819882 100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c
@@ -537,7 +537,7 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
 		 * Enforce "SYN-ACK" according to figure 8, figure 6
 		 * of RFC793, fixed by RFC1122.
 		 */
-		req->rsk_ops->rtx_syn_ack(sk, req);
+		req->rsk_ops->rtx_syn_ack(sk, req, NULL);
 		return NULL;
 	}
 
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 2e2eb74..79042f4 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -2219,6 +2219,7 @@ int tcp_send_synack(struct sock *sk)
 
 /* Prepare a SYN-ACK. */
 struct sk_buff *tcp_make_synack(struct sock *sk, struct dst_entry *dst,
+				struct request_values *rvp,
 				struct request_sock *req)
 {
 	struct inet_request_sock *ireq = inet_rsk(req);
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index c54ec36..3960d72 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -461,7 +461,8 @@ out:
 }
 
 
-static int tcp_v6_send_synack(struct sock *sk, struct request_sock *req)
+static int tcp_v6_send_synack(struct sock *sk, struct request_sock *req,
+			      struct request_values *rvp)
 {
 	struct inet6_request_sock *treq = inet6_rsk(req);
 	struct ipv6_pinfo *np = inet6_sk(sk);
@@ -499,7 +500,7 @@ static int tcp_v6_send_synack(struct sock *sk, struct request_sock *req)
 	if ((err = xfrm_lookup(sock_net(sk), &dst, &fl, sk, 0)) < 0)
 		goto done;
 
-	skb = tcp_make_synack(sk, dst, req);
+	skb = tcp_make_synack(sk, dst, rvp, req);
 	if (skb) {
 		struct tcphdr *th = tcp_hdr(skb);
 
@@ -1243,13 +1244,12 @@ static int tcp_v6_conn_request(struct sock *sk, struct sk_buff *skb)
 
 	security_inet_conn_request(sk, skb, req);
 
-	if (tcp_v6_send_synack(sk, req))
+	if (tcp_v6_send_synack(sk, req, NULL) ||
+	    want_cookie)
 		goto drop;
 
-	if (!want_cookie) {
-		inet6_csk_reqsk_queue_hash_add(sk, req, TCP_TIMEOUT_INIT);
-		return 0;
-	}
+	inet6_csk_reqsk_queue_hash_add(sk, req, TCP_TIMEOUT_INIT);
+	return 0;
 
 drop:
 	if (req)
-- 
1.6.0.4


^ permalink raw reply related

* [net-next-2.6 PATCH v4 2/3] TCPCT part 1b: sysctl_tcp_cookie_size, socket option TCP_COOKIE_TRANSACTIONS, functions
From: William Allen Simpson @ 2009-10-27 12:18 UTC (permalink / raw)
  To: Linux Kernel Network Developers
In-Reply-To: <4AE6E35C.2050101@gmail.com>

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

Define sysctl (tcp_cookie_size) to turn on and off the cookie option
default globally, instead of a compiled configuration option.

Define per socket option (TCP_COOKIE_TRANSACTIONS) for setting constant
data values, retrieving variable cookie values, and other facilities.

Redefine two TCP header functions to accept TCP header pointer.
When subtracting, return signed int to allow error checking.

Move inline tcp_clear_options() unchanged from net/tcp.h to linux/tcp.h,
near its corresponding struct tcp_options_received (prior to changes).

This is a straightforward re-implementation of an earlier (year-old)
patch that no longer applies cleanly, with permission of the original
author (Adam Langley).  The patch was previously reviewed:

    http://thread.gmane.org/gmane.linux.network/102586

These functions will also be used in subsequent patches that implement
additional features.

Signed-off-by: William.Allen.Simpson@gmail.com
---
  Documentation/networking/ip-sysctl.txt |    8 +++++
  include/linux/tcp.h                    |   47 +++++++++++++++++++++++++++++++-
  include/net/tcp.h                      |    6 +---
  net/ipv4/sysctl_net_ipv4.c             |    8 +++++
  net/ipv4/tcp_output.c                  |    8 +++++
  5 files changed, 71 insertions(+), 6 deletions(-)


[-- Attachment #2: TCPCT+1b4.patch --]
[-- Type: text/plain, Size: 5734 bytes --]

diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt
index a0e134d..d47c000 100644
--- a/Documentation/networking/ip-sysctl.txt
+++ b/Documentation/networking/ip-sysctl.txt
@@ -164,6 +164,14 @@ tcp_congestion_control - STRING
 	additional choices may be available based on kernel configuration.
 	Default is set as part of kernel configuration.
 
+tcp_cookie_size - INTEGER
+	Default size of TCP Cookie Transactions (TCPCT) option, that may be
+	overridden on a per socket basis by the TCPCT socket option.
+	Values greater than the maximum (16) are interpreted as the maximum.
+	Values greater than zero and less than the minimum (8) are interpreted
+	as the minimum.
+	Default: 0 (off).
+
 tcp_dsack - BOOLEAN
 	Allows TCP to send "duplicate" SACKs.
 
diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index 61723a7..6fd59d1 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -96,6 +96,7 @@ enum {
 #define TCP_QUICKACK		12	/* Block/reenable quick acks */
 #define TCP_CONGESTION		13	/* Congestion control algorithm */
 #define TCP_MD5SIG		14	/* TCP MD5 Signature (RFC2385) */
+#define TCP_COOKIE_TRANSACTIONS	15	/* TCP Cookie Transactions */
 
 #define TCPI_OPT_TIMESTAMPS	1
 #define TCPI_OPT_SACK		2
@@ -170,6 +171,34 @@ struct tcp_md5sig {
 	__u8	tcpm_key[TCP_MD5SIG_MAXKEYLEN];		/* key (binary) */
 };
 
+/* for TCP_COOKIE_TRANSACTIONS (TCPCT) socket option */
+#define TCP_COOKIE_MAX		16		/* 128-bits */
+#define TCP_COOKIE_MIN		 8		/*  64-bits */
+#define TCP_COOKIE_PAIR_SIZE	(2*TCP_COOKIE_MAX)
+
+#define TCP_S_DATA_MAX		64U		/* after TCP+IP options */
+#define TCP_S_DATA_MSS_DEFAULT	536U		/* default MSS (RFC1122) */
+
+/* Flags for both getsockopt and setsockopt */
+#define TCP_COOKIE_IN_ALWAYS	(1 << 0)	/* Discard SYN without cookie */
+#define TCP_COOKIE_OUT_NEVER	(1 << 1)	/* Prohibit outgoing cookies,
+						 * supercedes everything else. */
+#define TCP_EXTEND_TIMESTAMP	(1 << 4)	/* Initiate 64-bit timestamps */
+
+/* Flags for getsockopt */
+#define TCP_S_DATA_IN		(1 << 2)	/* Was data received? */
+#define TCP_S_DATA_OUT		(1 << 3)	/* Was data sent? */
+
+/* TCP_COOKIE_TRANSACTIONS data */
+struct tcp_cookie_transactions {
+	__u16	tcpct_flags;			/* see above */
+	__u8	__tcpct_pad1;			/* zero */
+	__u8	tcpct_cookie_desired;		/* bytes */
+	__u16	tcpct_s_data_desired;		/* bytes of variable data */
+	__u16	tcpct_used;			/* bytes in value */
+	__u8	tcpct_value[TCP_S_DATA_MSS_DEFAULT];
+};
+
 #ifdef __KERNEL__
 
 #include <linux/skbuff.h>
@@ -193,6 +222,17 @@ static inline unsigned int tcp_optlen(const struct sk_buff *skb)
 	return (tcp_hdr(skb)->doff - 5) * 4;
 }
 
+static inline unsigned int tcp_header_len_th(const struct tcphdr *th)
+{
+	return th->doff * 4;
+}
+
+/* When doff is bad, this could be negative. */
+static inline int tcp_option_len_th(const struct tcphdr *th)
+{
+	return (int)(th->doff * 4) - sizeof(*th);
+}
+
 /* This defines a selective acknowledgement block. */
 struct tcp_sack_block_wire {
 	__be32	start_seq;
@@ -223,6 +263,11 @@ struct tcp_options_received {
 	u16	mss_clamp;	/* Maximal mss, negotiated at connection setup */
 };
 
+static inline void tcp_clear_options(struct tcp_options_received *rx_opt)
+{
+	rx_opt->tstamp_ok = rx_opt->sack_ok = rx_opt->wscale_ok = rx_opt->snd_wscale = 0;
+}
+
 /* This is the max number of SACKS that we'll generate and process. It's safe
  * to increse this, although since:
  *   size = TCPOLEN_SACK_BASE_ALIGNED (4) + n * TCPOLEN_SACK_PERBLOCK (8)
@@ -431,6 +476,6 @@ static inline struct tcp_timewait_sock *tcp_twsk(const struct sock *sk)
 	return (struct tcp_timewait_sock *)sk;
 }
 
-#endif
+#endif	/* __KERNEL__ */
 
 #endif	/* _LINUX_TCP_H */
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 4abdb4d..142f32e 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -237,6 +237,7 @@ extern int sysctl_tcp_base_mss;
 extern int sysctl_tcp_workaround_signed_windows;
 extern int sysctl_tcp_slow_start_after_idle;
 extern int sysctl_tcp_max_ssthresh;
+extern int sysctl_tcp_cookie_size;
 
 extern atomic_t tcp_memory_allocated;
 extern struct percpu_counter tcp_sockets_allocated;
@@ -343,11 +344,6 @@ static inline void tcp_dec_quickack_mode(struct sock *sk,
 
 extern void tcp_enter_quickack_mode(struct sock *sk);
 
-static inline void tcp_clear_options(struct tcp_options_received *rx_opt)
-{
- 	rx_opt->tstamp_ok = rx_opt->sack_ok = rx_opt->wscale_ok = rx_opt->snd_wscale = 0;
-}
-
 #define	TCP_ECN_OK		1
 #define	TCP_ECN_QUEUE_CWR	2
 #define	TCP_ECN_DEMAND_CWR	4
diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
index 2dcf04d..3422c54 100644
--- a/net/ipv4/sysctl_net_ipv4.c
+++ b/net/ipv4/sysctl_net_ipv4.c
@@ -714,6 +714,14 @@ static struct ctl_table ipv4_table[] = {
 	},
 	{
 		.ctl_name	= CTL_UNNUMBERED,
+		.procname	= "tcp_cookie_size",
+		.data		= &sysctl_tcp_cookie_size,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec
+	},
+	{
+		.ctl_name	= CTL_UNNUMBERED,
 		.procname	= "udp_mem",
 		.data		= &sysctl_udp_mem,
 		.maxlen		= sizeof(sysctl_udp_mem),
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 79042f4..bcfbe41 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -59,6 +59,14 @@ int sysctl_tcp_base_mss __read_mostly = 512;
 /* By default, RFC2861 behavior.  */
 int sysctl_tcp_slow_start_after_idle __read_mostly = 1;
 
+#ifdef CONFIG_SYSCTL
+/* By default, let the user enable it. */
+int sysctl_tcp_cookie_size __read_mostly = 0;
+#else
+int sysctl_tcp_cookie_size __read_mostly = TCP_COOKIE_MAX;
+#endif
+
+
 /* Account for new data that has been sent to the network. */
 static void tcp_event_new_data_sent(struct sock *sk, struct sk_buff *skb)
 {
-- 
1.6.0.4


^ permalink raw reply related

* Re: [PATCH 1/5] page allocator: Always wake kswapd when restarting an allocation attempt after direct reclaim failed
From: Mel Gorman @ 2009-10-27 12:27 UTC (permalink / raw)
  To: KOSAKI Motohiro
  Cc: David Rientjes, Frans Pop, Jiri Kosina, Sven Geggus,
	Karol Lewandowski, Tobias Oetiker, Rafael J. Wysocki,
	David Miller, Reinette Chatre, Kalle Valo, Mohamed Abbas,
	Jens Axboe, John W. Linville, Pekka Enberg,
	Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman,
	Stephan von Krawczynski, Kernel Testers List, netdev,
	linux-kernel, linux-mm@kvack.org
In-Reply-To: <20091026222159.2F72.A69D9226@jp.fujitsu.com>

On Tue, Oct 27, 2009 at 11:42:55AM +0900, KOSAKI Motohiro wrote:
> > On Mon, 26 Oct 2009, KOSAKI Motohiro wrote:
> > 
> > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > > index bf72055..5a27896 100644
> > > --- a/mm/page_alloc.c
> > > +++ b/mm/page_alloc.c
> > > @@ -1899,6 +1899,12 @@ rebalance:
> > >  	if (should_alloc_retry(gfp_mask, order, pages_reclaimed)) {
> > >  		/* Wait for some write requests to complete then retry */
> > >  		congestion_wait(BLK_RW_ASYNC, HZ/50);
> > > +
> > > +		/*
> > > +		 * While we wait congestion wait, Amount of free memory can
> > > +		 * be changed dramatically. Thus, we kick kswapd again.
> > > +		 */
> > > +		wake_all_kswapd(order, zonelist, high_zoneidx);
> > >  		goto rebalance;
> > >  	}
> > >  
> > 
> > We're blocking to finish writeback of the directly reclaimed memory, why 
> > do we need to wake kswapd afterwards?
> 
> the same reason of "goto restart" case. that's my intention.
> if following scenario occur, it is equivalent that we didn't call wake_all_kswapd().
> 
>   1. call congestion_wait()
>   2. kswapd reclaimed lots memory and sleep
>   3. another task consume lots memory
>   4. wakeup from congestion_wait()
> 
> IOW, if we falled into __alloc_pages_slowpath(), we naturally expect
> next page_alloc() don't fall into slowpath. however if kswapd end to
> its work too early, this assumption isn't true.
> 
> Is this too pessimistic assumption?
> 

hmm.

The reason it's not woken in both cases a second time was to match the
behaviour of 2.6.30.  If the direct reclaimer goes asleep and another task
consumes the memory the direct reclaimer freed then the greedy process should
kick kswapd back awake again as free memory goes below the low watermark.

However, if the greedy process was allocating order-0, it's possible that
the watermarks for order-0 are being met leaving kswapd alone where as the
high-order ones are not leaving kswapd to go back asleep or to reclaim at
the wrong order.

It's a functional change but I can add it to the list of things to
consider. Thanks

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ 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