* Re: [PATCH] e1000, e1000e valid-addr fixes
From: Jeff Garzik @ 2007-11-01 18:47 UTC (permalink / raw)
To: Kok, Auke; +Cc: David Miller, netdev, davej, ajax, linux-kernel
In-Reply-To: <472A1526.8070208@intel.com>
Kok, Auke wrote:
> David Miller wrote:
>> From: Jeff Garzik <jeff@garzik.org>
>> Date: Tue, 23 Oct 2007 22:20:30 -0400
>>
>>> David Miller wrote:
>>>> From: Jeff Garzik <jeff@garzik.org>
>>>> Date: Tue, 23 Oct 2007 21:03:36 -0400
>>>>
>>>>> I'm wondering if there is a way to avoid adding
>>>>>
>>>>> if (!is_valid_ether_addr(dev->dev_addr))
>>>>> return -EINVAL;
>>>>>
>>>>> to every ethernet driver's ->open() hook.
>>>> The first idea I get is:
>>>>
>>>> 1) Create netdev->validate_dev_addr().
>>>>
>>>> 2) If it exists, invoke it before ->open(), abort
>>>> and return if any errors signaled.
>>>>
>>>> etherdev init hooks up a function that does the above
>>>> check, which allows us to avoid editing every ethernet
>>>> driver
>>>>
>>>> What do you think?
>>> Seems sane to me. Something like this (attached)?
>> Looks great:
>>
>> Acked-by: David S. Miller <davem@davemloft.net>
>
> I like it.
>
> Should I start sending patches to remove the checks from e1000/e1000e/ixgb/ixgbe
> already (to David, I assume?)?
Send the patches to me like normal...
Jeff
^ permalink raw reply
* Re: [PATCH 2/2] [POWERPC] Fix region size check in mpc5200 FEC driver
From: Grant Likely @ 2007-11-01 18:25 UTC (permalink / raw)
To: Ingo Oeser; +Cc: netdev, jgarzik, domen.puncer
In-Reply-To: <200711011913.55525.netdev@axxeo.de>
On 11/1/07, Ingo Oeser <netdev@axxeo.de> wrote:
> Hi Grant,
>
> Grant Likely schrieb:
> > From: Grant Likely <grant.likely@secretlab.ca>
> >
> > Driver shouldn't complain if the register range is larger than what
> > it expects. This works around failures with some device trees.
> >
>
> But maybe the firmware guys like to know about it?
> May I suggest putting this in front of the other check?
>
> if ((mem.end - mem.start + 1) > sizeof(struct mpc52xx_fec)) {
> printk(KERN_DEBUG DRIVER_NAME
> " - gratious resource size (%lx > %x), check mpc52xx_devices.c\n",
> (unsigned long)(mem.end - mem.start + 1), sizeof(struct mpc52xx_fec));
Personally, I'm not concerned about it. Even if the device tree says
the range is larger than what the driver knows about it is not
technically an error. If a new version of the chip appears that is
compatible, but defines a larger register range with extra feature
registers, then this message would be erroneously printed. Finally,
depending on how you read the mpc5200 user guild, it can be 100% valid
to specify the reg size as 0x800 instead of 0x400.
Cheers,
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195
^ permalink raw reply
* Re: [PATCH 2/2] [POWERPC] Fix region size check in mpc5200 FEC driver
From: Ingo Oeser @ 2007-11-01 18:13 UTC (permalink / raw)
To: Grant Likely; +Cc: netdev, jgarzik, domen.puncer
In-Reply-To: <20071101142235.7620.36853.stgit@trillian.cg.shawcable.net>
Hi Grant,
Grant Likely schrieb:
> From: Grant Likely <grant.likely@secretlab.ca>
>
> Driver shouldn't complain if the register range is larger than what
> it expects. This works around failures with some device trees.
>
But maybe the firmware guys like to know about it?
May I suggest putting this in front of the other check?
if ((mem.end - mem.start + 1) > sizeof(struct mpc52xx_fec)) {
printk(KERN_DEBUG DRIVER_NAME
" - gratious resource size (%lx > %x), check mpc52xx_devices.c\n",
(unsigned long)(mem.end - mem.start + 1), sizeof(struct mpc52xx_fec));
}
> - if ((mem.end - mem.start + 1) != sizeof(struct mpc52xx_fec)) {
> + if ((mem.end - mem.start + 1) < sizeof(struct mpc52xx_fec)) {
> printk(KERN_ERR DRIVER_NAME
> - " - invalid resource size (%lx != %x), check mpc52xx_devices.c\n",
> + " - invalid resource size (%lx < %x), check mpc52xx_devices.c\n",
> (unsigned long)(mem.end - mem.start + 1), sizeof(struct mpc52xx_fec));
> return -EINVAL;
> }
Best Regards
Ingo Oeser
^ permalink raw reply
* Re: [PATCH] e1000, e1000e valid-addr fixes
From: Stephen Hemminger @ 2007-11-01 18:11 UTC (permalink / raw)
To: Jeff Garzik
Cc: David Miller, netdev, davej, auke-jan.h.kok, ajax, linux-kernel
In-Reply-To: <471EABEE.8030900@garzik.org>
How about:
static int eth_validate_addr(const struct net_device *dev)
{
return is_valid_ether_addr(dev->dev_addr) ? 0 : -EINVAL;
}
--
Stephen Hemminger <shemminger@linux-foundation.org>
^ permalink raw reply
* Endianness problem with u32 classifier hash masks
From: Radu Rendec @ 2007-11-01 17:55 UTC (permalink / raw)
To: netdev
Hi,
While trying to implement u32 hashes in my shaping machine I ran into a
possible bug in the u32 hash/bucket computing algorithm
(net/sched/cls_u32.c).
The problem occurs only with hash masks that extend over the octet
boundary, on little endian machines (where htonl() actually does
something).
I'm not 100% sure this is a problem with u32 itself, but at least I'm
sure u32 with the same configuration would behave differently on little
endian and big endian machines. Detailed description of the problem and
proposed patch follow.
Let's say that I would like to use 0x3fc0 as the hash mask. This means 8
contiguous "1" bits starting at b6. With such a mask, the expected (and
logical) behavior is to hash any address in, for instance,
192.168.0.0/26 in bucket 0, then any address in 192.168.0.64/26 in
bucket 1, then 192.168.0.128/26 in bucket 2 and so on.
This is exactly what would happen on a big endian machine, but on little
endian machines, what would actually happen with current implementation
is 0x3fc0 being reversed (into 0xc03f0000) by htonl() in the userspace
tool and then applied to 192.168.x.x in the u32 classifier. When
shifting right by 16 bits (rank of first "1" bit in the reversed mask)
and applying the divisor mask (0xff for divisor 256), what would
actually remain is 0x3f applied on the "168" octet of the address.
One could say is this can be easily worked around by taking endianness
into account in userspace and supplying an appropriate mask (0xfc03)
that would be turned into contiguous "1" bits when reversed
(0x03fc0000). But the actual problem is the network address (inside the
packet) not being converted to host order, but used as a host-order
value when computing the bucket.
Let's say the network address is written as n31 n30 ... n0, with n0
being the least significant bit. When used directly (without any
conversion) on a little endian machine, it becomes
n7 ... n0 n8 ..n15 etc in the machine's registers. Thus bits n7 and n8
would no longer be adjacent and 192.168.64.0/26 and 192.168.128.0/26
would no longer be consecutive.
My approach to this issue was keeping the hash mask in host order and
converting the octets in the packet to host order before applying the
mask. This proved to work just fine on my little endian machine, but I'm
interested in finding out (from you) if this really is an issue with u32
itself.
My changes to the u32 classifier are attached below as a patch. It was
made against 2.6.22.9, but applies cleanly on Dave Miller's net-2.6
tree.
The idea behind my changes is to keep the user space tool intact and
work everything out in kernel space (because converting the packet
octets to host order must be done in kernel anyway).
Therefore, hash masks are converted back to host order when a selector
is configured - in u32_change() - and converted to network order
(because userspace tools expect to get them in network order from the
kernel) when a selector is dumped - in u32_dump().
I would like at least to know your opinion about this issue.
Thanks,
Radu Rendec
--- linux-2.6.22.9/net/sched/cls_u32.c.orig 2007-10-30 17:08:03.000000000 +0200
+++ linux-2.6.22.9/net/sched/cls_u32.c 2007-10-30 17:04:49.000000000 +0200
@@ -198,7 +198,7 @@
ht = n->ht_down;
sel = 0;
if (ht->divisor)
- sel = ht->divisor&u32_hash_fold(*(u32*)(ptr+n->sel.hoff), &n->sel,n->fshift);
+ sel = ht->divisor&u32_hash_fold(ntohl(*(u32*)(ptr+n->sel.hoff)), &n->sel,n->fshift);
if (!(n->sel.flags&(TC_U32_VAROFFSET|TC_U32_OFFSET|TC_U32_EAT)))
goto next_ht;
@@ -626,6 +626,10 @@
}
#endif
+ /* userspace tc tool sends us the hmask in network order, but we
+ * need host order, so change it here */
+ s->hmask = ntohl(s->hmask);
+
memcpy(&n->sel, s, sizeof(*s) + s->nkeys*sizeof(struct tc_u32_key));
n->ht_up = ht;
n->handle = handle;
@@ -735,9 +739,14 @@
u32 divisor = ht->divisor+1;
RTA_PUT(skb, TCA_U32_DIVISOR, 4, &divisor);
} else {
+ /* get the address where the selector will be put, then
+ * change the hmask after it is put there */
+ struct tc_u32_sel *s =
+ (struct tc_u32_sel *)RTA_DATA(skb_tail_pointer(skb));
RTA_PUT(skb, TCA_U32_SEL,
sizeof(n->sel) + n->sel.nkeys*sizeof(struct tc_u32_key),
&n->sel);
+ s->hmask = htonl(s->hmask);
if (n->ht_up) {
u32 htid = n->handle & 0xFFFFF000;
RTA_PUT(skb, TCA_U32_HASH, 4, &htid);
^ permalink raw reply
* Re: [PATCH] e1000, e1000e valid-addr fixes
From: Kok, Auke @ 2007-11-01 18:04 UTC (permalink / raw)
To: David Miller; +Cc: jeff, netdev, davej, ajax, linux-kernel
In-Reply-To: <20071023.192313.115635750.davem@davemloft.net>
David Miller wrote:
> From: Jeff Garzik <jeff@garzik.org>
> Date: Tue, 23 Oct 2007 22:20:30 -0400
>
>> David Miller wrote:
>>> From: Jeff Garzik <jeff@garzik.org>
>>> Date: Tue, 23 Oct 2007 21:03:36 -0400
>>>
>>>> I'm wondering if there is a way to avoid adding
>>>>
>>>> if (!is_valid_ether_addr(dev->dev_addr))
>>>> return -EINVAL;
>>>>
>>>> to every ethernet driver's ->open() hook.
>>> The first idea I get is:
>>>
>>> 1) Create netdev->validate_dev_addr().
>>>
>>> 2) If it exists, invoke it before ->open(), abort
>>> and return if any errors signaled.
>>>
>>> etherdev init hooks up a function that does the above
>>> check, which allows us to avoid editing every ethernet
>>> driver
>>>
>>> What do you think?
>> Seems sane to me. Something like this (attached)?
>
> Looks great:
>
> Acked-by: David S. Miller <davem@davemloft.net>
I like it.
Should I start sending patches to remove the checks from e1000/e1000e/ixgb/ixgbe
already (to David, I assume?)?
Auke
^ permalink raw reply
* Re: [PATCH] INET : removes per bucket rwlock in tcp/dccp ehash table
From: Eric Dumazet @ 2007-11-01 18:00 UTC (permalink / raw)
To: Jarek Poplawski
Cc: David S. Miller, Linux Netdev List, Andi Kleen,
Arnaldo Carvalho de Melo
In-Reply-To: <4729F96B.8000802@o2.pl>
Jarek Poplawski a écrit :
> Hi,
>
> A few doubts below:
>
>>
>> +#if defined(CONFIG_SMP) || defined(CONFIG_PROVE_LOCKING)
>
> Probably "|| defined(CONFIG_DEBUG_SPINLOCK)" is needed here.
Not sure, because DEBUG_SPINLOCK only applies to spinlocks.
Here we deal with rwlocks.
>
>> +/*
>> + * Instead of using one rwlock for each inet_ehash_bucket, we use a table of locks
>> + * The size of this table is a power of two and depends on the number of CPUS.
>> + */
>> +# if defined(CONFIG_DEBUG_LOCK_ALLOC)
>> +# define EHASH_LOCK_SZ 256
>> +# elif NR_CPUS >= 32
>> +# define EHASH_LOCK_SZ 4096
>> +# elif NR_CPUS >= 16
>> +# define EHASH_LOCK_SZ 2048
>> +# elif NR_CPUS >= 8
>> +# define EHASH_LOCK_SZ 1024
>> +# elif NR_CPUS >= 4
>> +# define EHASH_LOCK_SZ 512
>> +# else
>> +# define EHASH_LOCK_SZ 256
>> +# endif
>> +#else
>> +# define EHASH_LOCK_SZ 0
>> +#endif
>> +
>
> Looks hackish: usually DEBUG code checks "real" environment, and here it's
> a special case. But omitting locks if no SMP or DEBUG is strange. IMHO,
> there should be 1 instead of 0.
It is 0 so that no alloc is done. (see your next questions)
>
>> struct inet_hashinfo {
>> /* This is for sockets with full identity only. Sockets here will
>> * always be without wildcards and will have the following invariant:
>> @@ -100,6 +121,7 @@ struct inet_hashinfo {
>> * TIME_WAIT sockets use a separate chain (twchain).
>> */
>> struct inet_ehash_bucket *ehash;
>> + rwlock_t *ehash_locks;
>>
>> /* Ok, let's try this, I give up, we do need a local binding
>> * TCP hash as well as the others for fast bind/connect.
>> @@ -134,6 +156,13 @@ static inline struct inet_ehash_bucket *inet_ehash_bucket(
>> return &hashinfo->ehash[hash & (hashinfo->ehash_size - 1)];
>> }
>>
>> +static inline rwlock_t *inet_ehash_lockp(
>> + struct inet_hashinfo *hashinfo,
>> + unsigned int hash)
>> +{
>> + return &hashinfo->ehash_locks[hash & (EHASH_LOCK_SZ - 1)];
>> +}
>> +
>
> Is it OK for EHASH_LOCK_SZ == 0?
At least, compiled tested and booted on UP ;)
>
> ...
>> diff --git a/net/dccp/proto.c b/net/dccp/proto.c
>> index d849739..3b5f97a 100644
>> --- a/net/dccp/proto.c
>> +++ b/net/dccp/proto.c
>> @@ -1072,11 +1072,18 @@ static int __init dccp_init(void)
>> }
>>
>> for (i = 0; i < dccp_hashinfo.ehash_size; i++) {
>> - rwlock_init(&dccp_hashinfo.ehash[i].lock);
>> INIT_HLIST_HEAD(&dccp_hashinfo.ehash[i].chain);
>> INIT_HLIST_HEAD(&dccp_hashinfo.ehash[i].twchain);
>> }
>> -
>> + if (EHASH_LOCK_SZ) {
>
> Why not #ifdef then? But, IMHO, rwlock_init() should be done at least
> once here. (Similarly later for tcp.)
well, #ifdef are not so nice :)
>
>> + dccp_hashinfo.ehash_locks =
>> + kmalloc(EHASH_LOCK_SZ * sizeof(rwlock_t),
>> + GFP_KERNEL);
>> + if (!dccp_hashinfo.ehash_locks)
>> + goto out_free_dccp_ehash;
>> + for (i = 0; i < EHASH_LOCK_SZ; i++)
>> + rwlock_init(&dccp_hashinfo.ehash_locks[i]);
>> + }
>> bhash_order = ehash_order;
>>
>> do {
>> @@ -1091,7 +1098,7 @@ static int __init dccp_init(void)
>>
>> if (!dccp_hashinfo.bhash) {
>> DCCP_CRIT("Failed to allocate DCCP bind hash table");
>> - goto out_free_dccp_ehash;
>> + goto out_free_dccp_locks;
>> }
>>
>> for (i = 0; i < dccp_hashinfo.bhash_size; i++) {
>> @@ -1121,6 +1128,9 @@ out_free_dccp_mib:
>> out_free_dccp_bhash:
>> free_pages((unsigned long)dccp_hashinfo.bhash, bhash_order);
>> dccp_hashinfo.bhash = NULL;
>> +out_free_dccp_locks:
>> + kfree(dccp_hashinfo.ehash_locks);
>> + dccp_hashinfo.ehash_locks = NULL;
>> out_free_dccp_ehash:
>> free_pages((unsigned long)dccp_hashinfo.ehash, ehash_order);
>> dccp_hashinfo.ehash = NULL;
>
> Isn't such kfree(dccp_hashinfo.ehash_locks) needed in dccp_fini()?
>
Probably ! Thank you for reviewing !
Eric
^ permalink raw reply
* Re: [PATCH] INET : removes per bucket rwlock in tcp/dccp ehash table
From: Eric Dumazet @ 2007-11-01 17:54 UTC (permalink / raw)
To: Stephen Hemminger
Cc: David S. Miller, Linux Netdev List, Andi Kleen,
Arnaldo Carvalho de Melo
In-Reply-To: <20071101091456.26248ce0@freepuppy.rosehill>
Stephen Hemminger a écrit :
> On Thu, 01 Nov 2007 11:16:20 +0100
> Eric Dumazet <dada1@cosmosbay.com> wrote:
>
>> As done two years ago on IP route cache table (commit
>> 22c047ccbc68fa8f3fa57f0e8f906479a062c426) , we can avoid using one lock per
>> hash bucket for the huge TCP/DCCP hash tables.
>>
>> On a typical x86_64 platform, this saves about 2MB or 4MB of ram, for litle
>> performance differences. (we hit a different cache line for the rwlock, but
>> then the bucket cache line have a better sharing factor among cpus, since we
>> dirty it less often)
>>
>> Using a 'small' table of hashed rwlocks should be more than enough to provide
>> correct SMP concurrency between different buckets, without using too much
>> memory. Sizing of this table depends on NR_CPUS and various CONFIG settings.
>>
>> This patch provides some locking abstraction that may ease a future work using
>> a different model for TCP/DCCP table.
>>
>> Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
>>
>> include/net/inet_hashtables.h | 40 ++++++++++++++++++++++++++++----
>> net/dccp/proto.c | 16 ++++++++++--
>> net/ipv4/inet_diag.c | 9 ++++---
>> net/ipv4/inet_hashtables.c | 7 +++--
>> net/ipv4/inet_timewait_sock.c | 13 +++++-----
>> net/ipv4/tcp.c | 11 +++++++-
>> net/ipv4/tcp_ipv4.c | 11 ++++----
>> net/ipv6/inet6_hashtables.c | 19 ++++++++-------
>> 8 files changed, 89 insertions(+), 37 deletions(-)
>>
>
> Longterm is there any chance of using rcu for this? Seems like
> it could be a big win.
>
This was discussed in the past, and I even believe some patch was proposed,
but some guys (including David) complained that RCU is well suited for 'mostly
read' structures.
On some web server workloads, TCP hash table is constantly accessed in write
mode (socket creation, socket move to timewait state, socket deleted...), and
RCU added overhead and poor cache re-use (because sockets must be placed on
RCU queue before reuse)
On these typical workload, hash table without RCU is still the best.
Longterm changes would rather be based on Robert Olsson suggestion last year
(trie based lookups and unified IP/TCP cache)
Short term changes would be to be able to resize the TCP hash table (being
small at boot, and be able to grow it if necessary). Its current size on
modern machines is just insane.
^ permalink raw reply
* Re: [PATCH] decnet: "addr" module param can't be __initdata
From: Steven Whitehouse @ 2007-11-01 15:24 UTC (permalink / raw)
To: Alexey Dobriyan; +Cc: davem, netdev, devel
In-Reply-To: <20071101153629.GA16499@localhost.sw.ru>
Hi,
Looks good, Feel free to add:
Acked-by: Steven Whitehouse <swhiteho@redhat.com>
Steve.
On Thu, Nov 01, 2007 at 06:36:29PM +0300, Alexey Dobriyan wrote:
> sysfs keeps references to module parameters via /sys/module/*/parameters,
> so marking them as __initdata can't work.
>
> Steps to reproduce:
>
> modprobe decnet
> cat /sys/module/decnet/parameters/addr
>
> BUG: unable to handle kernel paging request at virtual address f88cd410
> printing eip: c043dfd1 *pdpt = 0000000000004001 *pde = 0000000004408067 *pte = 0000000000000000
> Oops: 0000 [#1] PREEMPT SMP
> Modules linked in: decnet sunrpc af_packet ipv6 binfmt_misc dm_mirror dm_multipath dm_mod sbs sbshc fan dock battery backlight ac power_supply parport loop rtc_cmos serio_raw rtc_core rtc_lib button amd_rng sr_mod cdrom shpchp pci_hotplug ehci_hcd ohci_hcd uhci_hcd usbcore
> Pid: 2099, comm: cat Not tainted (2.6.24-rc1-b1d08ac064268d0ae2281e98bf5e82627e0f0c56-bloat #6)
> EIP: 0060:[<c043dfd1>] EFLAGS: 00210286 CPU: 1
> EIP is at param_get_int+0x6/0x20
> EAX: c5c87000 EBX: 00000000 ECX: 000080d0 EDX: f88cd410
> ESI: f8a108f8 EDI: c5c87000 EBP: 00000000 ESP: c5c97f00
> DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
> Process cat (pid: 2099, ti=c5c97000 task=c641ee10 task.ti=c5c97000)
> Stack: 00000000 f8a108f8 c5c87000 c043db6b f8a108f1 00000124 c043de1a c043db2f
> f88cd410 ffffffff c5c87000 f8a16bc8 f8a16bc8 c043dd69 c043dd54 c5dd5078
> c043dbc8 c5cc7580 c06ee64c c5d679f8 c04c431f c641f480 c641f484 00001000
> Call Trace:
> [<c043db6b>] param_array_get+0x3c/0x62
> [<c043de1a>] param_array_set+0x0/0xdf
> [<c043db2f>] param_array_get+0x0/0x62
> [<c043dd69>] param_attr_show+0x15/0x2d
> [<c043dd54>] param_attr_show+0x0/0x2d
> [<c043dbc8>] module_attr_show+0x1a/0x1e
> [<c04c431f>] sysfs_read_file+0x7c/0xd9
> [<c04c42a3>] sysfs_read_file+0x0/0xd9
> [<c048d4b2>] vfs_read+0x88/0x134
> [<c042090b>] do_page_fault+0x0/0x7d5
> [<c048d920>] sys_read+0x41/0x67
> [<c04080fa>] sysenter_past_esp+0x6b/0xc1
> =======================
> Code: 00 83 c4 0c c3 83 ec 0c 8b 52 10 8b 12 c7 44 24 04 27 dd 6c c0 89 04 24 89 54 24 08 e8 ea 01 0c 00 83 c4 0c c3 83 ec 0c 8b 52 10 <8b> 12 c7 44 24 04 58 8c 6a c0 89 04 24 89 54 24 08 e8 ca 01 0c
> EIP: [<c043dfd1>] param_get_int+0x6/0x20 SS:ESP 0068:c5c97f00
>
> Signed-off-by: Alexey Dobriyan <adobriyan@sw.ru>
> ---
>
> net/decnet/dn_dev.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- a/net/decnet/dn_dev.c
> +++ b/net/decnet/dn_dev.c
> @@ -1439,7 +1439,7 @@ static const struct file_operations dn_dev_seq_fops = {
>
> #endif /* CONFIG_PROC_FS */
>
> -static int __initdata addr[2];
> +static int addr[2];
> module_param_array(addr, int, NULL, 0444);
> MODULE_PARM_DESC(addr, "The DECnet address of this machine: area,node");
>
>
> -
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] Clean the ip_sockglue.c from some ugly ifdefs
From: Pavel Emelyanov @ 2007-11-01 16:38 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo; +Cc: David Miller, Linux Netdev List, devel
In-Reply-To: <20071101163112.GE13130@ghostprotocols.net>
Arnaldo Carvalho de Melo wrote:
> Em Thu, Nov 01, 2007 at 06:52:34PM +0300, Pavel Emelyanov escreveu:
>> The #idfed CONFIG_IP_MROUTE is sometimes places inside the if-s,
>> which looks completely bad. Similar ifdefs inside the functions
>> looks a bit better, but they are also not recommended to be used.
>>
>> Provide an ifdef-ed ip_mroute_opt() helper to cleanup the code.
>>
>> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
>
> Perhaps a better name would be ip_mroute_valid_opt()?
No :) The _valid_ mrote opts are from 0 to 8, according to MRT_XXX
macros, not from 0 to 10 as checked. I suspect this was a kind of
reserve for future use and thus do not change this.
Correct me if I am wrong.
> - Arnaldo
Thanks,
Pavel
^ permalink raw reply
* Re: [PATCH] Clean the ip_sockglue.c from some ugly ifdefs
From: Arnaldo Carvalho de Melo @ 2007-11-01 16:31 UTC (permalink / raw)
To: Pavel Emelyanov; +Cc: David Miller, Linux Netdev List, devel
In-Reply-To: <4729F642.7000709@openvz.org>
Em Thu, Nov 01, 2007 at 06:52:34PM +0300, Pavel Emelyanov escreveu:
> The #idfed CONFIG_IP_MROUTE is sometimes places inside the if-s,
> which looks completely bad. Similar ifdefs inside the functions
> looks a bit better, but they are also not recommended to be used.
>
> Provide an ifdef-ed ip_mroute_opt() helper to cleanup the code.
>
> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
Perhaps a better name would be ip_mroute_valid_opt()?
- Arnaldo
^ permalink raw reply
* Re: af_packet.c flush_dcache_page
From: Evgeniy Polyakov @ 2007-11-01 16:27 UTC (permalink / raw)
To: Patrick McHardy; +Cc: David Miller, netdev
In-Reply-To: <4729FA78.5040702@trash.net>
On Thu, Nov 01, 2007 at 05:10:32PM +0100, Patrick McHardy (kaber@trash.net) wrote:
> David Miller wrote:
> >Instead of answering your questions, I'm going to show you
> >how to avoid having to do any of this cache flushing crap :-)
> >
> >You can avoid having to flush anything as long as the virtual
> >addresses on the kernel side are modulo SHMLBA the virtual addresses
> >on the userland side.
> >
> >We have some (decidedly awkward) mechanisms to try and achieve
> >this in the kernel, but they are cumbersome and not air tight.
> >
> >Instead, I would recommend simply that you access the ring
> >buffer directly in userspace. This avoids all of the cache
> >aliasing issues.
> >
> >Yes, this means you have to do the ring buffer accesses in
> >the context of the user, but it simplifies so much that I think
> >it'd be worth it.
>
>
> I'm probably misunderstanding your suggestion because of my
> limited mm knowledge, are you suggesting to do something like
> this:
>
> setsockopt(RX_RING, ...):
>
> Allocate ring using get_user_pages, return address to user
>
> tpacket_rcv/netlink_unicast/netlink_broadcast:
>
> for each receiver:
> switch_mm(...)
> copy data to ring
>
> switch_mm(original mm)
>
> Would this work in softirq context?
IIRC it requires disabled interrupts.
Probably David suggests to provide a pointer to allocated
in userspace buffer and use copy_to_user() and friends.
> >Another option is to use the "copy_to_user_page()" and
> >"copy_from_user_page()" interfaces which will do all of
> >the necessary cache flushing for you.
> >
> >Actually it might be nice to convert AF_PACKET's mmap() code
> >over to using those things.
>
>
> That would also require to do the copy in the context of
> the user, right?
Most of the time it is possible to call copy_to_user() in atomic
context, but it can fail, in which case some additional mechanism to
make a copy should be invented (workqueue, kthread, whatever you like :)
--
Evgeniy Polyakov
^ permalink raw reply
* Re: [PATCH] INET : removes per bucket rwlock in tcp/dccp ehash table
From: Stephen Hemminger @ 2007-11-01 16:14 UTC (permalink / raw)
To: Eric Dumazet
Cc: David S. Miller, Linux Netdev List, Andi Kleen,
Arnaldo Carvalho de Melo
In-Reply-To: <4729A774.9030409@cosmosbay.com>
On Thu, 01 Nov 2007 11:16:20 +0100
Eric Dumazet <dada1@cosmosbay.com> wrote:
> As done two years ago on IP route cache table (commit
> 22c047ccbc68fa8f3fa57f0e8f906479a062c426) , we can avoid using one lock per
> hash bucket for the huge TCP/DCCP hash tables.
>
> On a typical x86_64 platform, this saves about 2MB or 4MB of ram, for litle
> performance differences. (we hit a different cache line for the rwlock, but
> then the bucket cache line have a better sharing factor among cpus, since we
> dirty it less often)
>
> Using a 'small' table of hashed rwlocks should be more than enough to provide
> correct SMP concurrency between different buckets, without using too much
> memory. Sizing of this table depends on NR_CPUS and various CONFIG settings.
>
> This patch provides some locking abstraction that may ease a future work using
> a different model for TCP/DCCP table.
>
> Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
>
> include/net/inet_hashtables.h | 40 ++++++++++++++++++++++++++++----
> net/dccp/proto.c | 16 ++++++++++--
> net/ipv4/inet_diag.c | 9 ++++---
> net/ipv4/inet_hashtables.c | 7 +++--
> net/ipv4/inet_timewait_sock.c | 13 +++++-----
> net/ipv4/tcp.c | 11 +++++++-
> net/ipv4/tcp_ipv4.c | 11 ++++----
> net/ipv6/inet6_hashtables.c | 19 ++++++++-------
> 8 files changed, 89 insertions(+), 37 deletions(-)
>
Longterm is there any chance of using rcu for this? Seems like
it could be a big win.
--
Stephen Hemminger <shemminger@linux-foundation.org>
^ permalink raw reply
* Re: af_packet.c flush_dcache_page
From: Patrick McHardy @ 2007-11-01 16:10 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20071031.155749.26538335.davem@davemloft.net>
David Miller wrote:
> Instead of answering your questions, I'm going to show you
> how to avoid having to do any of this cache flushing crap :-)
>
> You can avoid having to flush anything as long as the virtual
> addresses on the kernel side are modulo SHMLBA the virtual addresses
> on the userland side.
>
> We have some (decidedly awkward) mechanisms to try and achieve
> this in the kernel, but they are cumbersome and not air tight.
>
> Instead, I would recommend simply that you access the ring
> buffer directly in userspace. This avoids all of the cache
> aliasing issues.
>
> Yes, this means you have to do the ring buffer accesses in
> the context of the user, but it simplifies so much that I think
> it'd be worth it.
I'm probably misunderstanding your suggestion because of my
limited mm knowledge, are you suggesting to do something like
this:
setsockopt(RX_RING, ...):
Allocate ring using get_user_pages, return address to user
tpacket_rcv/netlink_unicast/netlink_broadcast:
for each receiver:
switch_mm(...)
copy data to ring
switch_mm(original mm)
Would this work in softirq context?
> Another option is to use the "copy_to_user_page()" and
> "copy_from_user_page()" interfaces which will do all of
> the necessary cache flushing for you.
>
> Actually it might be nice to convert AF_PACKET's mmap() code
> over to using those things.
That would also require to do the copy in the context of
the user, right?
^ permalink raw reply
* Re: [PATCH] - e1000e/ethtool.c - convert macros to functions
From: Kok, Auke @ 2007-11-01 16:01 UTC (permalink / raw)
To: Joe Perches; +Cc: e1000-devel, netdev, Kok, Auke, Jeff Garzik
In-Reply-To: <1193887792.11020.54.camel@localhost>
Joe Perches wrote:
> Add functions for reg_pattern_test and reg_set_and check
> Changed macros to use these functions
>
> Compiled x86, untested
>
> Size decreased ~2K
>
> old:
>
> $ size drivers/net/e1000e/ethtool.o
> text data bss dec hex filename
> 14461 0 0 14461 387d drivers/net/e1000e/ethtool.o
>
> new:
>
> $ size drivers/net/e1000e/ethtool.o
> text data bss dec hex filename
> 12498 0 0 12498 30d2 drivers/net/e1000e/ethtool.o
>
>
> Signed-off-by: Joe Perches <joe@perches.com>
ok, this looks great.
I'm going to have this tested a bit before I pass it on to Jeff Garzik.
Thanks!
Auke
>
> ---
>
> drivers/net/e1000e/ethtool.c | 78 +++++++++++++++++++++++++----------------
> 1 files changed, 47 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/net/e1000e/ethtool.c b/drivers/net/e1000e/ethtool.c
> index 6a39784..225db17 100644
> --- a/drivers/net/e1000e/ethtool.c
> +++ b/drivers/net/e1000e/ethtool.c
> @@ -691,41 +691,57 @@ err_setup:
> return err;
> }
>
> -#define REG_PATTERN_TEST(R, M, W) REG_PATTERN_TEST_ARRAY(R, 0, M, W)
> -#define REG_PATTERN_TEST_ARRAY(reg, offset, mask, writeable) \
> -{ \
> - u32 _pat; \
> - u32 _value; \
> - u32 _test[] = {0x5A5A5A5A, 0xA5A5A5A5, 0x00000000, 0xFFFFFFFF}; \
> - for (_pat = 0; _pat < ARRAY_SIZE(_test); _pat++) { \
> - E1000_WRITE_REG_ARRAY(hw, reg, offset, \
> - (_test[_pat] & writeable)); \
> - _value = E1000_READ_REG_ARRAY(hw, reg, offset); \
> - if (_value != (_test[_pat] & writeable & mask)) { \
> - ndev_err(netdev, "pattern test reg %04X " \
> - "failed: got 0x%08X expected 0x%08X\n", \
> - reg + offset, \
> - value, (_test[_pat] & writeable & mask)); \
> - *data = reg; \
> - return 1; \
> - } \
> - } \
> +bool reg_pattern_test_array(struct e1000_adapter *adapter, u64 *data,
> + int reg, int offset, u32 mask, u32 write)
> +{
> + int i;
> + u32 read;
> + static const u32 test[] =
> + {0x5A5A5A5A, 0xA5A5A5A5, 0x00000000, 0xFFFFFFFF};
> + for (i = 0; i < ARRAY_SIZE(test); i++) {
> + E1000_WRITE_REG_ARRAY(&adapter->hw, reg, offset,
> + (test[i] & write));
> + read = E1000_READ_REG_ARRAY(&adapter->hw, reg, offset);
> + if (read != (test[i] & write & mask)) {
> + ndev_err(adapter->netdev, "pattern test reg %04X "
> + "failed: got 0x%08X expected 0x%08X\n",
> + reg + offset,
> + read, (test[i] & write & mask));
> + *data = reg;
> + return true;
> + }
> + }
> + return false;
> }
>
> -#define REG_SET_AND_CHECK(R, M, W) \
> -{ \
> - u32 _value; \
> - __ew32(hw, R, W & M); \
> - _value = __er32(hw, R); \
> - if ((W & M) != (_value & M)) { \
> - ndev_err(netdev, "set/check reg %04X test failed: " \
> - "got 0x%08X expected 0x%08X\n", R, (_value & M), \
> - (W & M)); \
> - *data = R; \
> - return 1; \
> - } \
> +#define REG_PATTERN_TEST(R, M, W) \
> + if (reg_pattern_test_array(adapter, data, R, 0, M, W)) \
> + return 1;
> +
> +#define REG_PATTERN_TEST_ARRAY(R, offset, M, W) \
> + if (reg_pattern_test_array(adapter, data, R, offset, M, W)) \
> + return 1;
> +
> +static bool reg_set_and_check(struct e1000_adapter *adapter, u64 *data,
> + int reg, u32 mask, u32 write)
> +{
> + u32 read;
> + __ew32(&adapter->hw, reg, write & mask);
> + read = __er32(&adapter->hw, reg);
> + if ((write & mask) != (read & mask)) {
> + ndev_err(adapter->netdev, "set/check reg %04X test failed: "
> + "got 0x%08X expected 0x%08X\n", reg, (read & mask),
> + (write & mask));
> + *data = reg;
> + return true;
> + }
> + return false;
> }
>
> +#define REG_SET_AND_CHECK(R, M, W) \
> + if (reg_set_and_check(adapter, data, R, M, W)) \
> + return 1;
> +
> static int e1000_reg_test(struct e1000_adapter *adapter, u64 *data)
> {
> struct e1000_hw *hw = &adapter->hw;
>
>
> -
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
^ permalink raw reply
* Re: [PATCH] INET : removes per bucket rwlock in tcp/dccp ehash table
From: Jarek Poplawski @ 2007-11-01 16:06 UTC (permalink / raw)
To: Eric Dumazet
Cc: David S. Miller, Linux Netdev List, Andi Kleen,
Arnaldo Carvalho de Melo
In-Reply-To: <4729A774.9030409@cosmosbay.com>
Hi,
A few doubts below:
Eric Dumazet wrote:
> As done two years ago on IP route cache table (commit
> 22c047ccbc68fa8f3fa57f0e8f906479a062c426) , we can avoid using one lock per
> hash bucket for the huge TCP/DCCP hash tables.
...
> diff --git a/include/net/inet_hashtables.h b/include/net/inet_hashtables.h
> index 4427dcd..5cbfbac 100644
> --- a/include/net/inet_hashtables.h
> +++ b/include/net/inet_hashtables.h
> @@ -37,7 +37,6 @@
> * I'll experiment with dynamic table growth later.
> */
> struct inet_ehash_bucket {
> - rwlock_t lock;
> struct hlist_head chain;
> struct hlist_head twchain;
> };
> @@ -91,6 +90,28 @@ struct inet_bind_hashbucket {
> /* This is for listening sockets, thus all sockets which possess wildcards. */
> #define INET_LHTABLE_SIZE 32 /* Yes, really, this is all you need. */
>
> +#if defined(CONFIG_SMP) || defined(CONFIG_PROVE_LOCKING)
Probably "|| defined(CONFIG_DEBUG_SPINLOCK)" is needed here.
> +/*
> + * Instead of using one rwlock for each inet_ehash_bucket, we use a table of locks
> + * The size of this table is a power of two and depends on the number of CPUS.
> + */
> +# if defined(CONFIG_DEBUG_LOCK_ALLOC)
> +# define EHASH_LOCK_SZ 256
> +# elif NR_CPUS >= 32
> +# define EHASH_LOCK_SZ 4096
> +# elif NR_CPUS >= 16
> +# define EHASH_LOCK_SZ 2048
> +# elif NR_CPUS >= 8
> +# define EHASH_LOCK_SZ 1024
> +# elif NR_CPUS >= 4
> +# define EHASH_LOCK_SZ 512
> +# else
> +# define EHASH_LOCK_SZ 256
> +# endif
> +#else
> +# define EHASH_LOCK_SZ 0
> +#endif
> +
Looks hackish: usually DEBUG code checks "real" environment, and here it's
a special case. But omitting locks if no SMP or DEBUG is strange. IMHO,
there should be 1 instead of 0.
> struct inet_hashinfo {
> /* This is for sockets with full identity only. Sockets here will
> * always be without wildcards and will have the following invariant:
> @@ -100,6 +121,7 @@ struct inet_hashinfo {
> * TIME_WAIT sockets use a separate chain (twchain).
> */
> struct inet_ehash_bucket *ehash;
> + rwlock_t *ehash_locks;
>
> /* Ok, let's try this, I give up, we do need a local binding
> * TCP hash as well as the others for fast bind/connect.
> @@ -134,6 +156,13 @@ static inline struct inet_ehash_bucket *inet_ehash_bucket(
> return &hashinfo->ehash[hash & (hashinfo->ehash_size - 1)];
> }
>
> +static inline rwlock_t *inet_ehash_lockp(
> + struct inet_hashinfo *hashinfo,
> + unsigned int hash)
> +{
> + return &hashinfo->ehash_locks[hash & (EHASH_LOCK_SZ - 1)];
> +}
> +
Is it OK for EHASH_LOCK_SZ == 0?
...
> diff --git a/net/dccp/proto.c b/net/dccp/proto.c
> index d849739..3b5f97a 100644
> --- a/net/dccp/proto.c
> +++ b/net/dccp/proto.c
> @@ -1072,11 +1072,18 @@ static int __init dccp_init(void)
> }
>
> for (i = 0; i < dccp_hashinfo.ehash_size; i++) {
> - rwlock_init(&dccp_hashinfo.ehash[i].lock);
> INIT_HLIST_HEAD(&dccp_hashinfo.ehash[i].chain);
> INIT_HLIST_HEAD(&dccp_hashinfo.ehash[i].twchain);
> }
> -
> + if (EHASH_LOCK_SZ) {
Why not #ifdef then? But, IMHO, rwlock_init() should be done at least
once here. (Similarly later for tcp.)
> + dccp_hashinfo.ehash_locks =
> + kmalloc(EHASH_LOCK_SZ * sizeof(rwlock_t),
> + GFP_KERNEL);
> + if (!dccp_hashinfo.ehash_locks)
> + goto out_free_dccp_ehash;
> + for (i = 0; i < EHASH_LOCK_SZ; i++)
> + rwlock_init(&dccp_hashinfo.ehash_locks[i]);
> + }
> bhash_order = ehash_order;
>
> do {
> @@ -1091,7 +1098,7 @@ static int __init dccp_init(void)
>
> if (!dccp_hashinfo.bhash) {
> DCCP_CRIT("Failed to allocate DCCP bind hash table");
> - goto out_free_dccp_ehash;
> + goto out_free_dccp_locks;
> }
>
> for (i = 0; i < dccp_hashinfo.bhash_size; i++) {
> @@ -1121,6 +1128,9 @@ out_free_dccp_mib:
> out_free_dccp_bhash:
> free_pages((unsigned long)dccp_hashinfo.bhash, bhash_order);
> dccp_hashinfo.bhash = NULL;
> +out_free_dccp_locks:
> + kfree(dccp_hashinfo.ehash_locks);
> + dccp_hashinfo.ehash_locks = NULL;
> out_free_dccp_ehash:
> free_pages((unsigned long)dccp_hashinfo.ehash, ehash_order);
> dccp_hashinfo.ehash = NULL;
Isn't such kfree(dccp_hashinfo.ehash_locks) needed in dccp_fini()?
Regards,
Jarek P.
^ permalink raw reply
* [PATCH 2/2][NETFILTER] Use the list_for_each_entry in nf_sockopt.c
From: Pavel Emelyanov @ 2007-11-01 15:58 UTC (permalink / raw)
To: David Miller; +Cc: Patrick McHardy, Linux Netdev List, devel
The list_head pointer, used to iterate over the list, is not used
at all, but to get the struct nf_sockopt_ops pointer (and actually
not in the 100% clean way).
So use the list_for_each_entry, removing one unneeded variable
from each place of use.
Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
---
diff --git a/net/netfilter/nf_sockopt.c b/net/netfilter/nf_sockopt.c
index a5e5e30..87bc144 100644
--- a/net/netfilter/nf_sockopt.c
+++ b/net/netfilter/nf_sockopt.c
@@ -23,14 +23,13 @@ static inline int overlap(int min1, int max1, int min2, int max2)
/* Functions to register sockopt ranges (exclusive). */
int nf_register_sockopt(struct nf_sockopt_ops *reg)
{
- struct list_head *i;
+ struct nf_sockopt_ops *ops;
int ret = 0;
if (mutex_lock_interruptible(&nf_sockopt_mutex) != 0)
return -EINTR;
- list_for_each(i, &nf_sockopts) {
- struct nf_sockopt_ops *ops = (struct nf_sockopt_ops *)i;
+ list_for_each_entry(ops, &nf_sockopts, list) {
if (ops->pf == reg->pf
&& (overlap(ops->set_optmin, ops->set_optmax,
reg->set_optmin, reg->set_optmax)
@@ -64,7 +63,6 @@ EXPORT_SYMBOL(nf_unregister_sockopt);
static struct nf_sockopt_ops *nf_sockopt_find(struct sock *sk, int pf,
int val, int get)
{
- struct list_head *i;
struct nf_sockopt_ops *ops;
if (sk->sk_net != &init_net)
@@ -73,8 +71,7 @@ static struct nf_sockopt_ops *nf_sockopt_find(struct sock *sk, int pf,
if (mutex_lock_interruptible(&nf_sockopt_mutex) != 0)
return ERR_PTR(-EINTR);
- list_for_each(i, &nf_sockopts) {
- ops = (struct nf_sockopt_ops *)i;
+ list_for_each_entry(ops, &nf_sockopts, list) {
if (ops->pf == pf) {
if (!try_module_get(ops->owner))
goto out_nosup;
--
1.5.3.4
^ permalink raw reply related
* [PATCH 1/2][NETFILTER] Consolidate nf_sockopt and compat_nf_sockopt
From: Pavel Emelyanov @ 2007-11-01 15:55 UTC (permalink / raw)
To: David Miller; +Cc: Patrick McHardy, Linux Netdev List, devel
Both lookup the nf_sockopt_ops object to call the get/set callbacks
from, but they perform it in a completely similar way.
Introduce the helper for finding the ops.
Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
---
diff --git a/net/netfilter/nf_sockopt.c b/net/netfilter/nf_sockopt.c
index aa28315..a5e5e30 100644
--- a/net/netfilter/nf_sockopt.c
+++ b/net/netfilter/nf_sockopt.c
@@ -61,48 +61,59 @@ void nf_unregister_sockopt(struct nf_sockopt_ops *reg)
}
EXPORT_SYMBOL(nf_unregister_sockopt);
-/* Call get/setsockopt() */
-static int nf_sockopt(struct sock *sk, int pf, int val,
- char __user *opt, int *len, int get)
+static struct nf_sockopt_ops *nf_sockopt_find(struct sock *sk, int pf,
+ int val, int get)
{
struct list_head *i;
struct nf_sockopt_ops *ops;
- int ret;
if (sk->sk_net != &init_net)
- return -ENOPROTOOPT;
+ return ERR_PTR(-ENOPROTOOPT);
if (mutex_lock_interruptible(&nf_sockopt_mutex) != 0)
- return -EINTR;
+ return ERR_PTR(-EINTR);
list_for_each(i, &nf_sockopts) {
ops = (struct nf_sockopt_ops *)i;
if (ops->pf == pf) {
if (!try_module_get(ops->owner))
goto out_nosup;
+
if (get) {
- if (val >= ops->get_optmin
- && val < ops->get_optmax) {
- mutex_unlock(&nf_sockopt_mutex);
- ret = ops->get(sk, val, opt, len);
+ if (val >= ops->get_optmin &&
+ val < ops->get_optmax)
goto out;
- }
} else {
- if (val >= ops->set_optmin
- && val < ops->set_optmax) {
- mutex_unlock(&nf_sockopt_mutex);
- ret = ops->set(sk, val, opt, *len);
+ if (val >= ops->set_optmin &&
+ val < ops->set_optmax)
goto out;
- }
}
module_put(ops->owner);
}
}
- out_nosup:
+out_nosup:
+ ops = ERR_PTR(-ENOPROTOOPT);
+out:
mutex_unlock(&nf_sockopt_mutex);
- return -ENOPROTOOPT;
+ return ops;
+}
+
+/* Call get/setsockopt() */
+static int nf_sockopt(struct sock *sk, int pf, int val,
+ char __user *opt, int *len, int get)
+{
+ struct nf_sockopt_ops *ops;
+ int ret;
+
+ ops = nf_sockopt_find(sk, pf, val, get);
+ if (IS_ERR(ops))
+ return PTR_ERR(ops);
+
+ if (get)
+ ret = ops->get(sk, val, opt, len);
+ else
+ ret = ops->set(sk, val, opt, *len);
- out:
module_put(ops->owner);
return ret;
}
@@ -124,56 +135,25 @@ EXPORT_SYMBOL(nf_getsockopt);
static int compat_nf_sockopt(struct sock *sk, int pf, int val,
char __user *opt, int *len, int get)
{
- struct list_head *i;
struct nf_sockopt_ops *ops;
int ret;
- if (sk->sk_net != &init_net)
- return -ENOPROTOOPT;
-
-
- if (mutex_lock_interruptible(&nf_sockopt_mutex) != 0)
- return -EINTR;
-
- list_for_each(i, &nf_sockopts) {
- ops = (struct nf_sockopt_ops *)i;
- if (ops->pf == pf) {
- if (!try_module_get(ops->owner))
- goto out_nosup;
-
- if (get) {
- if (val >= ops->get_optmin
- && val < ops->get_optmax) {
- mutex_unlock(&nf_sockopt_mutex);
- if (ops->compat_get)
- ret = ops->compat_get(sk,
- val, opt, len);
- else
- ret = ops->get(sk,
- val, opt, len);
- goto out;
- }
- } else {
- if (val >= ops->set_optmin
- && val < ops->set_optmax) {
- mutex_unlock(&nf_sockopt_mutex);
- if (ops->compat_set)
- ret = ops->compat_set(sk,
- val, opt, *len);
- else
- ret = ops->set(sk,
- val, opt, *len);
- goto out;
- }
- }
- module_put(ops->owner);
- }
+ ops = nf_sockopt_find(sk, pf, val, get);
+ if (IS_ERR(ops))
+ return PTR_ERR(ops);
+
+ if (get) {
+ if (ops->compat_get)
+ ret = ops->compat_get(sk, val, opt, len);
+ else
+ ret = ops->get(sk, val, ops, len);
+ } else {
+ if (ops->compat_set)
+ ret = ops->compat_set(sk, val, ops, *len);
+ else
+ ret = ops->set(sk, val, ops, *len);
}
- out_nosup:
- mutex_unlock(&nf_sockopt_mutex);
- return -ENOPROTOOPT;
- out:
module_put(ops->owner);
return ret;
}
--
1.5.3.4
^ permalink raw reply related
* [PATCH] Clean the ip_sockglue.c from some ugly ifdefs
From: Pavel Emelyanov @ 2007-11-01 15:52 UTC (permalink / raw)
To: David Miller; +Cc: Linux Netdev List, devel
The #idfed CONFIG_IP_MROUTE is sometimes places inside the if-s,
which looks completely bad. Similar ifdefs inside the functions
looks a bit better, but they are also not recommended to be used.
Provide an ifdef-ed ip_mroute_opt() helper to cleanup the code.
Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
---
diff --git a/include/linux/mroute.h b/include/linux/mroute.h
index 7da2cee..200fbb2 100644
--- a/include/linux/mroute.h
+++ b/include/linux/mroute.h
@@ -128,6 +128,18 @@ struct igmpmsg
#ifdef __KERNEL__
#include <net/sock.h>
+#ifdef CONFIG_IP_MROUTE
+static inline int ip_mroute_opt(int opt)
+{
+ return (opt >= MRT_BASE) && (op <= MRT_BASE + 10);
+}
+#else
+static inline int ip_mroute_opt(int opt)
+{
+ return 0;
+}
+#endif
+
extern int ip_mroute_setsockopt(struct sock *, int, char __user *, int);
extern int ip_mroute_getsockopt(struct sock *, int, char __user *, int __user *);
extern int ipmr_ioctl(struct sock *sk, int cmd, void __user *arg);
diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
index f51f20e..82817e5 100644
--- a/net/ipv4/ip_sockglue.c
+++ b/net/ipv4/ip_sockglue.c
@@ -437,10 +437,8 @@ static int do_ip_setsockopt(struct sock *sk, int level,
/* If optlen==0, it is equivalent to val == 0 */
-#ifdef CONFIG_IP_MROUTE
- if (optname >= MRT_BASE && optname <= (MRT_BASE + 10))
+ if (ip_mroute_opt(optname))
return ip_mroute_setsockopt(sk,optname,optval,optlen);
-#endif
err = 0;
lock_sock(sk);
@@ -909,11 +907,9 @@ int ip_setsockopt(struct sock *sk, int level,
#ifdef CONFIG_NETFILTER
/* we need to exclude all possible ENOPROTOOPTs except default case */
if (err == -ENOPROTOOPT && optname != IP_HDRINCL &&
- optname != IP_IPSEC_POLICY && optname != IP_XFRM_POLICY
-#ifdef CONFIG_IP_MROUTE
- && (optname < MRT_BASE || optname > (MRT_BASE + 10))
-#endif
- ) {
+ optname != IP_IPSEC_POLICY &&
+ optname != IP_XFRM_POLICY &&
+ !ip_mroute_opt(optname)) {
lock_sock(sk);
err = nf_setsockopt(sk, PF_INET, optname, optval, optlen);
release_sock(sk);
@@ -935,11 +931,9 @@ int compat_ip_setsockopt(struct sock *sk, int level, int optname,
#ifdef CONFIG_NETFILTER
/* we need to exclude all possible ENOPROTOOPTs except default case */
if (err == -ENOPROTOOPT && optname != IP_HDRINCL &&
- optname != IP_IPSEC_POLICY && optname != IP_XFRM_POLICY
-#ifdef CONFIG_IP_MROUTE
- && (optname < MRT_BASE || optname > (MRT_BASE + 10))
-#endif
- ) {
+ optname != IP_IPSEC_POLICY &&
+ optname != IP_XFRM_POLICY &&
+ !ip_mroute_opt(optname)) {
lock_sock(sk);
err = compat_nf_setsockopt(sk, PF_INET, optname,
optval, optlen);
@@ -967,11 +961,8 @@ static int do_ip_getsockopt(struct sock *sk, int level, int optname,
if (level != SOL_IP)
return -EOPNOTSUPP;
-#ifdef CONFIG_IP_MROUTE
- if (optname >= MRT_BASE && optname <= MRT_BASE+10) {
+ if (ip_mroute_opt(optname))
return ip_mroute_getsockopt(sk,optname,optval,optlen);
- }
-#endif
if (get_user(len,optlen))
return -EFAULT;
@@ -1171,11 +1162,8 @@ int ip_getsockopt(struct sock *sk, int level,
err = do_ip_getsockopt(sk, level, optname, optval, optlen);
#ifdef CONFIG_NETFILTER
/* we need to exclude all possible ENOPROTOOPTs except default case */
- if (err == -ENOPROTOOPT && optname != IP_PKTOPTIONS
-#ifdef CONFIG_IP_MROUTE
- && (optname < MRT_BASE || optname > MRT_BASE+10)
-#endif
- ) {
+ if (err == -ENOPROTOOPT && optname != IP_PKTOPTIONS &&
+ !ip_mroute_opt(optname)) {
int len;
if (get_user(len,optlen))
@@ -1200,11 +1188,8 @@ int compat_ip_getsockopt(struct sock *sk, int level, int optname,
int err = do_ip_getsockopt(sk, level, optname, optval, optlen);
#ifdef CONFIG_NETFILTER
/* we need to exclude all possible ENOPROTOOPTs except default case */
- if (err == -ENOPROTOOPT && optname != IP_PKTOPTIONS
-#ifdef CONFIG_IP_MROUTE
- && (optname < MRT_BASE || optname > MRT_BASE+10)
-#endif
- ) {
+ if (err == -ENOPROTOOPT && optname != IP_PKTOPTIONS &&
+ !ip_mroute_opt(optname)) {
int len;
if (get_user(len, optlen))
--
1.5.3.4
^ permalink raw reply related
* [PATCH] decnet: "addr" module param can't be __initdata
From: Alexey Dobriyan @ 2007-11-01 15:36 UTC (permalink / raw)
To: davem; +Cc: netdev, devel
sysfs keeps references to module parameters via /sys/module/*/parameters,
so marking them as __initdata can't work.
Steps to reproduce:
modprobe decnet
cat /sys/module/decnet/parameters/addr
BUG: unable to handle kernel paging request at virtual address f88cd410
printing eip: c043dfd1 *pdpt = 0000000000004001 *pde = 0000000004408067 *pte = 0000000000000000
Oops: 0000 [#1] PREEMPT SMP
Modules linked in: decnet sunrpc af_packet ipv6 binfmt_misc dm_mirror dm_multipath dm_mod sbs sbshc fan dock battery backlight ac power_supply parport loop rtc_cmos serio_raw rtc_core rtc_lib button amd_rng sr_mod cdrom shpchp pci_hotplug ehci_hcd ohci_hcd uhci_hcd usbcore
Pid: 2099, comm: cat Not tainted (2.6.24-rc1-b1d08ac064268d0ae2281e98bf5e82627e0f0c56-bloat #6)
EIP: 0060:[<c043dfd1>] EFLAGS: 00210286 CPU: 1
EIP is at param_get_int+0x6/0x20
EAX: c5c87000 EBX: 00000000 ECX: 000080d0 EDX: f88cd410
ESI: f8a108f8 EDI: c5c87000 EBP: 00000000 ESP: c5c97f00
DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
Process cat (pid: 2099, ti=c5c97000 task=c641ee10 task.ti=c5c97000)
Stack: 00000000 f8a108f8 c5c87000 c043db6b f8a108f1 00000124 c043de1a c043db2f
f88cd410 ffffffff c5c87000 f8a16bc8 f8a16bc8 c043dd69 c043dd54 c5dd5078
c043dbc8 c5cc7580 c06ee64c c5d679f8 c04c431f c641f480 c641f484 00001000
Call Trace:
[<c043db6b>] param_array_get+0x3c/0x62
[<c043de1a>] param_array_set+0x0/0xdf
[<c043db2f>] param_array_get+0x0/0x62
[<c043dd69>] param_attr_show+0x15/0x2d
[<c043dd54>] param_attr_show+0x0/0x2d
[<c043dbc8>] module_attr_show+0x1a/0x1e
[<c04c431f>] sysfs_read_file+0x7c/0xd9
[<c04c42a3>] sysfs_read_file+0x0/0xd9
[<c048d4b2>] vfs_read+0x88/0x134
[<c042090b>] do_page_fault+0x0/0x7d5
[<c048d920>] sys_read+0x41/0x67
[<c04080fa>] sysenter_past_esp+0x6b/0xc1
=======================
Code: 00 83 c4 0c c3 83 ec 0c 8b 52 10 8b 12 c7 44 24 04 27 dd 6c c0 89 04 24 89 54 24 08 e8 ea 01 0c 00 83 c4 0c c3 83 ec 0c 8b 52 10 <8b> 12 c7 44 24 04 58 8c 6a c0 89 04 24 89 54 24 08 e8 ca 01 0c
EIP: [<c043dfd1>] param_get_int+0x6/0x20 SS:ESP 0068:c5c97f00
Signed-off-by: Alexey Dobriyan <adobriyan@sw.ru>
---
net/decnet/dn_dev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/net/decnet/dn_dev.c
+++ b/net/decnet/dn_dev.c
@@ -1439,7 +1439,7 @@ static const struct file_operations dn_dev_seq_fops = {
#endif /* CONFIG_PROC_FS */
-static int __initdata addr[2];
+static int addr[2];
module_param_array(addr, int, NULL, 0444);
MODULE_PARM_DESC(addr, "The DECnet address of this machine: area,node");
^ permalink raw reply
* Re: [PATCH] [IPv6] SNMP: Restore Udp6InErrors incrementation
From: Herbert Xu @ 2007-11-01 15:05 UTC (permalink / raw)
To: Mitsuru Chinen; +Cc: David S. Miller, netdev, YOSHIFUJI Hideaki
In-Reply-To: <20071101224638.b954a697.mitch@linux.vnet.ibm.com>
On Thu, Nov 01, 2007 at 10:46:38PM +0900, Mitsuru Chinen wrote:
> As the checksum verification is postponed till user calls recv or poll,
> the inrementation of Udp6InErrors counter should be also postponed.
> Currently, it is postponed in non-blocking operation case. However it
> should be postponed in all case like the IPv4 code.
>
> Signed-off-by: Mitsuru Chinen <mitch@linux.vnet.ibm.com>
Looks good to me. Thanks for catching this!
Cheers,
--
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
* Re: expected behavior of PF_PACKET on NETIF_F_HW_VLAN_RX device?
From: Ben Greear @ 2007-11-01 15:04 UTC (permalink / raw)
To: David Miller
Cc: shemminger, djohnson+linux-kernel, linux-kernel, netdev, bguo
In-Reply-To: <20071031.215025.195350296.davem@davemloft.net>
David Miller wrote:
> From: Stephen Hemminger <shemminger@linux-foundation.org>
> Date: Wed, 31 Oct 2007 18:23:37 -0700
>
>
>> The code in AF_PACKET should fix the skb before passing to user
>> space so that there is no difference between accel and non-accel
>> hardware. Internal choices shouldn't leak to user space. Ditto,
>> the receive checksum offload should be fixed up as well.
>>
>
> The hardware has stripped the VLAN header completely and has not
> provided it to us at all.
>
Do the NICs not save the QoS bits in the VLAN header anywhere that we could
use to reconstitute the header?
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* [PATCH] net: Add 405EX support to new EMAC driver
From: Stefan Roese @ 2007-11-01 14:54 UTC (permalink / raw)
To: netdev; +Cc: Benjamin Herrenschmidt, linuxppc-dev, Josh Boyer
This patch adds support for the 405EX to the new EMAC driver.
Tested on AMCC Kilauea.
Signed-off-by: Stefan Roese <sr@denx.de>
---
drivers/net/ibm_newemac/core.c | 3 ++-
drivers/net/ibm_newemac/rgmii.c | 6 ------
2 files changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/net/ibm_newemac/core.c b/drivers/net/ibm_newemac/core.c
index 0de3aa2..fd0a585 100644
--- a/drivers/net/ibm_newemac/core.c
+++ b/drivers/net/ibm_newemac/core.c
@@ -2466,7 +2466,8 @@ static int __devinit emac_init_config(struct emac_instance *dev)
if (of_device_is_compatible(np, "ibm,emac4"))
dev->features |= EMAC_FTR_EMAC4;
if (of_device_is_compatible(np, "ibm,emac-axon")
- || of_device_is_compatible(np, "ibm,emac-440epx"))
+ || of_device_is_compatible(np, "ibm,emac-440epx")
+ || of_device_is_compatible(np, "ibm,emac-405ex"))
dev->features |= EMAC_FTR_HAS_AXON_STACR
| EMAC_FTR_STACR_OC_INVERT;
if (of_device_is_compatible(np, "ibm,emac-440spe"))
diff --git a/drivers/net/ibm_newemac/rgmii.c b/drivers/net/ibm_newemac/rgmii.c
index de41695..e393f68 100644
--- a/drivers/net/ibm_newemac/rgmii.c
+++ b/drivers/net/ibm_newemac/rgmii.c
@@ -140,9 +140,6 @@ void rgmii_get_mdio(struct of_device *ofdev, int input)
RGMII_DBG2(dev, "get_mdio(%d)" NL, input);
- if (dev->type != RGMII_AXON)
- return;
-
mutex_lock(&dev->lock);
fer = in_be32(&p->fer);
@@ -161,9 +158,6 @@ void rgmii_put_mdio(struct of_device *ofdev, int input)
RGMII_DBG2(dev, "put_mdio(%d)" NL, input);
- if (dev->type != RGMII_AXON)
- return;
-
fer = in_be32(&p->fer);
fer &= ~(0x00080000u >> input);
out_be32(&p->fer, fer);
--
1.5.3.4.498.g9c514
^ permalink raw reply related
* [PATCH 2/2] [POWERPC] Fix region size check in mpc5200 FEC driver
From: Grant Likely @ 2007-11-01 14:22 UTC (permalink / raw)
To: netdev, jgarzik, domen.puncer
In-Reply-To: <20071101142005.7620.54250.stgit@trillian.cg.shawcable.net>
From: Grant Likely <grant.likely@secretlab.ca>
Driver shouldn't complain if the register range is larger than what
it expects. This works around failures with some device trees.
Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---
drivers/net/fec_mpc52xx.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/fec_mpc52xx.c b/drivers/net/fec_mpc52xx.c
index fc1cf0b..a8a0ee2 100644
--- a/drivers/net/fec_mpc52xx.c
+++ b/drivers/net/fec_mpc52xx.c
@@ -879,9 +879,9 @@ mpc52xx_fec_probe(struct of_device *op, const struct of_device_id *match)
"Error while parsing device node resource\n" );
return rv;
}
- if ((mem.end - mem.start + 1) != sizeof(struct mpc52xx_fec)) {
+ if ((mem.end - mem.start + 1) < sizeof(struct mpc52xx_fec)) {
printk(KERN_ERR DRIVER_NAME
- " - invalid resource size (%lx != %x), check mpc52xx_devices.c\n",
+ " - invalid resource size (%lx < %x), check mpc52xx_devices.c\n",
(unsigned long)(mem.end - mem.start + 1), sizeof(struct mpc52xx_fec));
return -EINVAL;
}
^ permalink raw reply related
* [PATCH 0/2] Fixes to MPC5200 FEC driver
From: Grant Likely @ 2007-11-01 14:22 UTC (permalink / raw)
To: netdev, jgarzik, domen.puncer
Oops, send this series yesterday but forgot to include jgarzik, domen
and netdev to the 'to:' list.
These are fixes which should go in for .24
Cheers,
g.
--
Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox