* Re: [PATCH v7 01/16] hashtable: introduce a small and naive hashtable
From: Sasha Levin @ 2012-10-29 16:06 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: torvalds, tj, akpm, linux-kernel, linux-mm, paul.gortmaker, davem,
rostedt, mingo, ebiederm, aarcange, ericvh, netdev, josh,
eric.dumazet, axboe, agk, dm-devel, neilb, ccaulfie, teigland,
Trond.Myklebust, bfields, fweisbec, jesse, venkat.x.venkatsubra,
ejt, snitzer, edumazet, linux-nfs, dev, rds-devel, lw
In-Reply-To: <20121029112907.GA9115@Krystal>
On Mon, Oct 29, 2012 at 7:29 AM, Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
> * Sasha Levin (levinsasha928@gmail.com) wrote:
>> +
>> + for (i = 0; i < sz; i++)
>> + INIT_HLIST_HEAD(&ht[sz]);
>
> ouch. How did this work ? Has it been tested at all ?
>
> sz -> i
Funny enough, it works perfectly. Generally as a test I boot the
kernel in a VM and let it fuzz with trinity for a bit, doing that with
the code above worked flawlessly.
While it works, it's obviously wrong. Why does it work though? Usually
there's a list op happening pretty soon after that which brings the
list into proper state.
I've been playing with a patch that adds a magic value into list_head
if CONFIG_DEBUG_LIST is set, and checks that magic in the list debug
code in lib/list_debug.c.
Does it sound like something useful? If so I'll send that patch out.
Thanks,
Sasha
--
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 v7 10/16] dlm: use new hashtable implementation
From: Mathieu Desnoyers @ 2012-10-29 16:07 UTC (permalink / raw)
To: Sasha Levin
Cc: torvalds, tj, akpm, linux-kernel, linux-mm, paul.gortmaker, davem,
rostedt, mingo, ebiederm, aarcange, ericvh, netdev, josh,
eric.dumazet, axboe, agk, dm-devel, neilb, ccaulfie, teigland,
Trond.Myklebust, bfields, fweisbec, jesse, venkat.x.venkatsubra,
ejt, snitzer, edumazet, linux-nfs, dev, rds-devel, lw
In-Reply-To: <CA+1xoqfxgB+8BybPpf+jwT-ObfGPxnbKvkz1MUMuJuR8NDSNaw@mail.gmail.com>
* Sasha Levin (levinsasha928@gmail.com) wrote:
> On Mon, Oct 29, 2012 at 9:07 AM, Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
> > * Mathieu Desnoyers (mathieu.desnoyers@efficios.com) wrote:
> >> * Sasha Levin (levinsasha928@gmail.com) wrote:
> >> [...]
> >> > @@ -158,34 +159,21 @@ static int dlm_allow_conn;
> >> > static struct workqueue_struct *recv_workqueue;
> >> > static struct workqueue_struct *send_workqueue;
> >> >
> >> > -static struct hlist_head connection_hash[CONN_HASH_SIZE];
> >> > +static struct hlist_head connection_hash[CONN_HASH_BITS];
> >> > static DEFINE_MUTEX(connections_lock);
> >> > static struct kmem_cache *con_cache;
> >> >
> >> > static void process_recv_sockets(struct work_struct *work);
> >> > static void process_send_sockets(struct work_struct *work);
> >> >
> >> > -
> >> > -/* This is deliberately very simple because most clusters have simple
> >> > - sequential nodeids, so we should be able to go straight to a connection
> >> > - struct in the array */
> >> > -static inline int nodeid_hash(int nodeid)
> >> > -{
> >> > - return nodeid & (CONN_HASH_SIZE-1);
> >> > -}
> >>
> >> There is one thing I dislike about this change: you remove a useful
> >> comment. It's good to be informed of the reason why a direct mapping
> >> "value -> hash" without any dispersion function is preferred here.
>
> Yes, I've removed the comment because it's no longer true with the patch :)
>
> > And now that I come to think of it: you're changing the behavior : you
> > will now use a dispersion function on the key, which goes against the
> > intent expressed in this comment.
>
> The comment gave us the information that nodeids are mostly
> sequential, we no longer need to rely on that.
I'm fine with turning a direct + modulo mapping into a dispersed hash as
long as there are no underlying assumptions about sequentiality of value
accesses.
If the access pattern would happen to be typically sequential, then
adding dispersion could hurt performances significantly, turning a
frequent L1 access into a L2 access for instance.
>
> > It might be good to change hash_add(), hash_add_rcu(),
> > hash_for_each_possible*() key parameter for a "hash" parameter, and let
> > the caller provide the hash value computed by the function they like as
> > parameter, rather than enforcing hash_32/hash_64.
>
> Why? We already proved that hash_32() is more than enough as a hashing
> function, why complicate things?
>
> Even doing hash_32() on top of another hash is probably a good idea to
> keep things simple.
All I'm asking is: have you made sure that this hash table is not
deliberately kept sequential (without dispersion) to accelerate specific
access patterns ? This should at least be documented in the changelog.
Thanks,
Mathieu
>
> Thanks,
> Sasha
--
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
--
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 v7 01/16] hashtable: introduce a small and naive hashtable
From: Mathieu Desnoyers @ 2012-10-29 16:14 UTC (permalink / raw)
To: Sasha Levin
Cc: torvalds, tj, akpm, linux-kernel, linux-mm, paul.gortmaker, davem,
rostedt, mingo, ebiederm, aarcange, ericvh, netdev, josh,
eric.dumazet, axboe, agk, dm-devel, neilb, ccaulfie, teigland,
Trond.Myklebust, bfields, fweisbec, jesse, venkat.x.venkatsubra,
ejt, snitzer, edumazet, linux-nfs, dev, rds-devel, lw
In-Reply-To: <CA+1xoqfQn92igbFS1TtrpYuSiy7+Ro02ar=axgqSOJOuE_EVuA@mail.gmail.com>
* Sasha Levin (levinsasha928@gmail.com) wrote:
> On Mon, Oct 29, 2012 at 7:29 AM, Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
> > * Sasha Levin (levinsasha928@gmail.com) wrote:
> >> +
> >> + for (i = 0; i < sz; i++)
> >> + INIT_HLIST_HEAD(&ht[sz]);
> >
> > ouch. How did this work ? Has it been tested at all ?
> >
> > sz -> i
>
> Funny enough, it works perfectly. Generally as a test I boot the
> kernel in a VM and let it fuzz with trinity for a bit, doing that with
> the code above worked flawlessly.
>
> While it works, it's obviously wrong. Why does it work though? Usually
> there's a list op happening pretty soon after that which brings the
> list into proper state.
>
> I've been playing with a patch that adds a magic value into list_head
> if CONFIG_DEBUG_LIST is set, and checks that magic in the list debug
> code in lib/list_debug.c.
>
> Does it sound like something useful? If so I'll send that patch out.
Most of the calls to this initialization function apply it on zeroed
memory (static/kzalloc'd...), which makes it useless. I'd actually be in
favor of removing those redundant calls (as I pointed out in another
email), and document that zeroed memory don't need to be explicitly
initialized.
Those sites that need to really reinitialize memory, or initialize it
(if located on the stack or in non-zeroed dynamically allocated memory)
could use a memset to 0, which will likely be faster than setting to
NULL on many architectures.
About testing, I'd recommend taking the few sites that still need the
initialization function, and just initialize the array with garbage
before calling the initialization function. Things should blow up quite
quickly. Doing it as a one-off thing might be enough to catch any issue.
I don't think we need extra magic numbers to catch issues in this rather
obvious init function.
Thanks,
Mathieu
>
>
> Thanks,
> Sasha
--
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
--
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 v7 01/16] hashtable: introduce a small and naive hashtable
From: Tejun Heo @ 2012-10-29 16:18 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: snitzer-H+wXaHxf7aLQT0dZR+AlfA, neilb-l3A5Bk7waGM,
fweisbec-Re5JQEeQqe8AvxtiuMwx3w,
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA,
bfields-uC3wQj2KruNg9hUCZPvPmw,
paul.gortmaker-CWA4WttNNZF54TAoqtyWWQ,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA, agk-H+wXaHxf7aLQT0dZR+AlfA,
aarcange-H+wXaHxf7aLQT0dZR+AlfA, rds-devel-N0ozoZBvEnrZJqsBc5GL+g,
eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w,
venkat.x.venkatsubra-QHcLZuEGTsvQT0dZR+AlfA,
ccaulfie-H+wXaHxf7aLQT0dZR+AlfA, mingo-X9Un+BFzKDI,
dev-yBygre7rU0TnMu66kgdUjQ, ericvh-Re5JQEeQqe8AvxtiuMwx3w,
josh-iaAMLnmF4UmaiuxdJuQwMA, rostedt-nx8X9YLhiw1AfugRpC6u6w,
teigland-H+wXaHxf7aLQT0dZR+AlfA, Sasha Levin,
axboe-tSWWG44O7X1aa/9Udqfwiw, linux-nfs-u79uwXL29TY76Z2rM5mHXA,
edumazet-hpIqsD4AKlfQT0dZR+AlfA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, ejt-H+wXaHxf7aLQT0dZR+AlfA,
ebiederm-aS9lmoZGLiVWk0Htik3J/w, lw-BthXqXjhjHXQFUHtdCDX3A,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <20121029161412.GB18944@Krystal>
Hello,
On Mon, Oct 29, 2012 at 12:14:12PM -0400, Mathieu Desnoyers wrote:
> Most of the calls to this initialization function apply it on zeroed
> memory (static/kzalloc'd...), which makes it useless. I'd actually be in
> favor of removing those redundant calls (as I pointed out in another
> email), and document that zeroed memory don't need to be explicitly
> initialized.
>
> Those sites that need to really reinitialize memory, or initialize it
> (if located on the stack or in non-zeroed dynamically allocated memory)
> could use a memset to 0, which will likely be faster than setting to
> NULL on many architectures.
I don't think it's a good idea to optimize out the basic encapsulation
there. We're talking about re-zeroing some static memory areas which
are pretty small. It's just not worth optimizing out at the cost of
proper initializtion. e.g. We might add debug fields to list_head
later.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH v7 01/16] hashtable: introduce a small and naive hashtable
From: Mathieu Desnoyers @ 2012-10-29 16:22 UTC (permalink / raw)
To: Tejun Heo
Cc: Sasha Levin, torvalds, akpm, linux-kernel, linux-mm,
paul.gortmaker, davem, rostedt, mingo, ebiederm, aarcange, ericvh,
netdev, josh, eric.dumazet, axboe, agk, dm-devel, neilb, ccaulfie,
teigland, Trond.Myklebust, bfields, fweisbec, jesse,
venkat.x.venkatsubra, ejt, snitzer, edumazet, linux-nfs, dev,
rds-devel, lw
In-Reply-To: <20121029161809.GA4066@htj.dyndns.org>
* Tejun Heo (tj@kernel.org) wrote:
> Hello,
>
> On Mon, Oct 29, 2012 at 12:14:12PM -0400, Mathieu Desnoyers wrote:
> > Most of the calls to this initialization function apply it on zeroed
> > memory (static/kzalloc'd...), which makes it useless. I'd actually be in
> > favor of removing those redundant calls (as I pointed out in another
> > email), and document that zeroed memory don't need to be explicitly
> > initialized.
> >
> > Those sites that need to really reinitialize memory, or initialize it
> > (if located on the stack or in non-zeroed dynamically allocated memory)
> > could use a memset to 0, which will likely be faster than setting to
> > NULL on many architectures.
>
> I don't think it's a good idea to optimize out the basic encapsulation
> there. We're talking about re-zeroing some static memory areas which
> are pretty small. It's just not worth optimizing out at the cost of
> proper initializtion. e.g. We might add debug fields to list_head
> later.
Future-proofness for debugging fields is indeed a very compelling
argument. Fair enough!
We might want to document this intent at the top of the initialization
function though, just in case anyone want to short-circuit it.
Thanks,
Mathieu
--
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
--
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 v7 10/16] dlm: use new hashtable implementation
From: David Teigland @ 2012-10-29 16:23 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Sasha Levin, torvalds, tj, akpm, linux-kernel, linux-mm,
paul.gortmaker, davem, rostedt, mingo, ebiederm, aarcange, ericvh,
netdev, josh, eric.dumazet, axboe, agk, dm-devel, neilb, ccaulfie,
Trond.Myklebust, bfields, fweisbec, jesse, venkat.x.venkatsubra,
ejt, snitzer, edumazet, linux-nfs, dev, rds-devel, lw
In-Reply-To: <20121029160710.GA18944@Krystal>
On Mon, Oct 29, 2012 at 12:07:10PM -0400, Mathieu Desnoyers wrote:
> I'm fine with turning a direct + modulo mapping into a dispersed hash as
> long as there are no underlying assumptions about sequentiality of value
> accesses.
>
> If the access pattern would happen to be typically sequential, then
> adding dispersion could hurt performances significantly, turning a
> frequent L1 access into a L2 access for instance.
> All I'm asking is: have you made sure that this hash table is not
> deliberately kept sequential (without dispersion) to accelerate specific
> access patterns ? This should at least be documented in the changelog.
It was not intentional. I don't expect any benefit would be lost by
making it non-sequential.
--
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 v7 01/16] hashtable: introduce a small and naive hashtable
From: Sasha Levin @ 2012-10-29 16:26 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: torvalds, tj, akpm, linux-kernel, linux-mm, paul.gortmaker, davem,
rostedt, mingo, ebiederm, aarcange, ericvh, netdev, josh,
eric.dumazet, axboe, agk, dm-devel, neilb, ccaulfie, teigland,
Trond.Myklebust, bfields, fweisbec, jesse, venkat.x.venkatsubra,
ejt, snitzer, edumazet, linux-nfs, dev, rds-devel, lw
In-Reply-To: <20121029161412.GB18944@Krystal>
On Mon, Oct 29, 2012 at 12:14 PM, Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
> * Sasha Levin (levinsasha928@gmail.com) wrote:
>> On Mon, Oct 29, 2012 at 7:29 AM, Mathieu Desnoyers
>> <mathieu.desnoyers@efficios.com> wrote:
>> > * Sasha Levin (levinsasha928@gmail.com) wrote:
>> >> +
>> >> + for (i = 0; i < sz; i++)
>> >> + INIT_HLIST_HEAD(&ht[sz]);
>> >
>> > ouch. How did this work ? Has it been tested at all ?
>> >
>> > sz -> i
>>
>> Funny enough, it works perfectly. Generally as a test I boot the
>> kernel in a VM and let it fuzz with trinity for a bit, doing that with
>> the code above worked flawlessly.
>>
>> While it works, it's obviously wrong. Why does it work though? Usually
>> there's a list op happening pretty soon after that which brings the
>> list into proper state.
>>
>> I've been playing with a patch that adds a magic value into list_head
>> if CONFIG_DEBUG_LIST is set, and checks that magic in the list debug
>> code in lib/list_debug.c.
>>
>> Does it sound like something useful? If so I'll send that patch out.
>
> Most of the calls to this initialization function apply it on zeroed
> memory (static/kzalloc'd...), which makes it useless. I'd actually be in
> favor of removing those redundant calls (as I pointed out in another
> email), and document that zeroed memory don't need to be explicitly
> initialized.
Why would that make it useless? The idea is that the init functions
will set the magic field to something random, like:
.magic = 0xBADBEEF0;
And have list_add() and friends WARN(.magic != 0xBADBEEF0, "Using an
uninitialized list\n");
This way we'll catch all places that don't go through list initialization code.
Thanks,
Sasha
--
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: [net-next 02/12] ixgbe: Fix return value from macvlan filter function
From: Rose, Gregory V @ 2012-10-29 16:27 UTC (permalink / raw)
To: Konstantin Stepanyuk, Kirsher, Jeffrey T
Cc: davem@davemloft.net, netdev@vger.kernel.org, gospo@redhat.com,
sassmann@redhat.com
In-Reply-To: <CAKgsOu-PH2OJRKxLWj=UEp=XuQCpu0e_pNL2_Nkbbz8Hx2bhEw@mail.gmail.com>
> -----Original Message-----
> From: Konstantin Stepanyuk [mailto:konstantin.stepanyuk@gmail.com]
> Sent: Saturday, October 27, 2012 11:37 AM
> To: Kirsher, Jeffrey T
> Cc: davem@davemloft.net; Rose, Gregory V; netdev@vger.kernel.org;
> gospo@redhat.com; sassmann@redhat.com
> Subject: Re: [net-next 02/12] ixgbe: Fix return value from macvlan filter
> function
>
> On Fri, Oct 26, 2012 at 6:08 PM, Jeff Kirsher
> <jeffrey.t.kirsher@intel.com> wrote:
> > From: Greg Rose <gregory.v.rose@intel.com>
> >
> > The function to set the macvlan filter should return success or
> > failure instead of the index of the filter. The message processing
> > function was misinterpreting the index as a non-zero return code
> > indicating failure and NACKing MAC filter set messages that actually
> succeeded.
> >
> > Signed-off-by: Greg Rose <gregory.v.rose@intel.com>
> > Tested-by: Robert Garrett <robertx.e.garrett@intel.com>
> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> > ---
> > drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
> > b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
> > index 96876b7..b68bf0f 100644
> > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
> > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
> > @@ -742,7 +742,8 @@ static int ixgbe_set_vf_macvlan_msg(struct
> ixgbe_adapter *adapter,
> > e_warn(drv,
> > "VF %d has requested a MACVLAN filter but there
> is no space for it\n",
> > vf);
> > - return err;
> > +
> > + return !!err < 0;
> This expression always resolves to false, either 0 < 0 or 1 < 0, because
> '!' has higher precedence than '<'.
It's a typo. I'll fix it.
- Greg
>
> Thanks,
> Konstantin
^ permalink raw reply
* Re: [PATCH v7 09/16] SUNRPC/cache: use new hashtable implementation
From: Andrew Morton @ 2012-10-29 16:27 UTC (permalink / raw)
To: Linus Torvalds
Cc: snitzer-H+wXaHxf7aLQT0dZR+AlfA, neilb-l3A5Bk7waGM,
fweisbec-Re5JQEeQqe8AvxtiuMwx3w,
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA,
bfields-uC3wQj2KruNg9hUCZPvPmw,
paul.gortmaker-CWA4WttNNZF54TAoqtyWWQ,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA, agk-H+wXaHxf7aLQT0dZR+AlfA,
aarcange-H+wXaHxf7aLQT0dZR+AlfA, rds-devel-N0ozoZBvEnrZJqsBc5GL+g,
eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w,
venkat.x.venkatsubra-QHcLZuEGTsvQT0dZR+AlfA,
ccaulfie-H+wXaHxf7aLQT0dZR+AlfA, mingo-X9Un+BFzKDI,
dev-yBygre7rU0TnMu66kgdUjQ, ericvh-Re5JQEeQqe8AvxtiuMwx3w,
josh-iaAMLnmF4UmaiuxdJuQwMA, rostedt-nx8X9YLhiw1AfugRpC6u6w,
lw-BthXqXjhjHXQFUHtdCDX3A, Mathieu Desnoyers, Sasha Levin,
axboe-tSWWG44O7X1aa/9Udqfwiw, linux-nfs-u79uwXL29TY76Z2rM5mHXA,
edumazet-hpIqsD4AKlfQT0dZR+AlfA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, ejt-H+wXaHxf7aLQT0dZR+AlfA,
ebiederm-aS9lmoZGLiVWk0Htik3J/w, tj-DgEjT+Ai2ygdnm+yROfE0A,
teigland-H+wXaHxf7aLQT0dZR+AlfA, davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <CA+55aFzO8DJJP3HBfgqXFac9r3=bYK+_nYe4cuXiNFg-623s6w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Mon, 29 Oct 2012 07:49:42 -0700 Linus Torvalds <torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> wrote:
> Because there's no reason to believe that '9' is in any way a worse
> random number than something page-shift-related, is there?
9 is much better than PAGE_SHIFT. PAGE_SIZE can vary by a factor of
16, depending on config.
Everyone thinks 4k, and tests only for that. There's potential for
very large performance and behavior changes when their code gets run
on a 64k PAGE_SIZE machine.
^ permalink raw reply
* Re: [PATCH v7 01/16] hashtable: introduce a small and naive hashtable
From: Mathieu Desnoyers @ 2012-10-29 16:29 UTC (permalink / raw)
To: Sasha Levin
Cc: snitzer-H+wXaHxf7aLQT0dZR+AlfA, neilb-l3A5Bk7waGM,
fweisbec-Re5JQEeQqe8AvxtiuMwx3w,
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA,
bfields-uC3wQj2KruNg9hUCZPvPmw,
paul.gortmaker-CWA4WttNNZF54TAoqtyWWQ,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA, agk-H+wXaHxf7aLQT0dZR+AlfA,
aarcange-H+wXaHxf7aLQT0dZR+AlfA, rds-devel-N0ozoZBvEnrZJqsBc5GL+g,
eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w,
venkat.x.venkatsubra-QHcLZuEGTsvQT0dZR+AlfA,
ccaulfie-H+wXaHxf7aLQT0dZR+AlfA, mingo-X9Un+BFzKDI,
dev-yBygre7rU0TnMu66kgdUjQ, ericvh-Re5JQEeQqe8AvxtiuMwx3w,
josh-iaAMLnmF4UmaiuxdJuQwMA, rostedt-nx8X9YLhiw1AfugRpC6u6w,
lw-BthXqXjhjHXQFUHtdCDX3A, teigland-H+wXaHxf7aLQT0dZR+AlfA,
axboe-tSWWG44O7X1aa/9Udqfwiw, linux-nfs-u79uwXL29TY76Z2rM5mHXA,
edumazet-hpIqsD4AKlfQT0dZR+AlfA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, ejt-H+wXaHxf7aLQT0dZR+AlfA,
ebiederm-aS9lmoZGLiVWk0Htik3J/w, tj-DgEjT+Ai2ygdnm+yROfE0A,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <CA+1xoqfBXM4sjvcZtUncnWAaUxA9_YBod3Hjx3ZO=K1oJO_j7g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
* Sasha Levin (levinsasha928-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org) wrote:
> On Mon, Oct 29, 2012 at 12:14 PM, Mathieu Desnoyers
> <mathieu.desnoyers-vg+e7yoeK/dWk0Htik3J/w@public.gmane.org> wrote:
> > * Sasha Levin (levinsasha928-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org) wrote:
> >> On Mon, Oct 29, 2012 at 7:29 AM, Mathieu Desnoyers
> >> <mathieu.desnoyers-vg+e7yoeK/dWk0Htik3J/w@public.gmane.org> wrote:
> >> > * Sasha Levin (levinsasha928-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org) wrote:
> >> >> +
> >> >> + for (i = 0; i < sz; i++)
> >> >> + INIT_HLIST_HEAD(&ht[sz]);
> >> >
> >> > ouch. How did this work ? Has it been tested at all ?
> >> >
> >> > sz -> i
> >>
> >> Funny enough, it works perfectly. Generally as a test I boot the
> >> kernel in a VM and let it fuzz with trinity for a bit, doing that with
> >> the code above worked flawlessly.
> >>
> >> While it works, it's obviously wrong. Why does it work though? Usually
> >> there's a list op happening pretty soon after that which brings the
> >> list into proper state.
> >>
> >> I've been playing with a patch that adds a magic value into list_head
> >> if CONFIG_DEBUG_LIST is set, and checks that magic in the list debug
> >> code in lib/list_debug.c.
> >>
> >> Does it sound like something useful? If so I'll send that patch out.
> >
> > Most of the calls to this initialization function apply it on zeroed
> > memory (static/kzalloc'd...), which makes it useless. I'd actually be in
> > favor of removing those redundant calls (as I pointed out in another
> > email), and document that zeroed memory don't need to be explicitly
> > initialized.
>
> Why would that make it useless? The idea is that the init functions
> will set the magic field to something random, like:
>
> .magic = 0xBADBEEF0;
>
> And have list_add() and friends WARN(.magic != 0xBADBEEF0, "Using an
> uninitialized list\n");
>
> This way we'll catch all places that don't go through list initialization code.
As I replied to Tejun Heo already, I agree that keeping the
initialization in place makes sense for future-proofness. This intent
should probably be documented in a comment about the initialization
function though, just to make sure nobody will try to skip it.
Thanks,
Mathieu
>
>
> Thanks,
> Sasha
--
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [PATCH v2] sctp: Clean up type-punning in sctp_cmd_t union
From: Neil Horman @ 2012-10-29 16:35 UTC (permalink / raw)
To: Vlad Yasevich; +Cc: netdev, David S. Miller, linux-sctp
In-Reply-To: <508AE0B6.2020506@gmail.com>
On Fri, Oct 26, 2012 at 03:12:54PM -0400, Vlad Yasevich wrote:
> On 10/26/2012 09:41 AM, Neil Horman wrote:
> >Lots of points in the sctp_cmd_interpreter function treat the sctp_cmd_t arg as
> >a void pointer, even though they are written as various other types. Theres no
> >need for this as doing so just leads to possible type-punning issues that could
> >cause crashes, and if we remain type-consistent we can actually just remove the
> >void * member of the union entirely.
> >
> >Change Notes:
> >
> >v2)
> > * Dropped chunk that modified SCTP_NULL to create a marker pattern
> > should anyone try to use a SCTP_NULL() assigned sctp_arg_t, Assigning
> > to .zero provides the same effect and should be faster, per Vlad Y.
> >
> >Signed-off-by: Neil Horman <nhorman@tuxdriver.com
> >CC: Vlad Yasevich <vyasevich@gmail.com>
>
> Acked-by: Vlad Yasevich <vyasevich@gmail.com>
>
> -vlad
>
Thanks for the ACk Vlad, do you have any thoughts regarding Dave M's comments
above on this .zero/memset question? He makes a good point regarding complete
initalization I think.
Neil
^ permalink raw reply
* Re: [PATCH net-next] sctp: support per-association stats via a new SCTP_GET_ASSOC_STATS call
From: Neil Horman @ 2012-10-29 16:38 UTC (permalink / raw)
To: Vlad Yasevich
Cc: Michele Baldessari, linux-sctp@vger.kernel.org, David S. Miller,
netdev@vger.kernel.org, Thomas Graf
In-Reply-To: <DB10D819-2751-4467-858A-C4ED1DBED9F4@gmail.com>
On Sat, Oct 27, 2012 at 11:48:33AM -0400, Vlad Yasevich wrote:
>
>
>
>
> On Oct 27, 2012, at 7:35 AM, Michele Baldessari <michele@acksyn.org> wrote:
>
> > Hi Neil & Vlad,
> >
> > On Fri, Oct 26, 2012 at 10:37:04AM -0400, Neil Horman wrote:
> >> We already have files in /proc/net/sctp to count snmp system-wide totals,
> >> per-endpoint totals, and per association totals. Why do these stats differently
> >> instead of just adding them the per-association file? I get that solaris does
> >> this, but its not codified in any of the RFC's or other standards. I would
> >> really rather see something like this go into the interfaces we have, rather
> >> than creating a new one.
> >>
> >> I also am a bit confused regarding the stats themselves. Most are fairly clear,
> >> but some seem lacking (you count most things sent and received, but only count
> >> received gap acks). Others seems vague and or confusing (when counting
> >> retransmitted chunks and packets, how do you count a packet that has both new
> >> and retransmitted chunks)? And the max observed rto stat is just odd. Each
> >> transport has an rto value, not each association, and you cal already see the
> >> individual transport rto values in /proc/net/sctp/remaddr.
> >
> > thanks a lot for your time reviewing this. I will try to address all
> > your comments in a second version of the patch. One thing I am not too
> > sure though: do you prefer me extending /proc/net/sctp/* or implement a
> > new call.
> >
> > I ask because from a previous private communication with Vlad the new
> > socket option seemed to be the preferred approach.
> > I am fine either way just let me know ;)
>
>
> socket option is preferable as /proc doesn't scale very well as number of associations grows.
>
> -Vlad
>
I completely agree with that notion, but at the same time, the socket option is
limited in that these stats will only be accessible to the process that owns the
socket. I imagine that someone will eventually ask for these stats to be made
available to utilities outside of the owning socket.
Neil
> >
> > cheers,
> > --
> > Michele Baldessari <michele@acksyn.org>
> > C2A5 9DA3 9961 4FFB E01B D0BC DDD4 DCCB 7515 5C6D
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" 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
* [PATCH] net: dsa/slave: Fix compilation warnings
From: Viresh Kumar @ 2012-10-29 16:57 UTC (permalink / raw)
To: davem; +Cc: linaro-dev, patches, netdev, linux-kernel, Viresh Kumar
Currently when none of CONFIG_NET_DSA_TAG_DSA, CONFIG_NET_DSA_TAG_EDSA and
CONFIG_NET_DSA_TAG_TRAILER is defined, we get following compilation warnings:
net/dsa/slave.c:51:12: warning: 'dsa_slave_init' defined but not used [-Wunused-function]
net/dsa/slave.c:60:12: warning: 'dsa_slave_open' defined but not used [-Wunused-function]
net/dsa/slave.c:98:12: warning: 'dsa_slave_close' defined but not used [-Wunused-function]
net/dsa/slave.c:116:13: warning: 'dsa_slave_change_rx_flags' defined but not used [-Wunused-function]
net/dsa/slave.c:127:13: warning: 'dsa_slave_set_rx_mode' defined but not used [-Wunused-function]
net/dsa/slave.c:136:12: warning: 'dsa_slave_set_mac_address' defined but not used [-Wunused-function]
net/dsa/slave.c:164:12: warning: 'dsa_slave_ioctl' defined but not used [-Wunused-function]
Fix them by enclosing these routines under #ifdef,endif.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
net/dsa/slave.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index e32083d..5606fae 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -48,6 +48,8 @@ void dsa_slave_mii_bus_init(struct dsa_switch *ds)
/* slave device handling ****************************************************/
+#if defined(CONFIG_NET_DSA_TAG_DSA) || defined(CONFIG_NET_DSA_TAG_EDSA) || \
+ defined(CONFIG_NET_DSA_TAG_TRAILER)
static int dsa_slave_init(struct net_device *dev)
{
struct dsa_slave_priv *p = netdev_priv(dev);
@@ -170,6 +172,8 @@ static int dsa_slave_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
return -EOPNOTSUPP;
}
+#endif /* defined(CONFIG_NET_DSA_TAG_DSA) || defined(CONFIG_NET_DSA_TAG_EDSA ||
+ defined(CONFIG_NET_DSA_TAG_TRAILER) */
/* ethtool operations *******************************************************/
--
1.7.12.rc2.18.g61b472e
^ permalink raw reply related
* Re: [Patch net-next] ipv6: fix a potential NULL deref
From: David Miller @ 2012-10-29 17:22 UTC (permalink / raw)
To: amwang; +Cc: eric.dumazet, netdev
In-Reply-To: <1351493366.8221.2.camel@cr0>
From: Cong Wang <amwang@redhat.com>
Date: Mon, 29 Oct 2012 14:49:26 +0800
> On Mon, 2012-10-29 at 07:10 +0100, Eric Dumazet wrote:
>> > - dst_release(&rt->dst);
>> > + if (rt)
>> > + dst_release(&rt->dst);
>> > }
>> >
>>
>> dst_release() is like kfree(), it accepts a NULL argument.
>>
>
> 'rt->dst' already dereferences 'rt', no matter dst_release() accepts
> NULL or not.
It's taking the address of a struct member, it's not a dereference.
You know what the difference is right?
^ permalink raw reply
* Re: [PATCH v7 06/16] tracepoint: use new hashtable implementation
From: Sasha Levin @ 2012-10-29 17:29 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: torvalds, tj, akpm, linux-kernel, linux-mm, paul.gortmaker, davem,
rostedt, mingo, ebiederm, aarcange, ericvh, netdev, josh,
eric.dumazet, axboe, agk, dm-devel, neilb, ccaulfie, teigland,
Trond.Myklebust, bfields, fweisbec, jesse, venkat.x.venkatsubra,
ejt, snitzer, edumazet, linux-nfs, dev, rds-devel, lw
In-Reply-To: <20121029113515.GB9115@Krystal>
On Mon, Oct 29, 2012 at 7:35 AM, Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
> * Sasha Levin (levinsasha928@gmail.com) wrote:
>> Switch tracepoints to use the new hashtable implementation. This reduces the amount of
>> generic unrelated code in the tracepoints.
>>
>> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
>> ---
>> kernel/tracepoint.c | 27 +++++++++++----------------
>> 1 file changed, 11 insertions(+), 16 deletions(-)
>>
>> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
>> index d96ba22..854df92 100644
>> --- a/kernel/tracepoint.c
>> +++ b/kernel/tracepoint.c
>> @@ -26,6 +26,7 @@
>> #include <linux/slab.h>
>> #include <linux/sched.h>
>> #include <linux/static_key.h>
>> +#include <linux/hashtable.h>
>>
>> extern struct tracepoint * const __start___tracepoints_ptrs[];
>> extern struct tracepoint * const __stop___tracepoints_ptrs[];
>> @@ -49,8 +50,7 @@ static LIST_HEAD(tracepoint_module_list);
>> * Protected by tracepoints_mutex.
>> */
>> #define TRACEPOINT_HASH_BITS 6
>> -#define TRACEPOINT_TABLE_SIZE (1 << TRACEPOINT_HASH_BITS)
>> -static struct hlist_head tracepoint_table[TRACEPOINT_TABLE_SIZE];
>> +static DEFINE_HASHTABLE(tracepoint_table, TRACEPOINT_HASH_BITS);
>>
> [...]
>>
>> @@ -722,6 +715,8 @@ struct notifier_block tracepoint_module_nb = {
>>
>> static int init_tracepoints(void)
>> {
>> + hash_init(tracepoint_table);
>> +
>> return register_module_notifier(&tracepoint_module_nb);
>> }
>> __initcall(init_tracepoints);
>
> So we have a hash table defined in .bss (therefore entirely initialized
> to NULL), and you add a call to "hash_init", which iterates on the whole
> array and initialize it to NULL (again) ?
>
> This extra initialization is redundant. I think it should be removed
> from here, and hashtable.h should document that hash_init() don't need
> to be called on zeroed memory (which includes static/global variables,
> kzalloc'd memory, etc).
This was discussed in the previous series, the conclusion was to call
hash_init() either way to keep the encapsulation and consistency.
It's cheap enough and happens only once, so why not?
Thanks,
Sasha
--
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 net-next] ipv6: fix a potential NULL deref
From: David Miller @ 2012-10-29 17:29 UTC (permalink / raw)
To: amwang; +Cc: eric.dumazet, netdev
In-Reply-To: <1351495523.8221.8.camel@cr0>
From: Cong Wang <amwang@redhat.com>
Date: Mon, 29 Oct 2012 15:25:23 +0800
> But this will be a problem if someone moved dst inside rt, as there
> is no comment saying dst has to be the first one?
If we move the initial dst member, many things that depend upon it
being first will have to change.
Your change was unnecessary and inappropriate, and let's just leave
it at that.
Thanks.
^ permalink raw reply
* [PATCH] vmxnet3: must split too big fragments
From: Eric Dumazet @ 2012-10-29 17:30 UTC (permalink / raw)
To: Shreyas Bhatewara, David Miller
Cc: VMware, Inc., netdev, linux-kernel, jongman heo
In-Reply-To: <1351021160.8609.2503.camel@edumazet-glaptop>
From: Eric Dumazet <edumazet@google.com>
vmxnet3 has a 16Kbytes limit per tx descriptor, that happened to work
as long as we provided PAGE_SIZE fragments.
Our stack can now build larger fragments, so we need to split them to
the 16kbytes boundary.
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: jongman heo <jongman.heo@samsung.com>
Tested-by: jongman heo <jongman.heo@samsung.com>
Cc: Shreyas Bhatewara <sbhatewara@vmware.com>
---
drivers/net/vmxnet3/vmxnet3_drv.c | 65 +++++++++++++++++++---------
1 file changed, 45 insertions(+), 20 deletions(-)
diff --git a/drivers/net/vmxnet3/vmxnet3_drv.c b/drivers/net/vmxnet3/vmxnet3_drv.c
index ce9d4f2..0ae1bcc 100644
--- a/drivers/net/vmxnet3/vmxnet3_drv.c
+++ b/drivers/net/vmxnet3/vmxnet3_drv.c
@@ -744,28 +744,43 @@ vmxnet3_map_pkt(struct sk_buff *skb, struct vmxnet3_tx_ctx *ctx,
for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
const struct skb_frag_struct *frag = &skb_shinfo(skb)->frags[i];
+ u32 buf_size;
- tbi = tq->buf_info + tq->tx_ring.next2fill;
- tbi->map_type = VMXNET3_MAP_PAGE;
- tbi->dma_addr = skb_frag_dma_map(&adapter->pdev->dev, frag,
- 0, skb_frag_size(frag),
- DMA_TO_DEVICE);
+ buf_offset = 0;
+ len = skb_frag_size(frag);
+ while (len) {
+ tbi = tq->buf_info + tq->tx_ring.next2fill;
+ if (len < VMXNET3_MAX_TX_BUF_SIZE) {
+ buf_size = len;
+ dw2 |= len;
+ } else {
+ buf_size = VMXNET3_MAX_TX_BUF_SIZE;
+ /* spec says that for TxDesc.len, 0 == 2^14 */
+ }
+ tbi->map_type = VMXNET3_MAP_PAGE;
+ tbi->dma_addr = skb_frag_dma_map(&adapter->pdev->dev, frag,
+ buf_offset, buf_size,
+ DMA_TO_DEVICE);
- tbi->len = skb_frag_size(frag);
+ tbi->len = buf_size;
- gdesc = tq->tx_ring.base + tq->tx_ring.next2fill;
- BUG_ON(gdesc->txd.gen == tq->tx_ring.gen);
+ gdesc = tq->tx_ring.base + tq->tx_ring.next2fill;
+ BUG_ON(gdesc->txd.gen == tq->tx_ring.gen);
- gdesc->txd.addr = cpu_to_le64(tbi->dma_addr);
- gdesc->dword[2] = cpu_to_le32(dw2 | skb_frag_size(frag));
- gdesc->dword[3] = 0;
+ gdesc->txd.addr = cpu_to_le64(tbi->dma_addr);
+ gdesc->dword[2] = cpu_to_le32(dw2);
+ gdesc->dword[3] = 0;
- dev_dbg(&adapter->netdev->dev,
- "txd[%u]: 0x%llu %u %u\n",
- tq->tx_ring.next2fill, le64_to_cpu(gdesc->txd.addr),
- le32_to_cpu(gdesc->dword[2]), gdesc->dword[3]);
- vmxnet3_cmd_ring_adv_next2fill(&tq->tx_ring);
- dw2 = tq->tx_ring.gen << VMXNET3_TXD_GEN_SHIFT;
+ dev_dbg(&adapter->netdev->dev,
+ "txd[%u]: 0x%llu %u %u\n",
+ tq->tx_ring.next2fill, le64_to_cpu(gdesc->txd.addr),
+ le32_to_cpu(gdesc->dword[2]), gdesc->dword[3]);
+ vmxnet3_cmd_ring_adv_next2fill(&tq->tx_ring);
+ dw2 = tq->tx_ring.gen << VMXNET3_TXD_GEN_SHIFT;
+
+ len -= buf_size;
+ buf_offset += buf_size;
+ }
}
ctx->eop_txd = gdesc;
@@ -886,6 +901,18 @@ vmxnet3_prepare_tso(struct sk_buff *skb,
}
}
+static int txd_estimate(const struct sk_buff *skb)
+{
+ int count = VMXNET3_TXD_NEEDED(skb_headlen(skb)) + 1;
+ int i;
+
+ for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
+ const struct skb_frag_struct *frag = &skb_shinfo(skb)->frags[i];
+
+ count += VMXNET3_TXD_NEEDED(skb_frag_size(frag));
+ }
+ return count;
+}
/*
* Transmits a pkt thru a given tq
@@ -914,9 +941,7 @@ vmxnet3_tq_xmit(struct sk_buff *skb, struct vmxnet3_tx_queue *tq,
union Vmxnet3_GenericDesc tempTxDesc;
#endif
- /* conservatively estimate # of descriptors to use */
- count = VMXNET3_TXD_NEEDED(skb_headlen(skb)) +
- skb_shinfo(skb)->nr_frags + 1;
+ count = txd_estimate(skb);
ctx.ipv4 = (vlan_get_protocol(skb) == cpu_to_be16(ETH_P_IP));
^ permalink raw reply related
* Re: [PATCH] bonding: fix bond-6-mode change MAC of arp reply from vif to cause Domu's network unreachable intermittently
From: David Miller @ 2012-10-29 17:33 UTC (permalink / raw)
To: zheng.x.li; +Cc: netdev, fubar, andy, linux-kernel, joe.jin
In-Reply-To: <508E4840.40104@oracle.com>
From: "zheng.li" <zheng.x.li@oracle.com>
Date: Mon, 29 Oct 2012 17:11:28 +0800
> @@ -700,7 +700,18 @@ static struct slave *rlb_arp_xmit(struct sk_buff
> *skb, struct bonding *bond)
You commit message is poorly formatted and your email client corrupted
the patch, making it completely unusable.
^ permalink raw reply
* Re: [PATCH v7 15/16] openvswitch: use new hashtable implementation
From: Sasha Levin @ 2012-10-29 17:35 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: torvalds, tj, akpm, linux-kernel, linux-mm, paul.gortmaker, davem,
rostedt, mingo, ebiederm, aarcange, ericvh, netdev, josh,
eric.dumazet, axboe, agk, dm-devel, neilb, ccaulfie, teigland,
Trond.Myklebust, bfields, fweisbec, jesse, venkat.x.venkatsubra,
ejt, snitzer, edumazet, linux-nfs, dev, rds-devel, lw
In-Reply-To: <20121029155957.GB18834@Krystal>
On Mon, Oct 29, 2012 at 11:59 AM, Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
> * Sasha Levin (levinsasha928@gmail.com) wrote:
>> Hi Mathieu,
>>
>> On Mon, Oct 29, 2012 at 9:29 AM, Mathieu Desnoyers
>> <mathieu.desnoyers@efficios.com> wrote:
>> > * Sasha Levin (levinsasha928@gmail.com) wrote:
>> > [...]
>> >> -static struct hlist_head *hash_bucket(struct net *net, const char *name)
>> >> -{
>> >> - unsigned int hash = jhash(name, strlen(name), (unsigned long) net);
>> >> - return &dev_table[hash & (VPORT_HASH_BUCKETS - 1)];
>> >> -}
>> >> -
>> >> /**
>> >> * ovs_vport_locate - find a port that has already been created
>> >> *
>> >> @@ -84,13 +76,12 @@ static struct hlist_head *hash_bucket(struct net *net, const char *name)
>> >> */
>> >> struct vport *ovs_vport_locate(struct net *net, const char *name)
>> >> {
>> >> - struct hlist_head *bucket = hash_bucket(net, name);
>> >> struct vport *vport;
>> >> struct hlist_node *node;
>> >> + int key = full_name_hash(name, strlen(name));
>> >>
>> >> - hlist_for_each_entry_rcu(vport, node, bucket, hash_node)
>> >> - if (!strcmp(name, vport->ops->get_name(vport)) &&
>> >> - net_eq(ovs_dp_get_net(vport->dp), net))
>> >> + hash_for_each_possible_rcu(dev_table, vport, node, hash_node, key)
>> >
>> > Is applying hash_32() on top of full_name_hash() needed and expected ?
>>
>> Since this was pointed out in several of the patches, I'll answer it
>> just once here.
>>
>> I've intentionally "allowed" double hashing with hash_32 to keep the
>> code simple.
>>
>> hash_32() is pretty simple and gcc optimizes it to be almost nothing,
>> so doing that costs us a multiplication and a shift. On the other
>> hand, we benefit from keeping our code simple - how would we avoid
>> doing this double hash? adding a different hashtable function for
>> strings? or a new function for already hashed keys? I think we benefit
>> a lot from having to mul/shr instead of adding extra lines of code
>> here.
>
> This could be done, as I pointed out in another email within this
> thread, by changing the "key" argument from add/for_each_possible to an
> expected "hash" value, and let the caller invoke hash_32() if they want.
> I doubt this would add a significant amount of complexity for users of
> this API, but would allow much more flexibility to choose hash
> functions.
Most callers do need to do the hashing though, so why add an
additional step for all callers instead of doing another hash_32 for
the ones that don't really need it?
Another question is why do you need flexibility? I think that
simplicity wins over flexibility here.
Thanks,
Sasha
--
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: Information about napi_struct and net_device
From: Javier Domingo @ 2012-10-29 17:40 UTC (permalink / raw)
To: netdev
In-Reply-To: <1351478461.4450.8.camel@deadeye.wl.decadent.org.uk>
I finally found the problem, and I think also the answer.
I am modifying netif_receive_skb(), putting there my own queue. So the
only problem I face now is how may I disable the main netpoll through.
Looking at dev.c file, and knowing that there must be some way the
solution I was looking for, I discovered enqueue_to_backlog function,
that does what I want to do, from a packet, after processing it, be
able to take the napi_struct that was responsible from it.
I if you could tell me if using the method to obtain the napi_struct
from the net_device pointer used in enqueue_to_backlog is ok, it would
be great,
Any way I will continue doing experiments and reading,
Thanks a lot for your help and attention, and for whoever did [1]
Javier Domingo
[1]: http://www.linuxfoundation.org/images/1/1c/Network_data_flow_through_kernel.png
2012/10/29 Ben Hutchings <bhutchings@solarflare.com>:
> On Sat, 2012-10-27 at 18:18 +0200, Javier Domingo wrote:
>> Hello,
>>
>> I am updating a kernel patch from 2.6.23-rc7 to v3.6, using git
>> facilities, and I have found that there have been lots of changes in
>> networking. Now the drivers are all by vendor, and more important,
>> net_device is not used always, as napi_struct took its place.
>>
>> I have asked in #kernel but no one answered me, so I subscribed here.
>> Is there any place I can read about how is it now designed the use of
>> napi_struct and net_device? I have found a presentation[1] about how
>> networking was changing etc, and I wondered if there is anything more
>> precise about how it is actually implemented.
>>
>> I understand a little why napi_struct, but don't really know much
>> about the change,
>>
>> Hope someone can help me,
>
> The initial change to napi_struct is explained in
> <http://lwn.net/Articles/244640/>.
>
> Since then there have been further changes:
>
> - netif_napi_del() has been added. You must call it to clean up NAPI
> contexts before freeing the associated net device(s).
>
> - Instead of netif_rx_schedule(), netif_rx_complete(), etc. you must use
> napi_schedule(), napi_complete() etc. which just take a napi_struct
> pointer.
>
> Ben.
>
> --
> Ben Hutchings, Staff Engineer, Solarflare
> Not speaking for my employer; that's the marketing department's job.
> They asked us to note that Solarflare product names are trademarked.
>
^ permalink raw reply
* Re: 3.7-rc3 oops on reboot
From: Alexandre Pereira da Silva @ 2012-10-29 17:41 UTC (permalink / raw)
To: Roland Stigge
Cc: David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, netdev, linux-kernel
In-Reply-To: <508E95B3.7070807@antcom.de>
On Mon, Oct 29, 2012 at 12:41 PM, Roland Stigge <stigge@antcom.de> wrote:
> On 10/29/2012 02:51 PM, Alexandre Pereira da Silva wrote:
>>> I just tried both on a PHY3250 reference machine, and on a custom board
>>> and couldn't reproduce sth. like this. Booted the kernel and connected
>>> the ethernet cable later on, as well as with cable connected on boot.
>>
>> This is happening on my custom board. I didn't try this on the
>> reference board yet.
>>
>>> Any related patches applied? Can you reproduce this on a reference
>>> machine? Can you provide a .config for this? Any more detail about how
>>> this could be reproduced?
>>
>> No patches applied. I'm trying to bisect this. This happens on a clean
>> -rc1 as well. Doesn't happen on v3.6.
>>
>> Attached my .config
>>
>> To reproduce this, I am just running "reboot". I have the ethernet
>> cable always connected here.
>
> Just tried this with your .config on the PHY3250 (w/o your initrd
> thought) but couldn't reproduce the issue.
>
> Maybe it's related to your Micrel ethernet PHY (activated in your
> config) which I don't have hardware for?
>
> Otherwise, we would need to have a look at your devicetree.
Thanks for testing. I will investigate this further to see if I can
narrow source of the problem.
^ permalink raw reply
* Re: [PATCH v7 06/16] tracepoint: use new hashtable implementation
From: Mathieu Desnoyers @ 2012-10-29 17:50 UTC (permalink / raw)
To: Sasha Levin
Cc: snitzer-H+wXaHxf7aLQT0dZR+AlfA, neilb-l3A5Bk7waGM,
fweisbec-Re5JQEeQqe8AvxtiuMwx3w,
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA,
bfields-uC3wQj2KruNg9hUCZPvPmw,
paul.gortmaker-CWA4WttNNZF54TAoqtyWWQ,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA, agk-H+wXaHxf7aLQT0dZR+AlfA,
aarcange-H+wXaHxf7aLQT0dZR+AlfA, rds-devel-N0ozoZBvEnrZJqsBc5GL+g,
eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w,
venkat.x.venkatsubra-QHcLZuEGTsvQT0dZR+AlfA,
ccaulfie-H+wXaHxf7aLQT0dZR+AlfA, mingo-X9Un+BFzKDI,
dev-yBygre7rU0TnMu66kgdUjQ, ericvh-Re5JQEeQqe8AvxtiuMwx3w,
josh-iaAMLnmF4UmaiuxdJuQwMA, rostedt-nx8X9YLhiw1AfugRpC6u6w,
lw-BthXqXjhjHXQFUHtdCDX3A, teigland-H+wXaHxf7aLQT0dZR+AlfA,
axboe-tSWWG44O7X1aa/9Udqfwiw, linux-nfs-u79uwXL29TY76Z2rM5mHXA,
edumazet-hpIqsD4AKlfQT0dZR+AlfA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, ejt-H+wXaHxf7aLQT0dZR+AlfA,
ebiederm-aS9lmoZGLiVWk0Htik3J/w, tj-DgEjT+Ai2ygdnm+yROfE0A,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <CA+1xoqce6uJ6wy3+2CBwsLHKnsz4wD0vt8MBEGKCFfXTvuC0Hg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
* Sasha Levin (levinsasha928-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org) wrote:
> On Mon, Oct 29, 2012 at 7:35 AM, Mathieu Desnoyers
> <mathieu.desnoyers-vg+e7yoeK/dWk0Htik3J/w@public.gmane.org> wrote:
> > * Sasha Levin (levinsasha928-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org) wrote:
> >> Switch tracepoints to use the new hashtable implementation. This reduces the amount of
> >> generic unrelated code in the tracepoints.
> >>
> >> Signed-off-by: Sasha Levin <levinsasha928-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> ---
> >> kernel/tracepoint.c | 27 +++++++++++----------------
> >> 1 file changed, 11 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> >> index d96ba22..854df92 100644
> >> --- a/kernel/tracepoint.c
> >> +++ b/kernel/tracepoint.c
> >> @@ -26,6 +26,7 @@
> >> #include <linux/slab.h>
> >> #include <linux/sched.h>
> >> #include <linux/static_key.h>
> >> +#include <linux/hashtable.h>
> >>
> >> extern struct tracepoint * const __start___tracepoints_ptrs[];
> >> extern struct tracepoint * const __stop___tracepoints_ptrs[];
> >> @@ -49,8 +50,7 @@ static LIST_HEAD(tracepoint_module_list);
> >> * Protected by tracepoints_mutex.
> >> */
> >> #define TRACEPOINT_HASH_BITS 6
> >> -#define TRACEPOINT_TABLE_SIZE (1 << TRACEPOINT_HASH_BITS)
> >> -static struct hlist_head tracepoint_table[TRACEPOINT_TABLE_SIZE];
> >> +static DEFINE_HASHTABLE(tracepoint_table, TRACEPOINT_HASH_BITS);
> >>
> > [...]
> >>
> >> @@ -722,6 +715,8 @@ struct notifier_block tracepoint_module_nb = {
> >>
> >> static int init_tracepoints(void)
> >> {
> >> + hash_init(tracepoint_table);
> >> +
> >> return register_module_notifier(&tracepoint_module_nb);
> >> }
> >> __initcall(init_tracepoints);
> >
> > So we have a hash table defined in .bss (therefore entirely initialized
> > to NULL), and you add a call to "hash_init", which iterates on the whole
> > array and initialize it to NULL (again) ?
> >
> > This extra initialization is redundant. I think it should be removed
> > from here, and hashtable.h should document that hash_init() don't need
> > to be called on zeroed memory (which includes static/global variables,
> > kzalloc'd memory, etc).
>
> This was discussed in the previous series, the conclusion was to call
> hash_init() either way to keep the encapsulation and consistency.
Agreed,
Thanks,
Mathieu
>
> It's cheap enough and happens only once, so why not?
>
>
> Thanks,
> Sasha
--
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [PATCH 0/6] Calxeda xgmac performance fixes
From: Rob Herring @ 2012-10-29 17:52 UTC (permalink / raw)
To: David S. Miller
Cc: linux-kernel, netdev, jonathan, eric.dumazet, Mark Langsdorf
In-Reply-To: <1350054908-30646-1-git-send-email-robherring2@gmail.com>
David,
On 10/12/2012 10:15 AM, Rob Herring wrote:
> From: Rob Herring <rob.herring@calxeda.com>
>
> This is a series of performance improvements to the xgmac driver. The most
> significant changes are the alignment fixes to avoid alignment traps on
> received frames and using relaxed i/o accessors.
Can you please apply this series for 3.7. They are all self-contained to
the xgmac driver and fix some performance issues including unaligned
access traps in the IP stack. If 3.7 is not acceptable, then 3.8 is fine.
Note that only patch 5 has a v2 version.
Rob
>
> Rob Herring (6):
> net: calxedaxgmac: enable operate on 2nd frame mode
> net: calxedaxgmac: remove explicit rx dma buffer polling
> net: calxedaxgmac: use relaxed i/o accessors in rx and tx paths
> net: calxedaxgmac: drop some unnecessary register writes
> net: calxedaxgmac: rework transmit ring handling
> net: calxedaxgmac: ip align receive buffers
>
> drivers/net/ethernet/calxeda/Kconfig | 2 +-
> drivers/net/ethernet/calxeda/xgmac.c | 57 +++++++++++++++-------------------
> 2 files changed, 26 insertions(+), 33 deletions(-)
>
^ permalink raw reply
* Re: [Pv-drivers] [PATCH] vmxnet3: must split too big fragments
From: Bhavesh Davda @ 2012-10-29 17:52 UTC (permalink / raw)
To: Eric Dumazet
Cc: VMware, Inc., netdev, linux-kernel, jongman heo,
Shreyas Bhatewara, David Miller
In-Reply-To: <1351531849.12280.54.camel@edumazet-glaptop>
LGTM. Thanks for doing this! Did you do any performance testing with this patch?
Reviewed-by: Bhavesh Davda <bhavesh@vmware.com>
--
Bhavesh Davda
----- Original Message -----
> From: "Eric Dumazet" <eric.dumazet@gmail.com>
> To: "Shreyas Bhatewara" <sbhatewara@vmware.com>, "David Miller" <davem@davemloft.net>
> Cc: "VMware, Inc." <pv-drivers@vmware.com>, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, "jongman heo"
> <jongman.heo@samsung.com>
> Sent: Monday, October 29, 2012 10:30:49 AM
> Subject: [Pv-drivers] [PATCH] vmxnet3: must split too big fragments
>
> From: Eric Dumazet <edumazet@google.com>
>
> vmxnet3 has a 16Kbytes limit per tx descriptor, that happened to work
> as long as we provided PAGE_SIZE fragments.
>
> Our stack can now build larger fragments, so we need to split them to
> the 16kbytes boundary.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: jongman heo <jongman.heo@samsung.com>
> Tested-by: jongman heo <jongman.heo@samsung.com>
> Cc: Shreyas Bhatewara <sbhatewara@vmware.com>
> ---
> drivers/net/vmxnet3/vmxnet3_drv.c | 65
> +++++++++++++++++++---------
> 1 file changed, 45 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/net/vmxnet3/vmxnet3_drv.c
> b/drivers/net/vmxnet3/vmxnet3_drv.c
> index ce9d4f2..0ae1bcc 100644
> --- a/drivers/net/vmxnet3/vmxnet3_drv.c
> +++ b/drivers/net/vmxnet3/vmxnet3_drv.c
> @@ -744,28 +744,43 @@ vmxnet3_map_pkt(struct sk_buff *skb, struct
> vmxnet3_tx_ctx *ctx,
>
> for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
> const struct skb_frag_struct *frag = &skb_shinfo(skb)->frags[i];
> + u32 buf_size;
>
> - tbi = tq->buf_info + tq->tx_ring.next2fill;
> - tbi->map_type = VMXNET3_MAP_PAGE;
> - tbi->dma_addr = skb_frag_dma_map(&adapter->pdev->dev, frag,
> - 0, skb_frag_size(frag),
> - DMA_TO_DEVICE);
> + buf_offset = 0;
> + len = skb_frag_size(frag);
> + while (len) {
> + tbi = tq->buf_info + tq->tx_ring.next2fill;
> + if (len < VMXNET3_MAX_TX_BUF_SIZE) {
> + buf_size = len;
> + dw2 |= len;
> + } else {
> + buf_size = VMXNET3_MAX_TX_BUF_SIZE;
> + /* spec says that for TxDesc.len, 0 == 2^14 */
> + }
> + tbi->map_type = VMXNET3_MAP_PAGE;
> + tbi->dma_addr = skb_frag_dma_map(&adapter->pdev->dev, frag,
> + buf_offset, buf_size,
> + DMA_TO_DEVICE);
>
> - tbi->len = skb_frag_size(frag);
> + tbi->len = buf_size;
>
> - gdesc = tq->tx_ring.base + tq->tx_ring.next2fill;
> - BUG_ON(gdesc->txd.gen == tq->tx_ring.gen);
> + gdesc = tq->tx_ring.base + tq->tx_ring.next2fill;
> + BUG_ON(gdesc->txd.gen == tq->tx_ring.gen);
>
> - gdesc->txd.addr = cpu_to_le64(tbi->dma_addr);
> - gdesc->dword[2] = cpu_to_le32(dw2 | skb_frag_size(frag));
> - gdesc->dword[3] = 0;
> + gdesc->txd.addr = cpu_to_le64(tbi->dma_addr);
> + gdesc->dword[2] = cpu_to_le32(dw2);
> + gdesc->dword[3] = 0;
>
> - dev_dbg(&adapter->netdev->dev,
> - "txd[%u]: 0x%llu %u %u\n",
> - tq->tx_ring.next2fill, le64_to_cpu(gdesc->txd.addr),
> - le32_to_cpu(gdesc->dword[2]), gdesc->dword[3]);
> - vmxnet3_cmd_ring_adv_next2fill(&tq->tx_ring);
> - dw2 = tq->tx_ring.gen << VMXNET3_TXD_GEN_SHIFT;
> + dev_dbg(&adapter->netdev->dev,
> + "txd[%u]: 0x%llu %u %u\n",
> + tq->tx_ring.next2fill, le64_to_cpu(gdesc->txd.addr),
> + le32_to_cpu(gdesc->dword[2]), gdesc->dword[3]);
> + vmxnet3_cmd_ring_adv_next2fill(&tq->tx_ring);
> + dw2 = tq->tx_ring.gen << VMXNET3_TXD_GEN_SHIFT;
> +
> + len -= buf_size;
> + buf_offset += buf_size;
> + }
> }
>
> ctx->eop_txd = gdesc;
> @@ -886,6 +901,18 @@ vmxnet3_prepare_tso(struct sk_buff *skb,
> }
> }
>
> +static int txd_estimate(const struct sk_buff *skb)
> +{
> + int count = VMXNET3_TXD_NEEDED(skb_headlen(skb)) + 1;
> + int i;
> +
> + for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
> + const struct skb_frag_struct *frag = &skb_shinfo(skb)->frags[i];
> +
> + count += VMXNET3_TXD_NEEDED(skb_frag_size(frag));
> + }
> + return count;
> +}
>
> /*
> * Transmits a pkt thru a given tq
> @@ -914,9 +941,7 @@ vmxnet3_tq_xmit(struct sk_buff *skb, struct
> vmxnet3_tx_queue *tq,
> union Vmxnet3_GenericDesc tempTxDesc;
> #endif
>
> - /* conservatively estimate # of descriptors to use */
> - count = VMXNET3_TXD_NEEDED(skb_headlen(skb)) +
> - skb_shinfo(skb)->nr_frags + 1;
> + count = txd_estimate(skb);
>
> ctx.ipv4 = (vlan_get_protocol(skb) == cpu_to_be16(ETH_P_IP));
>
>
>
> _______________________________________________
> Pv-drivers mailing list
> Pv-drivers@vmware.com
> http://mailman2.vmware.com/mailman/listinfo/pv-drivers
>
^ permalink raw reply
* [PATCH, RESEND] r8169: Kill SafeMtu macro
From: Kirill Smelkov @ 2012-10-29 17:55 UTC (permalink / raw)
To: Francois Romieu; +Cc: netdev, Kirill Smelkov
After d58d46b5 (r8169: jumbo fixes.) max frame len is stored in
rtl_chip_infos[].jumbo_max for each chip and SafeMtu should be gone.
Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
---
drivers/net/ethernet/realtek/r8169.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index e7ff886..7ca6618 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -78,7 +78,6 @@ static const int multicast_filter_limit = 32;
#define MAX_READ_REQUEST_SHIFT 12
#define TX_DMA_BURST 7 /* Maximum PCI burst, '7' is unlimited */
-#define SafeMtu 0x1c20 /* ... actually life sucks beyond ~7k */
#define InterFrameGap 0x03 /* 3 means InterFrameGap = the shortest one */
#define R8169_REGS_SIZE 256
--
1.8.0.rc1.231.g17358b9
^ permalink raw reply related
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