* Re: [PATCH] xfrm: cache bundle lookup results in flow cache
From: Herbert Xu @ 2010-03-23 9:41 UTC (permalink / raw)
To: Timo Teräs; +Cc: netdev, David S. Miller
In-Reply-To: <4BA88789.7060104@iki.fi>
On Tue, Mar 23, 2010 at 11:19:05AM +0200, Timo Teräs wrote:
>
> Normally, only the getter is called per-packet. So I'm thinking
> to have get() that would check for entry being stale or not,
> and bump up the refcount.
OK if it's just one call per packet it sounds good to me.
Thanks,
--
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: [PATCH] rps: make distributing packets fairly among all the online CPUs default
From: Changli Gao @ 2010-03-23 10:03 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S. Miller, Tom Herbert, netdev, Ben Hutchings
In-Reply-To: <1269329250.3043.19.camel@edumazet-laptop>
On Tue, Mar 23, 2010 at 3:27 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le mardi 23 mars 2010 à 14:24 +0800, Changli Gao a écrit :
>> make distributing packets fairly among all the online CPUs default.
>>
>> Make distributing packets fairly among all the online CPUs default, then
>> users don't need any explicit configuration to get the benefit of RPS.
>>
>
> Some people want to dedicate a cpu to network interrupts, cache hits
> only, to get low latencies.
> RPS is not a win for all workloads.
>
> I think we should not change default without a knob.
>
> Didnt we discussed all this during last months ?
>
Ben Hutchings raised this question, but nobody replied with any message.
I do think distributing packets fairly among all the online CPUs will
helps most of users. I remember the smp_affinity of IRQ is the
online_cpu_mask be default.
--
Regards，
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [PATCH] rps: make distributing packets fairly among all the online CPUs default
From: Changli Gao @ 2010-03-23 9:56 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S. Miller, Tom Herbert, netdev
In-Reply-To: <1269328852.3043.13.camel@edumazet-laptop>
On Tue, Mar 23, 2010 at 3:20 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le mardi 23 mars 2010 à 14:24 +0800, Changli Gao a écrit :
>
> You cannot do this like this, these allocations wont be freed.
>
> Better would be to take a look at net/core/net-sysfs.c
>
Sorry, but I don't know why these allocations won't be freed.
--
Regards，
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [PATCH] xfrm: cache bundle lookup results in flow cache
From: Timo Teräs @ 2010-03-23 9:19 UTC (permalink / raw)
To: Herbert Xu; +Cc: netdev, David S. Miller
In-Reply-To: <20100323074252.GA27623@gondor.apana.org.au>
Herbert Xu wrote:
> On Tue, Mar 23, 2010 at 09:28:33AM +0200, Timo Teräs wrote:
>> So, should I go ahead and do the virtualization of the
>> getters and putters?
>
> If we're going to have indirect calls per packet then let's at
> least minimise them. I think one should be enough, i.e., just
> add a ->stale() function pointer to be used in flow_cache_lookup.
Normally, only the getter is called per-packet. So I'm thinking
to have get() that would check for entry being stale or not,
and bump up the refcount.
The resolver can just swap the old entry and call appropriate
_put/_get, so we can avoid virtual calls there.
Thinking more about the flushing of the flow cache. It's basically
only needed to flush before the policies are garbage collected.
This is strictly needed so we can do the atomic_dec() without
turning policy's refcount to zero and causing a leak.
I'm now thinking if it'd be worth doing virtual _put(). This
way we would not need flushing at all for in policy garbage
collector (we could kill flow_cache_flush). We could also
call dst_release immediately in the flow cache _put, which
would release the memory faster. This way we would not need
at all any periodic xfrm_dst checker as flow cache is
regenerated regularly.
The code paths that would require calling the virtual put
are:
- randomization of flow cache (every 10 mins currently)
from flow_cache_lookup() with bh disabled
- flow cache going too full, from flow_cache_lookup()
with bh disabled
- cpu notifier
Do you think the virtual _put doing more work would be
too slow?
In that case the plain atomic_dec sounds ok, but we'd then
need to periodically check through the global bundle list
for garbage collection.
Cheers,
Timo
^ permalink raw reply
* RE,
From: FROM CENTRAL BANK @ 2010-03-23 7:53 UTC (permalink / raw)
Very Urgently,
We Conclude Our Meeting Today That $10.7m should be pay to you
as your contract entitlement. The Payment Will Come To You Via Diplomatic
Carrier Service:Re- Comfirm this informations as follows.
Your Full Name,
Home
Address,
Direct Phone No,
Occupation And Age.
Dr. Sanusi A. Lamido
Tel:+234
8067884885
^ permalink raw reply
* Re: [PATCH] xfrm: cache bundle lookup results in flow cache
From: Herbert Xu @ 2010-03-23 7:42 UTC (permalink / raw)
To: Timo Teräs; +Cc: netdev, David S. Miller
In-Reply-To: <4BA86DA1.7090707@iki.fi>
On Tue, Mar 23, 2010 at 09:28:33AM +0200, Timo Teräs wrote:
>
> So, should I go ahead and do the virtualization of the
> getters and putters?
If we're going to have indirect calls per packet then let's at
least minimise them. I think one should be enough, i.e., just
add a ->stale() function pointer to be used in flow_cache_lookup.
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: [PATCH] xfrm: cache bundle lookup results in flow cache
From: Timo Teräs @ 2010-03-23 7:28 UTC (permalink / raw)
To: Herbert Xu; +Cc: netdev, David S. Miller
In-Reply-To: <4BA7B10A.1030302@iki.fi>
Timo Teräs wrote:
> Herbert Xu wrote:
>> On Sun, Mar 21, 2010 at 10:31:23AM +0200, Timo Teräs wrote:
>>>> Ok, we can do that to skip 2. But I think 1 would be still useful.
>>>> It'd probably be good to actually have flow_cache_ops pointer in
>>>> each entry instead of the atomic_t pointer.
>>>>
>>>> The reasoning:
>>>> - we can then have type-based checks that the reference count
>>>> is valid (e.g. policy's refcount must not go to zero, it's bug,
>>>> and we can call dst_release which warns if refcount goes to
>>>> negative); imho it's hack to call atomic_dec instead of the
>>>> real type's xxx_put
>>>> - the flow cache needs to somehow know if the entry is stale so
>>>> it'll try to refresh it atomically; e.g. if there's no
>>>> check for 'stale', the lookup returns stale xfrm_dst. we'd
>>>> then need new api to update the stale entry, or flush it out
>>>> and repeat the lookup. the virtual get could check for it being
>>>> stale (if so release the entry) and then return null for the
>>>> generic code to call the resolver atomically
>>>> - for paranoia we can actually check the type of the object in
>>>> cache via the ops (if needed)
>>
>> The reason I'd prefer to keep the current scheme is to avoid
>> an additional indirect function call on each packet.
>>
>> The way it would work is (we need flow_cache_lookup to return
>> fle instead of the object):
>>
>> fle = flow_cache_lookup
>> xdst = fle->object
>> if (xdst is stale) {
>> flow_cache_mark_obsolete(fle)
>> fle = flow_cache_lookup
>> xdst = fle->object
>> if (xdst is stale)
>> return error
>> }
I've been thinking more about doing this. And I think this
approach is fundamentally racy.
Underlying fle->object can be changed underneath as since
bh are not disabled. This means the refcounting needs to
be done on fle level, and additional measures are needed
to ensure that fle->object is as expected.
Additionally, the second flow_cache_lookup can happen
on another cpu, and could return a stale object that
indeed would need refreshing instead of generating an
error.
Options:
1. return fle and fle->object separately, refcount both
2. call flow_cache_lookup with bh disabled
3. use flow_cache_entry_ops and virtualize put/get
Since 2. was previously said to leak generic code,
it does not sound good.
On 1 vs 3, I'd still choose 3 since:
- adding one more refcount means more atomic calls
which have same (or more) overhead as indirect call
- two refcounts sounds more complicated than one
- exposing fle and touching it without bh's disabled
requires a whole lot of more extra care; doing
3 is simpler
So, should I go ahead and do the virtualization of the
getters and putters?
- Timo
^ permalink raw reply
* Re: [PATCH] rps: make distributing packets fairly among all the online CPUs default
From: Eric Dumazet @ 2010-03-23 7:27 UTC (permalink / raw)
To: xiaosuo; +Cc: David S. Miller, Tom Herbert, netdev
In-Reply-To: <4BA85EA2.1090304@gmail.com>
Le mardi 23 mars 2010 à 14:24 +0800, Changli Gao a écrit :
> make distributing packets fairly among all the online CPUs default.
>
> Make distributing packets fairly among all the online CPUs default, then
> users don't need any explicit configuration to get the benefit of RPS.
>
Some people want to dedicate a cpu to network interrupts, cache hits
only, to get low latencies.
RPS is not a win for all workloads.
I think we should not change default without a knob.
Didnt we discussed all this during last months ?
^ permalink raw reply
* Re: [PATCH] rps: make distributing packets fairly among all the online CPUs default
From: Eric Dumazet @ 2010-03-23 7:20 UTC (permalink / raw)
To: xiaosuo; +Cc: David S. Miller, Tom Herbert, netdev
In-Reply-To: <4BA85EA2.1090304@gmail.com>
Le mardi 23 mars 2010 à 14:24 +0800, Changli Gao a écrit :
> make distributing packets fairly among all the online CPUs default.
>
> Make distributing packets fairly among all the online CPUs default, then
> users don't need any explicit configuration to get the benefit of RPS.
>
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
> ----
> net/core/dev.c | 34 ++++++++++++++++++++++++++++++++--
> 1 file changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index c0e2608..a4246f1 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5234,6 +5234,24 @@ void netif_stacked_transfer_operstate(const struct net_device *rootdev,
> }
> EXPORT_SYMBOL(netif_stacked_transfer_operstate);
>
> +static struct rps_map* alloc_rps_map(void)
> +{
> + struct rps_map *map;
> + int i, cpu;
> +
> + map = kzalloc(max_t(unsigned,
> + RPS_MAP_SIZE(cpumask_weight(cpu_online_mask)),
> + L1_CACHE_BYTES), GFP_KERNEL);
> + if (map == NULL)
> + return NULL;
> + i = 0;
> + for_each_online_cpu(cpu)
> + map->cpus[i++] = cpu;
> + map->len = i;
> +
> + return map;
> +}
> +
> /**
> * register_netdevice - register a network device
> * @dev: device to register
> @@ -5282,7 +5300,13 @@ int register_netdevice(struct net_device *dev)
> ret = -ENOMEM;
> goto out;
> }
> -
> + dev->_rx->rps_map = alloc_rps_map();
> + if (dev->_rx->rps_map == NULL) {
> + kfree(dev->_rx);
> + dev->_rx = NULL;
> + ret = -ENOMEM;
> + goto out;
> + }
> dev->_rx->first = dev->_rx;
> atomic_set(&dev->_rx->count, 1);
> dev->num_rx_queues = 1;
> @@ -5688,8 +5712,12 @@ struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
> * Set a pointer to first element in the array which holds the
> * reference count.
> */
> - for (i = 0; i < queue_count; i++)
> + for (i = 0; i < queue_count; i++) {
> rx[i].first = rx;
> + rx[i].rps_map = alloc_rps_map();
> + if (rx[i].rps_map == NULL)
> + goto free_rx;
> + }
>
> dev = PTR_ALIGN(p, NETDEV_ALIGN);
> dev->padded = (char *)dev - (char *)p;
> @@ -5723,6 +5751,8 @@ struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
> return dev;
>
> free_rx:
> + for (i = 0; i < queue_count; i++)
> + kfree(rx[i].rps_map);
> kfree(rx);
> free_tx:
> kfree(tx);
>
>
> --
You cannot do this like this, these allocations wont be freed.
Better would be to take a look at net/core/net-sysfs.c
^ permalink raw reply
* Re: [PATCH] rps: memory leak due to forget to release rx queues
From: Changli Gao @ 2010-03-23 7:02 UTC (permalink / raw)
To: Tom Herbert; +Cc: David S. Miller, netdev
In-Reply-To: <65634d661003222344s662000e8ode1b4f49ff73b345@mail.gmail.com>
On Tue, Mar 23, 2010 at 2:44 PM, Tom Herbert <therbert@google.com> wrote:
> 2010/3/22 Changli Gao <xiaosuo@gmail.com>:
>> memory leak due to forget to release rx queues.
>>
>> rx queues is allocated in alloc_netdev_mq(), but they aren't released in
>> free_netdev(), and the field rps_map is also miss.
>>
Thanks for explaining.
--
Regards，
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: tc download always to "default" [solved]
From: Mihamina Rakotomandimby @ 2010-03-23 7:02 UTC (permalink / raw)
To: netdev
In-Reply-To: <20100322143113.GA23778@wolff.to>
> Bruno Wolff III <bruno@wolff.to> :
>> My problem is all the traffic is caught by "classid 1:10", the
>> default class.
>> Nothing is trapped by "classid 1:301" -> "classid 1:426"
>> At a first glance, would you see something wrong in these?
>I took a quick look and didn't see anything obvious. But I haven't
>played with this stuff for a while and my scripts were set up a bit
>differently.
>How are you checking that everything is ending up in the default class?
1°) As I wrote my rules, each IP address has one handler.
I use this script to display UP and DOWN traffic for the handler in $1:
#!/bin/sh
INIF=eth1
OUTIF=ifb0
RATEIN=$(tc -s -d class show dev $INIF \
| egrep -A 5 "class htb 1:$1 " \
| egrep "rate .+ backlog" \
| awk '{print $2}')
RATEOUT=$(tc -s -d class show dev $OUTIF \
| egrep -A 5 "class htb 1:$1 " \
| egrep "rate .+ backlog" \
| awk '{print $2}')
echo $RATEIN
echo $RATEOUT
I launch it like this:
# get-bw.sh $NNN
Where $NNN is the handler.
When I launch it, I see "0bit" for IN traffic, that means nothing is
trapped.
When I launch it with the handler of the default, I see much traffic.
2°) When varying the "default" allocated bandwidth, I see in my charts
it is the one seen.
>I remember there is a command that tells you how much stuff is ending
> up getting handled by the different qdiscs. Is that what you are doing
> or are you just assuming that's what's happening because traffic isn't
> being limited the way you expect?
Anyway, I solved my problem:
The LAN interface is "eth1" but there is a tunnel "tun0" coupled to it
(I use coova chilli access point).
Just to test, I switched to "tun0" and it suddenly worked.
In all the rules I gave, just replacing "eth1" with "tun0" made it!
(What a dumb I am...)
Thanks for everything.
--
Architecte Informatique chez Blueline/Gulfsat:
Administration Systeme, Recherche & Developpement
+261 34 29 155 34 / +261 33 11 207 36
^ permalink raw reply
* Re: [PATCH] rps: memory leak due to forget to release rx queues
From: Tom Herbert @ 2010-03-23 6:44 UTC (permalink / raw)
To: xiaosuo; +Cc: David S. Miller, netdev
In-Reply-To: <4BA85A33.6050004@gmail.com>
2010/3/22 Changli Gao <xiaosuo@gmail.com>:
> memory leak due to forget to release rx queues.
>
> rx queues is allocated in alloc_netdev_mq(), but they aren't released in
> free_netdev(), and the field rps_map is also miss.
>
rx_queues can't be free from free_netdev since they contain kobjects
(only freed when refcnt goes to zero). They should be freed when the
device kobject is freed (netdev_unregister_kobject).
Tom
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
> ----
> net/core/dev.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index c0e2608..51b86e0 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5746,6 +5746,14 @@ void free_netdev(struct net_device *dev)
>
> release_net(dev_net(dev));
>
> + if (dev->num_rx_queues) {
> + int i;
> +
> + for (i = 0; i < dev->num_rx_queues; i++)
> + kfree(dev->_rx[i].rps_map);
> + kfree(dev->_rx);
> + }
> +
> kfree(dev->_tx);
>
> /* Flush device addresses */
>
>
>
^ permalink raw reply
* [PATCH net-next-2.6 2/2] be2net: fix unmap_single/page() called incorrectly in Tx compl processing
From: Sathya Perla @ 2010-03-23 6:41 UTC (permalink / raw)
To: netdev
The first wrb seen by tx compl processing does not have a dma handle in it.
Currently, pci_unmap_single() is attempted on this wrb and pci_unmap_page() on the
rest. So, pci_unmap_page() gets incorrectly called on the dma hdl of skb->data (that
was mapped using map_single()). This patch fixes this issue.
Signed-off-by: Sathya Perla <sathyap@serverengines.com>
---
drivers/net/benet/be_main.c | 33 ++++++++++++---------------------
1 files changed, 12 insertions(+), 21 deletions(-)
diff --git a/drivers/net/benet/be_main.c b/drivers/net/benet/be_main.c
index 0800c63..174e5f8 100644
--- a/drivers/net/benet/be_main.c
+++ b/drivers/net/benet/be_main.c
@@ -1047,35 +1047,26 @@ static void be_tx_compl_process(struct be_adapter *adapter, u16 last_index)
struct be_eth_wrb *wrb;
struct sk_buff **sent_skbs = adapter->tx_obj.sent_skb_list;
struct sk_buff *sent_skb;
- u64 busaddr;
- u16 cur_index, num_wrbs = 0;
+ u16 cur_index, num_wrbs = 1; /* account for hdr wrb */
+ bool unmap_skb_hdr = true;
- cur_index = txq->tail;
- sent_skb = sent_skbs[cur_index];
+ sent_skb = sent_skbs[txq->tail];
BUG_ON(!sent_skb);
- sent_skbs[cur_index] = NULL;
- wrb = queue_tail_node(txq);
- be_dws_le_to_cpu(wrb, sizeof(*wrb));
- busaddr = ((u64)wrb->frag_pa_hi << 32) | (u64)wrb->frag_pa_lo;
- if (busaddr != 0) {
- pci_unmap_single(adapter->pdev, busaddr,
- wrb->frag_len, PCI_DMA_TODEVICE);
- }
- num_wrbs++;
+ sent_skbs[txq->tail] = NULL;
+
+ /* skip header wrb */
queue_tail_inc(txq);
- while (cur_index != last_index) {
+ do {
cur_index = txq->tail;
wrb = queue_tail_node(txq);
- be_dws_le_to_cpu(wrb, sizeof(*wrb));
- busaddr = ((u64)wrb->frag_pa_hi << 32) | (u64)wrb->frag_pa_lo;
- if (busaddr != 0) {
- pci_unmap_page(adapter->pdev, busaddr,
- wrb->frag_len, PCI_DMA_TODEVICE);
- }
+ unmap_tx_frag(adapter->pdev, wrb, (unmap_skb_hdr &&
+ sent_skb->len > sent_skb->data_len));
+ unmap_skb_hdr = false;
+
num_wrbs++;
queue_tail_inc(txq);
- }
+ } while (cur_index != last_index);
atomic_sub(num_wrbs, &txq->used);
--
1.6.3.3
^ permalink raw reply related
* [PATCH net-next-2.6 1/2] be2net: handle dma mapping errors in Tx path
From: Sathya Perla @ 2010-03-23 6:41 UTC (permalink / raw)
To: netdev
Signed-off-by: Sathya Perla <sathyap@serverengines.com>
---
drivers/net/benet/be_main.c | 41 ++++++++++++++++++++++++++++++++++++++---
1 files changed, 38 insertions(+), 3 deletions(-)
diff --git a/drivers/net/benet/be_main.c b/drivers/net/benet/be_main.c
index 43e8032..0800c63 100644
--- a/drivers/net/benet/be_main.c
+++ b/drivers/net/benet/be_main.c
@@ -386,26 +386,48 @@ static void wrb_fill_hdr(struct be_eth_hdr_wrb *hdr, struct sk_buff *skb,
AMAP_SET_BITS(struct amap_eth_hdr_wrb, len, hdr, len);
}
+static void unmap_tx_frag(struct pci_dev *pdev, struct be_eth_wrb *wrb,
+ bool unmap_single)
+{
+ dma_addr_t dma;
+
+ be_dws_le_to_cpu(wrb, sizeof(*wrb));
+
+ dma = (u64)wrb->frag_pa_hi << 32 | (u64)wrb->frag_pa_lo;
+ if (dma != 0) {
+ if (unmap_single)
+ pci_unmap_single(pdev, dma, wrb->frag_len,
+ PCI_DMA_TODEVICE);
+ else
+ pci_unmap_page(pdev, dma, wrb->frag_len,
+ PCI_DMA_TODEVICE);
+ }
+}
static int make_tx_wrbs(struct be_adapter *adapter,
struct sk_buff *skb, u32 wrb_cnt, bool dummy_wrb)
{
- u64 busaddr;
- u32 i, copied = 0;
+ dma_addr_t busaddr;
+ int i, copied = 0;
struct pci_dev *pdev = adapter->pdev;
struct sk_buff *first_skb = skb;
struct be_queue_info *txq = &adapter->tx_obj.q;
struct be_eth_wrb *wrb;
struct be_eth_hdr_wrb *hdr;
+ bool map_single = false;
+ u16 map_head;
hdr = queue_head_node(txq);
- atomic_add(wrb_cnt, &txq->used);
queue_head_inc(txq);
+ map_head = txq->head;
if (skb->len > skb->data_len) {
int len = skb->len - skb->data_len;
busaddr = pci_map_single(pdev, skb->data, len,
PCI_DMA_TODEVICE);
+ if (pci_dma_mapping_error(pdev, busaddr))
+ goto dma_err;
+ map_single = true;
wrb = queue_head_node(txq);
wrb_fill(wrb, busaddr, len);
be_dws_cpu_to_le(wrb, sizeof(*wrb));
@@ -419,6 +441,8 @@ static int make_tx_wrbs(struct be_adapter *adapter,
busaddr = pci_map_page(pdev, frag->page,
frag->page_offset,
frag->size, PCI_DMA_TODEVICE);
+ if (pci_dma_mapping_error(pdev, busaddr))
+ goto dma_err;
wrb = queue_head_node(txq);
wrb_fill(wrb, busaddr, frag->size);
be_dws_cpu_to_le(wrb, sizeof(*wrb));
@@ -438,6 +462,16 @@ static int make_tx_wrbs(struct be_adapter *adapter,
be_dws_cpu_to_le(hdr, sizeof(*hdr));
return copied;
+dma_err:
+ txq->head = map_head;
+ while (copied) {
+ wrb = queue_head_node(txq);
+ unmap_tx_frag(pdev, wrb, map_single);
+ map_single = false;
+ copied -= wrb->frag_len;
+ queue_head_inc(txq);
+ }
+ return 0;
}
static netdev_tx_t be_xmit(struct sk_buff *skb,
@@ -462,6 +496,7 @@ static netdev_tx_t be_xmit(struct sk_buff *skb,
* *BEFORE* ringing the tx doorbell, so that we serialze the
* tx compls of the current transmit which'll wake up the queue
*/
+ atomic_add(wrb_cnt, &txq->used);
if ((BE_MAX_TX_FRAG_COUNT + atomic_read(&txq->used)) >=
txq->len) {
netif_stop_queue(netdev);
--
1.6.3.3
^ permalink raw reply related
* Re: [PATCH] Fix locking in flush_backlog
From: Eric Dumazet @ 2010-03-23 6:29 UTC (permalink / raw)
To: Tom Herbert; +Cc: davem, netdev
In-Reply-To: <alpine.DEB.1.00.1003222257470.3718@pokey.mtv.corp.google.com>
Le lundi 22 mars 2010 à 23:04 -0700, Tom Herbert a écrit :
> Need to take spinlocks when dequeuing from input_pkt_queue in
> flush_backlog. Also, with the spinlock the backlog queues can
> be flushed directly from netdev_run_todo.
>
> Signed-off-by: Tom Herbert <therbert@google.com>
> ---
> diff --git a/net/core/dev.c b/net/core/dev.c
> index a03aab4..e7db656 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2765,20 +2765,6 @@ int netif_receive_skb(struct sk_buff *skb)
> }
> EXPORT_SYMBOL(netif_receive_skb);
>
> -/* Network device is going away, flush any packets still pending */
> -static void flush_backlog(void *arg)
> -{
> - struct net_device *dev = arg;
> - struct softnet_data *queue = &__get_cpu_var(softnet_data);
> - struct sk_buff *skb, *tmp;
> -
> - skb_queue_walk_safe(&queue->input_pkt_queue, skb, tmp)
> - if (skb->dev == dev) {
> - __skb_unlink(skb, &queue->input_pkt_queue);
> - kfree_skb(skb);
> - }
> -}
> -
> static int napi_gro_complete(struct sk_buff *skb)
> {
> struct packet_type *ptype;
> @@ -5545,6 +5531,7 @@ void netdev_run_todo(void)
> while (!list_empty(&list)) {
> struct net_device *dev
> = list_first_entry(&list, struct net_device, todo_list);
> + int i;
> list_del(&dev->todo_list);
>
> if (unlikely(dev->reg_state != NETREG_UNREGISTERING)) {
> @@ -5556,7 +5543,22 @@ void netdev_run_todo(void)
>
> dev->reg_state = NETREG_UNREGISTERED;
>
> - on_each_cpu(flush_backlog, dev, 1);
> + /* Flush backlog queues of any pending packets */
> + for_each_online_cpu(i) {
> + struct softnet_data *queue = &per_cpu(softnet_data, i);
> + struct sk_buff *skb, *tmp;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&queue->input_pkt_queue.lock, flags);
> + skb_queue_walk_safe(&queue->input_pkt_queue, skb, tmp)
> + if (skb->dev == dev) {
> + __skb_unlink(skb,
> + &queue->input_pkt_queue);
> + kfree_skb(skb);
> + }
> + spin_unlock_irqrestore(&queue->input_pkt_queue.lock,
> + flags);
> + }
>
> netdev_wait_allrefs(dev);
>
OK (this is a patch for net-next-2.6)
Could you please keep the function, to ease netdev_run_todo() review ?
Thanks
^ permalink raw reply
* [PATCH] rps: make distributing packets fairly among all the online CPUs default
From: Changli Gao @ 2010-03-23 6:24 UTC (permalink / raw)
To: David S. Miller; +Cc: Tom Herbert, xiaosuo, netdev
make distributing packets fairly among all the online CPUs default.
Make distributing packets fairly among all the online CPUs default, then
users don't need any explicit configuration to get the benefit of RPS.
Signed-off-by: Changli Gao <xiaosuo@gmail.com>
----
net/core/dev.c | 34 ++++++++++++++++++++++++++++++++--
1 file changed, 32 insertions(+), 2 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index c0e2608..a4246f1 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5234,6 +5234,24 @@ void netif_stacked_transfer_operstate(const struct net_device *rootdev,
}
EXPORT_SYMBOL(netif_stacked_transfer_operstate);
+static struct rps_map* alloc_rps_map(void)
+{
+ struct rps_map *map;
+ int i, cpu;
+
+ map = kzalloc(max_t(unsigned,
+ RPS_MAP_SIZE(cpumask_weight(cpu_online_mask)),
+ L1_CACHE_BYTES), GFP_KERNEL);
+ if (map == NULL)
+ return NULL;
+ i = 0;
+ for_each_online_cpu(cpu)
+ map->cpus[i++] = cpu;
+ map->len = i;
+
+ return map;
+}
+
/**
* register_netdevice - register a network device
* @dev: device to register
@@ -5282,7 +5300,13 @@ int register_netdevice(struct net_device *dev)
ret = -ENOMEM;
goto out;
}
-
+ dev->_rx->rps_map = alloc_rps_map();
+ if (dev->_rx->rps_map == NULL) {
+ kfree(dev->_rx);
+ dev->_rx = NULL;
+ ret = -ENOMEM;
+ goto out;
+ }
dev->_rx->first = dev->_rx;
atomic_set(&dev->_rx->count, 1);
dev->num_rx_queues = 1;
@@ -5688,8 +5712,12 @@ struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
* Set a pointer to first element in the array which holds the
* reference count.
*/
- for (i = 0; i < queue_count; i++)
+ for (i = 0; i < queue_count; i++) {
rx[i].first = rx;
+ rx[i].rps_map = alloc_rps_map();
+ if (rx[i].rps_map == NULL)
+ goto free_rx;
+ }
dev = PTR_ALIGN(p, NETDEV_ALIGN);
dev->padded = (char *)dev - (char *)p;
@@ -5723,6 +5751,8 @@ struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
return dev;
free_rx:
+ for (i = 0; i < queue_count; i++)
+ kfree(rx[i].rps_map);
kfree(rx);
free_tx:
kfree(tx);
^ permalink raw reply related
* [PATCH] rps: memory leak due to forget to release rx queues
From: Changli Gao @ 2010-03-23 6:05 UTC (permalink / raw)
To: David S. Miller; +Cc: xiaosuo, Tom Herbert, netdev
memory leak due to forget to release rx queues.
rx queues is allocated in alloc_netdev_mq(), but they aren't released in
free_netdev(), and the field rps_map is also miss.
Signed-off-by: Changli Gao <xiaosuo@gmail.com>
----
net/core/dev.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/net/core/dev.c b/net/core/dev.c
index c0e2608..51b86e0 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5746,6 +5746,14 @@ void free_netdev(struct net_device *dev)
release_net(dev_net(dev));
+ if (dev->num_rx_queues) {
+ int i;
+
+ for (i = 0; i < dev->num_rx_queues; i++)
+ kfree(dev->_rx[i].rps_map);
+ kfree(dev->_rx);
+ }
+
kfree(dev->_tx);
/* Flush device addresses */
^ permalink raw reply related
* [PATCH] Fix locking in flush_backlog
From: Tom Herbert @ 2010-03-23 6:04 UTC (permalink / raw)
To: davem, netdev
Need to take spinlocks when dequeuing from input_pkt_queue in
flush_backlog. Also, with the spinlock the backlog queues can
be flushed directly from netdev_run_todo.
Signed-off-by: Tom Herbert <therbert@google.com>
---
diff --git a/net/core/dev.c b/net/core/dev.c
index a03aab4..e7db656 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2765,20 +2765,6 @@ int netif_receive_skb(struct sk_buff *skb)
}
EXPORT_SYMBOL(netif_receive_skb);
-/* Network device is going away, flush any packets still pending */
-static void flush_backlog(void *arg)
-{
- struct net_device *dev = arg;
- struct softnet_data *queue = &__get_cpu_var(softnet_data);
- struct sk_buff *skb, *tmp;
-
- skb_queue_walk_safe(&queue->input_pkt_queue, skb, tmp)
- if (skb->dev == dev) {
- __skb_unlink(skb, &queue->input_pkt_queue);
- kfree_skb(skb);
- }
-}
-
static int napi_gro_complete(struct sk_buff *skb)
{
struct packet_type *ptype;
@@ -5545,6 +5531,7 @@ void netdev_run_todo(void)
while (!list_empty(&list)) {
struct net_device *dev
= list_first_entry(&list, struct net_device, todo_list);
+ int i;
list_del(&dev->todo_list);
if (unlikely(dev->reg_state != NETREG_UNREGISTERING)) {
@@ -5556,7 +5543,22 @@ void netdev_run_todo(void)
dev->reg_state = NETREG_UNREGISTERED;
- on_each_cpu(flush_backlog, dev, 1);
+ /* Flush backlog queues of any pending packets */
+ for_each_online_cpu(i) {
+ struct softnet_data *queue = &per_cpu(softnet_data, i);
+ struct sk_buff *skb, *tmp;
+ unsigned long flags;
+
+ spin_lock_irqsave(&queue->input_pkt_queue.lock, flags);
+ skb_queue_walk_safe(&queue->input_pkt_queue, skb, tmp)
+ if (skb->dev == dev) {
+ __skb_unlink(skb,
+ &queue->input_pkt_queue);
+ kfree_skb(skb);
+ }
+ spin_unlock_irqrestore(&queue->input_pkt_queue.lock,
+ flags);
+ }
netdev_wait_allrefs(dev);
^ permalink raw reply related
* Re: [RFC Patch 1/3] netpoll: add generic support for bridge and bonding devices
From: Cong Wang @ 2010-03-23 5:15 UTC (permalink / raw)
To: David Miller
Cc: mpm, linux-kernel, netdev, bridge, gospo, nhorman, shemminger,
bonding-devel, fubar, jmoyer
In-Reply-To: <20100322.215822.123414773.davem@davemloft.net>
David Miller wrote:
> From: Cong Wang <amwang@redhat.com>
> Date: Tue, 23 Mar 2010 12:47:39 +0800
>
>> Yeah, for bonding case, probably. But for bridge case, I think
>> we still need to check all, right?
>
> Why? Who cares?
>
> If it goes out one port and reaches it's destination
> the objective has been achieved.
>
> Sending it out N more times achieves nothing.
We have to check which port has the right destination.
Ideally we should check the right destination address to
choose the port, but currently we don't have a generic
way to check this, thus I chose to send it to all ports.
You are right, this needs to be improved.
Thanks!
^ permalink raw reply
* Re: [RFC Patch 2/3] bridge: make bridge support netpoll
From: Cong Wang @ 2010-03-23 5:06 UTC (permalink / raw)
To: David Miller
Cc: mpm, linux-kernel, netdev, bridge, gospo, nhorman, shemminger,
bonding-devel, fubar
In-Reply-To: <20100322.215703.77339158.davem@davemloft.net>
David Miller wrote:
> From: Cong Wang <amwang@redhat.com>
> Date: Tue, 23 Mar 2010 12:39:35 +0800
>
>> How could you let the bridge know netpoll is not sent to
>> the one that doesn't support netpoll during setup? This will
>> be complex, I am afraid.
>
> Why does this matter at all?
Because currently we check netpoll support by ->ndo_poll_controller,
for example, tap driver doesn't have ->ndo_poll_controller now,
if I choose the target "@192.168.0.2/br0" where "192.168.0.2" is owned
by "tap0" which is managed by "br0", netconsole may not work.
>
> I told you in another mail that we should do away with
> these callbacks and all the crazy 'npinfo' assignments
> and just do it in the generic code.
I think ->ndo_poll_controller is not in the case that you talked about.
Thanks.
^ permalink raw reply
* Re: [RFC Patch 2/3] bridge: make bridge support netpoll
From: Cong Wang @ 2010-03-23 5:00 UTC (permalink / raw)
To: Matt Mackall
Cc: linux-kernel, netdev, bridge, Andy Gospodarek, Neil Horman,
Stephen Hemminger, bonding-devel, Jay Vosburgh, David Miller
In-Reply-To: <1269319861.3552.87.camel@calx>
Matt Mackall wrote:
> On Tue, 2010-03-23 at 12:39 +0800, Cong Wang wrote:
>> Matt Mackall wrote:
>>> On Tue, 2010-03-23 at 10:03 +0800, Cong Wang wrote:
>>>> Matt Mackall wrote:
>>>>> On Mon, 2010-03-22 at 04:17 -0400, Amerigo Wang wrote:
>>>>>> Based on the previous patch, make bridge support netpoll by:
>>>>>>
>>>>>> 1) implement the 4 methods to support netpoll for bridge;
>>>>>>
>>>>>> 2) modify netpoll during forwarding packets in bridge;
>>>>>>
>>>>>> 3) disable netpoll support of bridge when a netpoll-unabled device
>>>>>> is added to bridge;
>>>>> Not sure if this is the right thing to do. Shouldn't we simply enable
>>>>> polling on all devices that support it and warn about the others (aka
>>>>> best effort)?
>>>>>
>>>> I don't think it's a good idea, because we check if a device
>>>> supports netpoll by checking if it has ndo_poll_controller method.
>>> Uh, what? If we have 5 devices on a bridge and 4 support netpoll, then
>>> shouldn't we just send netconsole messages to those 4 devices? Isn't
>>> this much better than simply refusing to work?
>>>
>> How could you let the bridge know netpoll is not sent to
>> the one that doesn't support netpoll during setup? This will
>> be complex, I am afraid.
>
> I thought I saw a simple loop over bridge devices at poll time in your
> patch. So it should be a simple matter of skipping unsupported devices
> in that loop.
Nope, we need to check if the target address is owned by
a device that doesn't support netpoll or not, simple skipping
will not work.
>
> But Dave thinks there a bigger problems here, so I recommend first
> figuring out the architecture issues, then we can get back to the policy
> issues.
>
Ok. Thanks!
^ permalink raw reply
* Re: [RFC Patch 2/3] bridge: make bridge support netpoll
From: David Miller @ 2010-03-23 4:59 UTC (permalink / raw)
To: mpm
Cc: amwang, linux-kernel, netdev, bridge, gospo, nhorman, shemminger,
bonding-devel, fubar
In-Reply-To: <1269319861.3552.87.camel@calx>
From: Matt Mackall <mpm@selenic.com>
Date: Mon, 22 Mar 2010 23:51:01 -0500
> On Tue, 2010-03-23 at 12:39 +0800, Cong Wang wrote:
>> Matt Mackall wrote:
>> How could you let the bridge know netpoll is not sent to
>> the one that doesn't support netpoll during setup? This will
>> be complex, I am afraid.
>
> I thought I saw a simple loop over bridge devices at poll time in your
> patch. So it should be a simple matter of skipping unsupported devices
> in that loop.
It's because of all that "assign ->npinfo to slaves" crap he has to do
the way his patches are currently implemented.
It's basically another sign that the design is wrong.
^ permalink raw reply
* Re: [RFC Patch 1/3] netpoll: add generic support for bridge and bonding devices
From: David Miller @ 2010-03-23 4:58 UTC (permalink / raw)
To: amwang
Cc: mpm, linux-kernel, netdev, bridge, gospo, nhorman, shemminger,
bonding-devel, fubar, jmoyer
In-Reply-To: <4BA847EB.9040808@redhat.com>
From: Cong Wang <amwang@redhat.com>
Date: Tue, 23 Mar 2010 12:47:39 +0800
> Yeah, for bonding case, probably. But for bridge case, I think
> we still need to check all, right?
Why? Who cares?
If it goes out one port and reaches it's destination
the objective has been achieved.
Sending it out N more times achieves nothing.
^ permalink raw reply
* Re: [RFC Patch 2/3] bridge: make bridge support netpoll
From: David Miller @ 2010-03-23 4:57 UTC (permalink / raw)
To: amwang
Cc: mpm, linux-kernel, netdev, bridge, gospo, nhorman, shemminger,
bonding-devel, fubar
In-Reply-To: <4BA84607.7030304@redhat.com>
From: Cong Wang <amwang@redhat.com>
Date: Tue, 23 Mar 2010 12:39:35 +0800
> How could you let the bridge know netpoll is not sent to
> the one that doesn't support netpoll during setup? This will
> be complex, I am afraid.
Why does this matter at all?
I told you in another mail that we should do away with
these callbacks and all the crazy 'npinfo' assignments
and just do it in the generic code.
^ permalink raw reply
* Re: [Patch v2] netpoll: warn when there are spaces in parameters
From: Cong Wang @ 2010-03-23 4:56 UTC (permalink / raw)
To: Matt Mackall; +Cc: linux-kernel, netdev, elendil, David Miller
In-Reply-To: <1269319520.3552.76.camel@calx>
Matt Mackall wrote:
> On Tue, 2010-03-23 at 12:34 +0800, Cong Wang wrote:
>> Matt Mackall wrote:
>>> On Tue, 2010-03-23 at 09:44 +0800, Cong Wang wrote:
>>>> Matt Mackall wrote:
>>>>> On Mon, 2010-03-22 at 04:59 -0400, Amerigo Wang wrote:
>>>>>> + printk(KERN_INFO "%s: warning: whitespace"
>>>>>> + "is not allowed\n", np->name);
>>>>> Is it a warning or is it info? If it's a warning, then we probably need
>>>>> to add "netpoll" or whatever to the message so that people who've got a
>>>>> warning-level threshold will know what it's about.
>>>>>
>>>> If you mean KERN_INFO, yeah, I want to keep it in the same level
>>>> as other messages around.
>>> I should probably be more direct: I think that's the wrong thing to do.
>>> It IS a warning (it even says so!) telling users that something probably
>>> won't work and why and they might miss it if the severity is INFO and
>>> then come and ask us why things aren't working. So use KERN_WARN,
>>> please.
>>
>> They _are_ working, 0 will be assigned by default.
>
> If this patch has any point at all other than filling the logs, it
> should be WARN.
Others too.
>
>>> The other messages are INFO because when things are working, they're not
>>> interesting.
>>>
>> They are not all working, take this as an example:
>>
>> printk(KERN_INFO "%s: couldn't parse config at %s!\n",
>> np->name, cur);
>
> Yeah, that's obviously wrong too. Let's not add more wrongness just for
> consistency's sake.
>
Since you insist, please send a patch for all of these messages
in netpoll. This patch, from $subject, is aimed to warn spaces.
Thanks.
^ 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