* Re: linux-next: manual merge of the net tree with the i2c tree
From: Ben Hutchings @ 2009-11-10 15:10 UTC (permalink / raw)
To: Jean Delvare, David Miller
Cc: Stephen Rothwell, netdev, linux-next, linux-kernel, Mika Kuoppala
In-Reply-To: <20091110160246.506b7789@hyperion.delvare>
On Tue, 2009-11-10 at 16:02 +0100, Jean Delvare wrote:
> On Tue, 10 Nov 2009 13:22:58 +0000, Ben Hutchings wrote:
> > On Tue, 2009-11-10 at 12:42 +0100, Jean Delvare wrote:
> > > Ben, you can adjust your own patches to make use of this API instead of
> > > accessing the i2c_adapter mutex directly. That way, you are no longer
> > > dependent of implementation changes, and this should solve the conflict.
> > >
> > > Stephen, you can then drop your fixup patch.
> >
> > I don't think so, since the conflict resulted from joining two files
> > including sfe4001.c in net-next-2.6.
>
> My patch series no longer touches sfe4001.c, so how would the conflict
> remain?
Because your patch to introduce i2c_{lock,unlock}_adapter() are not in
net-next-2.6 yet.
David, you might want to pull from Linus and resolve this, giving
Stephen a break.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
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: [RFC PATCH] net: add dataref destructor to sk_buff
From: Gregory Haskins @ 2009-11-10 15:45 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: alacrityvm-devel, herbert.xu, linux-kernel, netdev
In-Reply-To: <20091110143652.GB19645@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 4225 bytes --]
Michael S. Tsirkin wrote:
> On Tue, Nov 10, 2009 at 09:11:10AM -0500, Gregory Haskins wrote:
>> Michael S. Tsirkin wrote:
>>> On Tue, Nov 10, 2009 at 05:40:50AM -0700, Gregory Haskins wrote:
>>>>>>> On 11/10/2009 at 6:53 AM, in message <20091110115335.GC6989@redhat.com>,
>>>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>
>>>>> Last time this was tried, this is the objection that was voiced:
>>>>>
>>>>> The problem with this patch is that it's tracking skb's, while
>>>>> you want use it to track pages for zero-copy. That just doesn't
>>>>> work. Through mechanisms like splice, individual pages in the
>>>>> skb can be detached and metastasize to other locations, e.g.,
>>>>> the VFS.
>>>> Right, and I don't think this applies here because I specifically chose the shinfo level to try to properly
>>>> track the page level avoid this issue. Multiple skb's can point to a single shinfo, iiuc.
>>> VFS does not know about shinfo either, does it?
>> I do not follow the reference. Where does VFS come into play?
>
> "Through mechanisms like splice, individual pages in the
> skb can be detached and metastasize to other locations, e.g.,
> the VFS"
Right, understood. What I mean is: How is that actually used in
real-life in a way that is valid?
What I am getting at is as follows: From a real basic perspective, you
can look at all of this as a simple synchronous call (i.e. sendmsg()).
The "app" (be it a userspace app, or a guest) prepares a buffer for
transmission, and offers it to the next layer in the stack. The app
must maintain the integrity of that buffer at least until the layer
below it signifies that it is "consumed". This may mean its a
synchronous call, like sendmsg(), or it may be asynchronous, like AIO.
But the key thing here is that at some point, the lower layer has to
signify that the buffer stability constraint has been met. In either
case, we have a clear delineated event: the io-completes = the buffer is
free to be reused.
In the simple case, the buffer in question is copied to a kernel buffer,
and the io completes immediately. In other cases (such as zero copy),
the buffer is mapped into the skb, and we have to wait for even lower
layers to signify the completion.
I am not a stack expert, but I was under the impression that we use this
model for userspace pages today as well using the wmem callbacks in
skb->destructor(). If so, I do not see how you could do something like
detach a page from a pskb and still expect to have a proper event that
delineates the io-completion to the higher layers.
So the questions are:
1) do we in fact map userspace pages to pskbs today?
2a) if so, how do we delineate the completion event?
2b) and how do we prevent worrying about the get_page() issue you refer
to.
>>
>>>>> In other words, this only *seems*
>>>>> to work for you because you are not trying to do things like
>>>>> guest to host communication, with host doing smart things.
>>>> I am not following what you mean here, as I do use this for guest->host and guest->host->remote, and
>>>> it works quite nicely. I map the guest pages in, and when the last reference to the pages are dropped,
>>>> I release the pages back to the guest. It doesn't matter if the skb egresses out a physical adapter or is
>>>> received locally. All that matters is the lifetime of the shinfo (and thus its pages) is handled correctly.
>>> Not if someone else is referencing the pages without a reference to shinfo.
>> I agree that if we can reference pages outside of the skb/shinfo then
>> there is a problem. I wasn't aware that we could do this, tbh.
>>
>> However, it seems to me that this is a problem with the overall stack,
>> if true....isn't it? For instance, if I do a sendmsg() from a userspace
>> app and block until its consumed,
>
> consumed == memcpy_from_iovec?
For non-zero-copy, sure why not.
>
>> how can the system function sanely if
>> the app returns from the call but something is still referencing the
>> page(s)?
>
> which pages?
You said that there are paths that get_page() out of shinfo without
holding a shinfo reference.
Kind Regards,
-Greg
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 267 bytes --]
^ permalink raw reply
* Re: [net-next-2.6 PATCH v5 5/5 RFC] TCPCT part1e: initial SYN exchange with SYNACK data
From: William Allen Simpson @ 2009-11-10 15:45 UTC (permalink / raw)
To: Eric Dumazet
Cc: Linux Kernel Network Developers, Ilpo Järvinen, Joe Perches
In-Reply-To: <4AF978D3.7030207@gmail.com>
Eric Dumazet wrote:
> William Allen Simpson a écrit :
>> This is a significantly revised implementation of an earlier (year-old)
>> patch that no longer applies cleanly, with permission of the original
>> author (Adam Langley). That patch was previously reviewed:
>>
>> http://thread.gmane.org/gmane.linux.network/102586
> I really tried hard to understand what was going on, and failed, because I dont
> have much time these days...
>
Thanks very much for your time.
As I just wrote in a previous message, this is the easy part. It's going
to get much more complicated -- as you know, since I sent you the papers
with 30+ references....
> Lack of documentation maybe ? Some DATA flow could help...
>
Where should that be? In the commit messages?
> Please please, cook up elementatry patches to perform one action at a time,
> even if they are not fully functionnal ?
>
Well, that's what I did with the (now) parts 1b, 1c, and 1d, but somebody
complained that the callers didn't exist yet.
> One patch to be able to send SYN + COOKIE (if we are the client)
>
> One patch to be able to receive this SYN + COOKIE and answer a SYNACK + cookies (we are the server)
>
I'll split them. Remember, I started with a code base previously
submitted, and everything was in one *single* large patch. I thought
that's what you (Linux folks in general) wanted.
> One patch to ... Receive the ACK from client (if we are the server) and check cookies
>
> One patch to ... Send the ACK (if we are the client)
>
> Patches to receive FIN + cookies
>
Maybe that's what you are missing, and why you're have difficulty
wrapping your mind around it. None of that is there yet!
Receive the Ack options from the client is currently part 2. I've also
lately divided part 2 into 4 sub-patches, although it all has to be
committed at one time:
2a) extending the TCP option space,
2b) receiving 64-bit timestamps,
2c) receiving extended SACKs,
2d) receiving the cookie pair.
Perhaps that could be split even further, although the order has to
stay the same. A lot is written, although I gave up updating regularly
as the part 1 process has taken so long....
Send the Ack is currently part 3.
Handling FIN is currently part 4, although it might be split, too.
Handling RST is probably part 5.
Handling transactions was part 4 or 5, but could be a future part 6
instead (although it's rather tied up with both 4 and 5).
> One patch to ... enable the whole thing and setsockopt() bits
>
Ummm, then there'd be no way to test. This is TCP we're talking about.
It has to be tested extensively. This stuff needs to work. I need the
bits to initiate all the tests....
That's why I like to get all my data structures nailed down first, and
then incrementally add code and test.
^ permalink raw reply
* Re: [RFC PATCH] net: add dataref destructor to sk_buff
From: Gregory Haskins @ 2009-11-10 15:47 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: alacrityvm-devel, herbert.xu, linux-kernel, netdev
In-Reply-To: <20091110144543.GC19645@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 806 bytes --]
Michael S. Tsirkin wrote:
> On Tue, Nov 10, 2009 at 09:11:10AM -0500, Gregory Haskins wrote:
>>> You are comparing with skb destructors, not skb data destructors :) skb
>>> data destructor is Rusty's patch which he wanted to use for vringfd. I
>>> mean e.g. this:
>>> http://lists.openwall.net/netdev/2008/04/18/7
>> Ah, I wasn't aware. I believe Anthony Ligouri had pointed me at this
>> same patch earlier this year. However, more recently when I saw
>> skb->destructor() in mainline (seemingly from Rusty), I thought it had
>> been accepted and didn't investigate further.
>
> skb->destructor seems to be there as far back as 2.2.0
> http://lxr.linux.no/#linux-old+v2.2.0/include/linux/skbuff.h
>
Right, I meant recent activity around it for fixing it up related to
leaks, etc.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 267 bytes --]
^ permalink raw reply
* net 01/02: allow to propagate errors through ->ndo_hard_start_xmit()
From: Patrick McHardy @ 2009-11-10 16:14 UTC (permalink / raw)
To: David S. Miller; +Cc: Linux Netdev List
[-- Attachment #1: Type: text/plain, Size: 438 bytes --]
The following two patches add support for error propagation
through ->ndo_hard_start_xmit() and use it to propagate qdisc
submission state through VLAN and macvlan devices.
I'm also experimenting with returning errno codes from
tunnel devices to the originating socket, but those patches
still need some work.
The only change relative to the patch posted yesterday is
removal of an unnecessary initialization in dev_hard_start_xmit().
[-- Attachment #2: 01.diff --]
[-- Type: text/x-patch, Size: 6241 bytes --]
commit ebf56943a164bfee312844bffff8f6b6ff8183dc
Author: Patrick McHardy <kaber@trash.net>
Date: Tue Nov 10 16:55:45 2009 +0100
net: allow to propagate errors through ->ndo_hard_start_xmit()
Currently the ->ndo_hard_start_xmit() callbacks are only permitted to return
one of the NETDEV_TX codes. This prevents any kind of error propagation for
virtual devices, like queue congestion of the underlying device in case of
layered devices, or unreachability in case of tunnels.
This patches changes the NET_XMIT codes to avoid clashes with the NETDEV_TX
codes and changes the two callers of dev_hard_start_xmit() to expect either
errno codes, NET_XMIT codes or NETDEV_TX codes as return value.
In case of qdisc_restart(), all non NETDEV_TX codes are mapped to NETDEV_TX_OK
since no error propagation is possible when using qdiscs. In case of
dev_queue_xmit(), the error is propagated upwards.
Signed-off-by: Patrick McHardy <kaber@trash.net>
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 465add6..ab2812c 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -63,27 +63,48 @@ struct wireless_dev;
#define HAVE_FREE_NETDEV /* free_netdev() */
#define HAVE_NETDEV_PRIV /* netdev_priv() */
-#define NET_XMIT_SUCCESS 0
-#define NET_XMIT_DROP 1 /* skb dropped */
-#define NET_XMIT_CN 2 /* congestion notification */
-#define NET_XMIT_POLICED 3 /* skb is shot by police */
-#define NET_XMIT_MASK 0xFFFF /* qdisc flags in net/sch_generic.h */
+/*
+ * Transmit return codes: transmit return codes originate from three different
+ * namespaces:
+ *
+ * - qdisc return codes
+ * - driver transmit return codes
+ * - errno values
+ *
+ * Drivers are allowed to return any one of those in their hard_start_xmit()
+ * function. Real network devices commonly used with qdiscs should only return
+ * the driver transmit return codes though - when qdiscs are used, the actual
+ * transmission happens asynchronously, so the value is not propagated to
+ * higher layers. Virtual network devices transmit synchronously, in this case
+ * the driver transmit return codes are consumed by dev_queue_xmit(), all
+ * others are propagated to higher layers.
+ */
+
+/* qdisc ->enqueue() return codes. */
+#define NET_XMIT_SUCCESS 0x00
+#define NET_XMIT_DROP 0x10 /* skb dropped */
+#define NET_XMIT_CN 0x20 /* congestion notification */
+#define NET_XMIT_POLICED 0x30 /* skb is shot by police */
+#define NET_XMIT_MASK 0xf0 /* qdisc flags in net/sch_generic.h */
/* Backlog congestion levels */
-#define NET_RX_SUCCESS 0 /* keep 'em coming, baby */
-#define NET_RX_DROP 1 /* packet dropped */
+#define NET_RX_SUCCESS 0 /* keep 'em coming, baby */
+#define NET_RX_DROP 1 /* packet dropped */
/* NET_XMIT_CN is special. It does not guarantee that this packet is lost. It
* indicates that the device will soon be dropping packets, or already drops
* some packets of the same priority; prompting us to send less aggressively. */
-#define net_xmit_eval(e) ((e) == NET_XMIT_CN? 0 : (e))
+#define net_xmit_eval(e) ((e) == NET_XMIT_CN ? 0 : (e))
#define net_xmit_errno(e) ((e) != NET_XMIT_CN ? -ENOBUFS : 0)
/* Driver transmit return codes */
+#define NETDEV_TX_MASK 0xf
+
enum netdev_tx {
- NETDEV_TX_OK = 0, /* driver took care of packet */
- NETDEV_TX_BUSY, /* driver tx path was busy*/
- NETDEV_TX_LOCKED = -1, /* driver tx lock was already taken */
+ __NETDEV_TX_MIN = INT_MIN, /* make sure enum is signed */
+ NETDEV_TX_OK = 0, /* driver took care of packet */
+ NETDEV_TX_BUSY = 1, /* driver tx path was busy*/
+ NETDEV_TX_LOCKED = 2, /* driver tx lock was already taken */
};
typedef enum netdev_tx netdev_tx_t;
diff --git a/net/core/dev.c b/net/core/dev.c
index bf629ac..babd45f 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1756,7 +1756,7 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
struct netdev_queue *txq)
{
const struct net_device_ops *ops = dev->netdev_ops;
- int rc;
+ int rc = NETDEV_TX_OK;
if (likely(!skb->next)) {
if (!list_empty(&ptype_all))
@@ -1804,6 +1804,8 @@ gso:
nskb->next = NULL;
rc = ops->ndo_start_xmit(nskb, dev);
if (unlikely(rc != NETDEV_TX_OK)) {
+ if (rc & ~NETDEV_TX_MASK)
+ goto out_kfree_gso_skb;
nskb->next = skb->next;
skb->next = nskb;
return rc;
@@ -1813,11 +1815,12 @@ gso:
return NETDEV_TX_BUSY;
} while (skb->next);
- skb->destructor = DEV_GSO_CB(skb)->destructor;
-
+out_kfree_gso_skb:
+ if (likely(skb->next == NULL))
+ skb->destructor = DEV_GSO_CB(skb)->destructor;
out_kfree_skb:
kfree_skb(skb);
- return NETDEV_TX_OK;
+ return rc;
}
static u32 skb_tx_hashrnd;
@@ -1905,6 +1908,23 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
return rc;
}
+static inline bool dev_xmit_complete(int rc)
+{
+ /* successful transmission */
+ if (rc == NETDEV_TX_OK)
+ return true;
+
+ /* error while transmitting, driver consumed skb */
+ if (rc < 0)
+ return true;
+
+ /* error while queueing to a different device, driver consumed skb */
+ if (rc & NET_XMIT_MASK)
+ return true;
+
+ return false;
+}
+
/**
* dev_queue_xmit - transmit a buffer
* @skb: buffer to transmit
@@ -2002,8 +2022,8 @@ gso:
HARD_TX_LOCK(dev, txq, cpu);
if (!netif_tx_queue_stopped(txq)) {
- rc = NET_XMIT_SUCCESS;
- if (!dev_hard_start_xmit(skb, dev, txq)) {
+ rc = dev_hard_start_xmit(skb, dev, txq);
+ if (dev_xmit_complete(rc)) {
HARD_TX_UNLOCK(dev, txq);
goto out;
}
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 4ae6aa5..b13821a 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -120,8 +120,15 @@ int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
HARD_TX_LOCK(dev, txq, smp_processor_id());
if (!netif_tx_queue_stopped(txq) &&
- !netif_tx_queue_frozen(txq))
+ !netif_tx_queue_frozen(txq)) {
ret = dev_hard_start_xmit(skb, dev, txq);
+
+ /* an error implies that the skb was consumed */
+ if (ret < 0)
+ ret = NETDEV_TX_OK;
+ /* all NET_XMIT codes map to NETDEV_TX_OK */
+ ret &= ~NET_XMIT_MASK;
+ }
HARD_TX_UNLOCK(dev, txq);
spin_lock(root_lock);
^ permalink raw reply related
* vlan/macvlan 02/02: propagate transmission state to upper layers
From: Patrick McHardy @ 2009-11-10 16:14 UTC (permalink / raw)
To: David S. Miller; +Cc: Linux Netdev List
[-- Attachment #1: Type: text/plain, Size: 0 bytes --]
[-- Attachment #2: 02.diff --]
[-- Type: text/x-patch, Size: 1514 bytes --]
commit 21fb413b55abb4665e057b884c289bb780f41b3c
Author: Patrick McHardy <kaber@trash.net>
Date: Tue Nov 10 16:55:50 2009 +0100
vlan/macvlan: propagate transmission state to upper layers
Both vlan and macvlan devices usually don't use a qdisc and immediately
queue packets to the underlying device. Propagate transmission state of
the underlying device to the upper layers so they can react on congestion
and/or inform the sending process.
Signed-off-by: Patrick McHardy <kaber@trash.net>
diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index d7dba3f..271aa7e 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -202,7 +202,7 @@ static netdev_tx_t macvlan_start_xmit(struct sk_buff *skb,
} else
txq->tx_dropped++;
- return NETDEV_TX_OK;
+ return ret;
}
static int macvlan_hard_header(struct sk_buff *skb, struct net_device *dev,
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 790fd55..9159659 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -332,7 +332,7 @@ static netdev_tx_t vlan_dev_hard_start_xmit(struct sk_buff *skb,
} else
txq->tx_dropped++;
- return NETDEV_TX_OK;
+ return ret;
}
static netdev_tx_t vlan_dev_hwaccel_hard_start_xmit(struct sk_buff *skb,
@@ -358,7 +358,7 @@ static netdev_tx_t vlan_dev_hwaccel_hard_start_xmit(struct sk_buff *skb,
} else
txq->tx_dropped++;
- return NETDEV_TX_OK;
+ return ret;
}
static int vlan_dev_change_mtu(struct net_device *dev, int new_mtu)
^ permalink raw reply related
* Bridge + Conntrack + SKB Recycle: Fragment Reassembly Errors
From: ben @ 2009-11-10 16:09 UTC (permalink / raw)
To: netdev
We have observed significant reassembly errors when combining
routing/bridging with conntrack + nf_defrag_ipv4 loaded, and
skb_recycle_check - enabled interfaces. For our test, we had a single
linux device with two interfaces (gianfars in this case) with SKB
recycling enabled. We sent large, continuous pings across the bridge,
like this:
ping -s 64000 -A <dest IP>
Then, we ran netstat -s --raw, and noticed that IPSTATS_MIB_REASMFAILS
were happening for about 40% of the received datagrams. Tracing the
code in ip_fragment.c, we instrumented each of the
IPSTATS_MIB_REASMFAILS locations, and found the culprit to be
ip_evictor. Nothing looked unusual here, so we placed tracing in
ip_frag_queue, directly above:
atomic_add(skb->truesize, &qp->q.net->mem);
We noticed that quite a few of the skb->truesize numbers were in the 67K
range, which quickly overwhelms the default 192K-ish ipfrag_low_thresh.
This means that the next time inet_frag_evictor is run:
work = atomic_read(&nf->mem) - nf->low_thresh;
Will surely be positive, and it is likely that our huge-frag-containing
queue will be one of those evicted.
Looking at the source of these huge skbs, it seems that during
re-fragmentation in br_nf_dev_queue_xmit (which calls ip_fragment with
CONFIG_NF_CONNTRACK_IPV4 enabled), the huge datagram that was allocated
to hold a successfully-reassembled skb may be getting reused? In any
case, when skb_recycle_check(skb, min_rx_size) is called, the huge
(skb->truesize huge, not data huge) skb is recycled for use on RX, and
it eventually gets enqueued for reassembly, causing the
inet_frag_evictor to have a positive work value.
Our solution was to add an upper-bounds check to skb_recycle_check,
which prevents the large-ish SKBs from being used to create future
frags, and overwhelming ipfrag_low_thresh. This seems quite clunky,
although I would be happy to submit this as a patch...
If this is not the right place...what is the "right" place?
Ben Menchaca
Bigfoot Networks
^ permalink raw reply
* [PATCH] ixgbe: r_idx not used in ixgbe_msix_clean_rx()
From: Ali Gholami Rudi @ 2009-11-10 16:40 UTC (permalink / raw)
To: David S. Miller
Cc: e1000-devel, netdev, Bruce Allan, Jesse Brandeburg, linux-kernel,
John Ronciak, Jeff Kirsher
The values of r_idx and rx_ring are not used after the last time they
are set in ixgbe_msix_clean_rx(), so they can be removed.
Signed-off-by: Ali Gholami Rudi <ali@rudi.ir>
---
drivers/net/ixgbe/ixgbe_main.c | 2 --
1 files changed, 0 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
index 5bd9e6b..6541c80 100644
--- a/drivers/net/ixgbe/ixgbe_main.c
+++ b/drivers/net/ixgbe/ixgbe_main.c
@@ -1344,8 +1344,6 @@ static irqreturn_t ixgbe_msix_clean_rx(int irq, void *data)
if (!q_vector->rxr_count)
return IRQ_HANDLED;
- r_idx = find_first_bit(q_vector->rxr_idx, adapter->num_rx_queues);
- rx_ring = &(adapter->rx_ring[r_idx]);
/* disable interrupts on this vector only */
ixgbe_irq_disable_queues(adapter, ((u64)1 << q_vector->v_idx));
napi_schedule(&q_vector->napi);
------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now. http://p.sf.net/sfu/bobj-july
^ permalink raw reply related
* Re: [PATCH] ixgbe: r_idx not used in ixgbe_msix_clean_rx()
From: Peter P Waskiewicz Jr @ 2009-11-10 16:45 UTC (permalink / raw)
To: Ali Gholami Rudi
Cc: e1000-devel@lists.sourceforge.net, netdev@vger.kernel.org,
Allan, Bruce W, Brandeburg, Jesse, linux-kernel@vger.kernel.org,
Ronciak, John, Kirsher, Jeffrey T, David S. Miller
In-Reply-To: <20091110164006.GB816@lilem.mirepesht>
On Tue, 2009-11-10 at 08:40 -0800, Ali Gholami Rudi wrote:
> The values of r_idx and rx_ring are not used after the last time they
> are set in ixgbe_msix_clean_rx(), so they can be removed.
>
Thanks for cleaning this up.
Acked-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
> Signed-off-by: Ali Gholami Rudi <ali@rudi.ir>
> ---
> drivers/net/ixgbe/ixgbe_main.c | 2 --
> 1 files changed, 0 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
> index 5bd9e6b..6541c80 100644
> --- a/drivers/net/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ixgbe/ixgbe_main.c
> @@ -1344,8 +1344,6 @@ static irqreturn_t ixgbe_msix_clean_rx(int irq, void *data)
> if (!q_vector->rxr_count)
> return IRQ_HANDLED;
>
> - r_idx = find_first_bit(q_vector->rxr_idx, adapter->num_rx_queues);
> - rx_ring = &(adapter->rx_ring[r_idx]);
> /* disable interrupts on this vector only */
> ixgbe_irq_disable_queues(adapter, ((u64)1 << q_vector->v_idx));
> napi_schedule(&q_vector->napi);
------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now. http://p.sf.net/sfu/bobj-july
^ permalink raw reply
* Re: [PATCH] ifb: add multi-queue support
From: Stephen Hemminger @ 2009-11-10 16:45 UTC (permalink / raw)
To: Changli Gao
Cc: Eric Dumazet, Patrick McHardy, David S. Miller, netdev,
Tom Herbert
In-Reply-To: <412e6f7f0911100549q68a57ba7s83b11be3a77323a2@mail.gmail.com>
On Tue, 10 Nov 2009 21:49:31 +0800
Changli Gao <xiaosuo@gmail.com> wrote:
> On Tue, Nov 10, 2009 at 9:34 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > Changli Gao a écrit :
> >
> > I believe Patrick was referring to vlan get_tx_queues() implementation,
> > but it actually gets values from real device (tb[IFLA_LINK])
> >
> > In your case you'll need to add a new IFLA_NTXQ attribute, and
> > change iproute2 to pass this new attribute at link creation.
> > (check include/linux/if_link.h)
> >
> > ip link add link ..... ntxq 2
> >
> > static int vlan_get_tx_queues(struct net *net,
> > struct nlattr *tb[],
> > unsigned int *num_tx_queues,
> > unsigned int *real_num_tx_queues)
> > {
> > struct net_device *real_dev;
> >
> > if (!tb[IFLA_LINK])
> > return -EINVAL;
> >
> > real_dev = __dev_get_by_index(net, nla_get_u32(tb[IFLA_LINK]));
> > if (!real_dev)
> > return -ENODEV;
> >
> > *num_tx_queues = real_dev->num_tx_queues;
> > *real_num_tx_queues = real_dev->real_num_tx_queues;
> > return 0;
> > }
> >
> >
> >
>
> got it. Thanks. BTW: why not merge iproute2 into linux, just like perf.
>
Distro's would find that hard, also it would cause people to forgot binary
API compatibility.
--
^ permalink raw reply
* Re: vlan/macvlan 02/02: propagate transmission state to upper layers
From: Stephen Hemminger @ 2009-11-10 16:48 UTC (permalink / raw)
To: Patrick McHardy; +Cc: David S. Miller, Linux Netdev List
In-Reply-To: <4AF99160.2060607@trash.net>
On Tue, 10 Nov 2009 17:14:24 +0100
Patrick McHardy <kaber@trash.net> wrote:
> vlan/macvlan: propagate transmission state to upper layers
>
> Both vlan and macvlan devices usually don't use a qdisc and immediately
> queue packets to the underlying device. Propagate transmission state of
> the underlying device to the upper layers so they can react on congestion
> and/or inform the sending process.
>
> Signed-off-by: Patrick McHardy <kaber@trash.net>
Bridging and bonding have same issue, but the solution is more difficult.
^ permalink raw reply
* Re: [net-next-2.6 PATCH v5 5/5 RFC] TCPCT part1e: initial SYN exchange with SYNACK data
From: William Allen Simpson @ 2009-11-10 16:49 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: Linux Kernel Network Developers, Eric Dumazet, Joe Perches
In-Reply-To: <alpine.DEB.2.00.0911101605050.20637@melkinpaasi.cs.helsinki.fi>
Ilpo Järvinen wrote:
> After some time I returned to it. My apologies for my previous harsh
> response. My point was to say that you must separate such changes because
> they add to noise which makes it rather annoying to review. At least my
> brains are rather limited still in keeping track of different things.
>
Apology accepted.
I'd think we agree, and that's why I wouldn't go around fixing all the
misspelled comments -- just those in the code I'm actually revising.
>> This *is* actually in CodingStyle (line 679), and I'm trying to conform:
>
> Sure, fell free to but not in some random places like you now do in this
> patch. If you are not touching a line because of a real change and want do
> a syntax cleanup, please do it in another patch. Especially when your
> actual change is as complicated as this is.
>
Not a single one is in a random place.
>>> ...Also some comment changes which certainly are not mandatory nor even
>>> related.
>>>
>> 1) The first is a hole left by the removal of the data fields some time
>> ago, but they left the old (now incorrect) comment:
>>
>> -/* SACKs data */
>> + u8 cookie_plus:6, /* bytes in authenticator/cookie option */
>> + cookie_out_never:1,
>> + cookie_in_always:1;
>> u8 num_sacks; /* Number of SACK blocks */
>>...
> I'm sorry but this response tells me that you don't seem to know even
> yourself what you were submitting. Does this ring a bell:
>
> - if (th->doff == sizeof(struct tcphdr) >> 2) {
> + /* In the spirit of fast parsing, compare doff directly to shifted
> + * constant values. Because equality is used, short doff can be
> + * ignored here, and checked later.
> + */
> + if (th->doff == (sizeof(*th) >> 2)) {
>
Ah, it's become apparent that you didn't click through to any of the
references, internet-drafts, or mailing list discussion, so you don't
know what this patch series is implementing.
One of the great difficulties in tracking down all several hundred uses
of doff -- and functions that use doff -- is the serious deficiency of
comments (compared to other code bases).
Oh yeah -- Miller says I'm anal. This is TCP. Strict scrutiny is much
better than the alternative.
> Besides, I don't understand even what you're trying to say. ...I think we
> already checked the length in tcp_v[46]_rcv against underflow. Why you add
> such a non-sense comment here (plus the sizeof change I covered
> elsewhere)? This is just a noise to annoy reviewer, nothing else.
>
You merely think? You don't know? (I didn't find one on the code walk
through that got me to this point in the code, but perhaps I missed
something.) If that's the case, there should at least be a comment that
underflow was already checked, and *where* it was checked!
The lack of pertinent comments and error checking annoys the next coder
that comes down the pike.
>> 2) The second is a spelling error that I noticed in passing. It's within
>> the same diff -u area (close to other insertions both before and after), so
>> fixing it was trivial:
>>
>> - * to increse this, although since:
>> + * to increase this, although since:
>>
>> Are we not supposed to fix spelling errors in comments?
>
> How many extra lines a reviewer has to read because of that? Why not do it
> in a separate patch. git add -i / git stash would help you there. ...To be
> honest, I'd rather have typos in comments which like this are not
> trouble for the reader than clutter feature patches with all this random
> junk.
>
I'll have to look up the usage of git add -i / git stash, but in any case,
I'm not likely to go around fixing comments later.
Each patch requires a great deal of time. I'd rather combine such things,
or not do them at all.
> ...I really wish that next time when you submit you get rid of all these
> showstoppers and annoyances (ie., at least honestly try to do your best)
> that we could finally concentrate to the actual change and reviewing
> that :-). My recommendation is that before hitting the send button you
> review (read through) your own patches, and if you find something to
> correct instead of submitting, postpone that until the problem is solved.
> Like DaveM once noted somebody, you'll not be timed :-). ...I usually do
> that (read my own patches) and end up rather often to not submit (and find
> even real bugs that way quite often).
>
Maybe you've been dealing too much with some folks that submitted a patch
that was accepted (over my expressed reservations), and then had to send
out 2 emergency patches to handle crashing bugs last week....
Before I send any patch, it's been compared against my last patch sent,
*and* tested for compilation individually, *and* _usually_ tested pretty
thoroughly -- although not this time, because the net-next-2.6 modules
don't compile, as I warned at the head of this patch series.
Because the compile blew up, I actually spent my Sunday evening
re-reviewing the patches again! I wanted to ensure it wasn't something
that I'd done, so I recompiled everything again incrementally.
It took me more than a day to make the syntax changes requested, and
another 6 hours to prepare and send these 5 patches.
I cannot promise there will never be any bugs, this patch series will be
much too complicated for that, but the very idea that I'd send without
reviewing.... At this point, I'm rather personally insulted.
And now I'm done with email for the day. I'll do the split that Eric
suggested, and send it out whenever the damn modules compile again.
In the meantime, I'd appreciate that you "finally concentrate"....
^ permalink raw reply
* Re: Bridge + Conntrack + SKB Recycle: Fragment Reassembly Errors
From: Patrick McHardy @ 2009-11-10 16:50 UTC (permalink / raw)
To: ben; +Cc: netdev
In-Reply-To: <767BAF49E93AFB4B815B11325788A8ED45F0BA@L01SLCXDB03.calltower.com>
ben@bigfootnetworks.com wrote:
> We have observed significant reassembly errors when combining
> routing/bridging with conntrack + nf_defrag_ipv4 loaded, and
> skb_recycle_check - enabled interfaces. For our test, we had a single
> linux device with two interfaces (gianfars in this case) with SKB
> recycling enabled. We sent large, continuous pings across the bridge,
> like this:
> ping -s 64000 -A <dest IP>
>
> Then, we ran netstat -s --raw, and noticed that IPSTATS_MIB_REASMFAILS
> were happening for about 40% of the received datagrams. Tracing the
> code in ip_fragment.c, we instrumented each of the
> IPSTATS_MIB_REASMFAILS locations, and found the culprit to be
> ip_evictor. Nothing looked unusual here, so we placed tracing in
> ip_frag_queue, directly above:
> atomic_add(skb->truesize, &qp->q.net->mem);
>
> We noticed that quite a few of the skb->truesize numbers were in the 67K
> range, which quickly overwhelms the default 192K-ish ipfrag_low_thresh.
> This means that the next time inet_frag_evictor is run:
> work = atomic_read(&nf->mem) - nf->low_thresh;
>
> Will surely be positive, and it is likely that our huge-frag-containing
> queue will be one of those evicted.
>
> Looking at the source of these huge skbs, it seems that during
> re-fragmentation in br_nf_dev_queue_xmit (which calls ip_fragment with
> CONFIG_NF_CONNTRACK_IPV4 enabled), the huge datagram that was allocated
> to hold a successfully-reassembled skb may be getting reused? In any
> case, when skb_recycle_check(skb, min_rx_size) is called, the huge
> (skb->truesize huge, not data huge) skb is recycled for use on RX, and
> it eventually gets enqueued for reassembly, causing the
> inet_frag_evictor to have a positive work value.
Interesting problem. I wonder what the linear size of the skb was
and whether we're just not properly adjusting truesize of the head
during refragmentation.
This code in ip_fragment() looks suspicious:
if (skb_has_frags(skb)) {
...
skb_walk_frags(skb, frag) {
...
if (skb->sk) {
frag->sk = skb->sk;
frag->destructor = sock_wfree;
truesizes += frag->truesize;
}
truesizes is later used to adjust truesize of the head skb.
For some reason this is only done when it originated from a
local socket.
> Our solution was to add an upper-bounds check to skb_recycle_check,
> which prevents the large-ish SKBs from being used to create future
> frags, and overwhelming ipfrag_low_thresh. This seems quite clunky,
> although I would be happy to submit this as a patch...
This seems reasonable to me, there might be large skbs for
different reasons.
^ permalink raw reply
* [RFC] vlan: GRO rx statistics
From: Eric Dumazet @ 2009-11-10 16:52 UTC (permalink / raw)
To: David S. Miller; +Cc: Herbert Xu, Linux Netdev List
It seems vlan interfaces with hardware-assisted VLAN reception
dont have rx statistics.
Following patch cures the problem for me but I have no idea
if it is correct ?
Another problem about all rx stats is about multi rx queues devices.
Several cpus can access these counters in parallel ?
diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index 8d5ca2a..e13b007 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -89,6 +89,11 @@ vlan_gro_common(struct napi_struct *napi, struct vlan_group *grp,
if (!skb->dev)
goto drop;
+ skb->dev->stats.rx_packets++;
+ skb->dev->stats.rx_bytes += skb->len;
+ if (skb->pkt_type == PACKET_MULTICAST)
+ skb->dev->stats.multicast++;
+
for (p = napi->gro_list; p; p = p->next) {
NAPI_GRO_CB(p)->same_flow =
p->dev == skb->dev && !compare_ether_header(
^ permalink raw reply related
* Re: vlan/macvlan 02/02: propagate transmission state to upper layers
From: Patrick McHardy @ 2009-11-10 16:56 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: David S. Miller, Linux Netdev List
In-Reply-To: <20091110084814.02593603@nehalam>
Stephen Hemminger wrote:
> On Tue, 10 Nov 2009 17:14:24 +0100
> Patrick McHardy <kaber@trash.net> wrote:
>
>> vlan/macvlan: propagate transmission state to upper layers
>>
>> Both vlan and macvlan devices usually don't use a qdisc and immediately
>> queue packets to the underlying device. Propagate transmission state of
>> the underlying device to the upper layers so they can react on congestion
>> and/or inform the sending process.
>>
>> Signed-off-by: Patrick McHardy <kaber@trash.net>
>
>
> Bridging and bonding have same issue, but the solution is more difficult.
Yes, in both cases the packet might be sent out on multiple
interfaces, so its not really clear which state we should
propagate. I guess it would make sense to indicate an error
if transmission on *all* interfaces fail, but I'm not sure
about the other cases.
^ permalink raw reply
* Re: Bugfix for iproute2
From: Stephen Hemminger @ 2009-11-10 17:12 UTC (permalink / raw)
To: Sven Anders; +Cc: netdev
In-Reply-To: <4AEF6F14.7080802@anduras.de>
On Tue, 03 Nov 2009 00:45:24 +0100
Sven Anders <anders@anduras.de> wrote:
> Hello!
>
> I already send this mail to Stephen Hemminger, but I'm not sure if it reached him.
> Moreover I tried to join the LARTC mailing list (http://mailman.ds9a.nl/mailman/listinfo/lartc)
> but it did not work. Is this mailing list dead?
> Therefore I sent this mail again over this mailing list...
>
>
> I experienced an error, if I try to perform a
>
> ip route flush proto 4
>
> with many routes in a complex environment, it
> gave me the following error:
>
> Failed to send flush request: Success
> Flush terminated
>
> I'm using version 2.6.29.
> I check GIT, but there was only the "MSG_PEEK" fix.
>
> I tracked it down to the rtnl_send_check() function
> in lib/libnetlink.c.
>
> In this function there is the following for loop:
>
> for (h = (struct nlmsghdr *)resp; NLMSG_OK(h, status);
> h = NLMSG_NEXT(h, status)) {
> if (h->nlmsg_type == NLMSG_ERROR) {
> struct nlmsgerr *err = (struct nlmsgerr*)NLMSG_DATA(h);
> if (h->nlmsg_len < NLMSG_LENGTH(sizeof(struct nlmsgerr)))
> fprintf(stderr, "ERROR truncated\n");
> else
> errno = -err->error;
> }
> return -1;
> }
>
> I think the "return -1;" should be inside the if statement.
>
> I attached a patch for this. The first part of the patch is taken from
> one of the late git commits.
>
> Please note me, if the fix is wrong...
>
> Regards
> Sven Anders
>
Applied thanks.
--
^ permalink raw reply
* Re: iproute uses too small of a receive buffer
From: Stephen Hemminger @ 2009-11-10 17:15 UTC (permalink / raw)
To: Patrick McHardy; +Cc: Ben Greear, Eric Dumazet, NetDev
In-Reply-To: <4AE8A098.8040207@trash.net>
>
> How about this? It uses 1MB as receive buf limit by default (without
> increasing /proc/sys/net/core/rmem_max it will be limited by less
> however) and allows to specify the size manually using "-rcvbuf X"
> (-r is already used, so you need to specify at least -rc).
>
> Additionally rtnl_listen() continues on ENOBUFS after printing the
> error message.
Applied, seems like the best workaround
^ permalink raw reply
* Re: RFC: net: allow to propagate errors through ->ndo_hard_start_xmit()
From: Jarek Poplawski @ 2009-11-10 17:08 UTC (permalink / raw)
To: Patrick McHardy
Cc: Linux Netdev List, Herbert Xu, David S. Miller, Stephen Hemminger
In-Reply-To: <4AF87070.6020007@trash.net>
On Mon, Nov 09, 2009 at 08:41:36PM +0100, Patrick McHardy wrote:
> I've updated my patch to propagate error values (errno and NET_XMIT
> codes) through ndo_hard_start_xmit() and incorporated the suggestions
> made last time, namely:
>
> - move slightly complicated return value check to inline function and
> add a few comments
>
> - fix error handling while in the middle of transmitting GSO skbs
>
> I've also audited the tree once again for invalid return values and
> found a single remaining instance in a Wimax driver, I'll take care
> of that later.
>
> Two questions remain:
>
> - I'm not sure the error handling in dev_hard_start_xmit() for GSO
> skbs is optimal. When the driver returns an error, it is assumed
> the current segment has been freed. The patch then frees the
> entire GSO skb, including all remaining segments. Alternatively
> it could try to transmit the remaining segments later.
Anyway, it seems this freeing should be described in the changelog,
if not moved to a separate patch, since it fixes another problem,
unless I forgot something.
>
> - Stephen recently introduced an enum for the netdev_tx codes, with
> this patch drivers are allowed to return different values.
> The mainly useful part about Stephen's patch IMO is the netdev_tx
> typedef to make it easier to locate ndo_start_xmit() functions
> which are defined in different files than the netdev_ops.
> So we could remove the enum again and simply typedef an int.
>
> Any opinions are welcome :)
> commit 08a98f11bc1c1452df74c171409218d2243f0818
> Author: Patrick McHardy <kaber@trash.net>
> Date: Mon Nov 9 20:33:14 2009 +0100
>
> net: allow to propagate errors through ->ndo_hard_start_xmit()
>
> Currently the ->ndo_hard_start_xmit() callbacks are only permitted to return
> one of the NETDEV_TX codes. This prevents any kind of error propagation for
> virtual devices, like queue congestion of the underlying device in case of
> layered devices, or unreachability in case of tunnels.
>
> This patches changes the NET_XMIT codes to avoid clashes with the NETDEV_TX
- This patches changes the NET_XMIT codes to avoid clashes with the NETDEV_TX
+ This patch changes the NET_XMIT codes to avoid clashes with the NETDEV_TX
> codes and changes the two callers of dev_hard_start_xmit() to expect either
> errno codes, NET_XMIT codes or NETDEV_TX codes as return value.
>
> In case of qdisc_restart(), all non NETDEV_TX codes are mapped to NETDEV_TX_OK
> since no error propagation is possible when using qdiscs. In case of
> dev_queue_xmit(), the error is propagated upwards.
>
> Signed-off-by: Patrick McHardy <kaber@trash.net>
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 465add6..ab2812c 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -63,27 +63,48 @@ struct wireless_dev;
> #define HAVE_FREE_NETDEV /* free_netdev() */
> #define HAVE_NETDEV_PRIV /* netdev_priv() */
>
> -#define NET_XMIT_SUCCESS 0
> -#define NET_XMIT_DROP 1 /* skb dropped */
> -#define NET_XMIT_CN 2 /* congestion notification */
> -#define NET_XMIT_POLICED 3 /* skb is shot by police */
> -#define NET_XMIT_MASK 0xFFFF /* qdisc flags in net/sch_generic.h */
> +/*
> + * Transmit return codes: transmit return codes originate from three different
> + * namespaces:
> + *
> + * - qdisc return codes
> + * - driver transmit return codes
> + * - errno values
> + *
> + * Drivers are allowed to return any one of those in their hard_start_xmit()
> + * function. Real network devices commonly used with qdiscs should only return
> + * the driver transmit return codes though - when qdiscs are used, the actual
> + * transmission happens asynchronously, so the value is not propagated to
> + * higher layers. Virtual network devices transmit synchronously, in this case
> + * the driver transmit return codes are consumed by dev_queue_xmit(), all
> + * others are propagated to higher layers.
> + */
> +
> +/* qdisc ->enqueue() return codes. */
> +#define NET_XMIT_SUCCESS 0x00
> +#define NET_XMIT_DROP 0x10 /* skb dropped */
> +#define NET_XMIT_CN 0x20 /* congestion notification */
> +#define NET_XMIT_POLICED 0x30 /* skb is shot by police */
> +#define NET_XMIT_MASK 0xf0 /* qdisc flags in net/sch_generic.h */
>
> /* Backlog congestion levels */
> -#define NET_RX_SUCCESS 0 /* keep 'em coming, baby */
> -#define NET_RX_DROP 1 /* packet dropped */
> +#define NET_RX_SUCCESS 0 /* keep 'em coming, baby */
> +#define NET_RX_DROP 1 /* packet dropped */
Should these NET_RX codes be mixed with transmit codes?
>
> /* NET_XMIT_CN is special. It does not guarantee that this packet is lost. It
> * indicates that the device will soon be dropping packets, or already drops
> * some packets of the same priority; prompting us to send less aggressively. */
> -#define net_xmit_eval(e) ((e) == NET_XMIT_CN? 0 : (e))
> +#define net_xmit_eval(e) ((e) == NET_XMIT_CN ? 0 : (e))
> #define net_xmit_errno(e) ((e) != NET_XMIT_CN ? -ENOBUFS : 0)
>
> /* Driver transmit return codes */
> +#define NETDEV_TX_MASK 0xf
> +
> enum netdev_tx {
> - NETDEV_TX_OK = 0, /* driver took care of packet */
> - NETDEV_TX_BUSY, /* driver tx path was busy*/
> - NETDEV_TX_LOCKED = -1, /* driver tx lock was already taken */
> + __NETDEV_TX_MIN = INT_MIN, /* make sure enum is signed */
> + NETDEV_TX_OK = 0, /* driver took care of packet */
> + NETDEV_TX_BUSY = 1, /* driver tx path was busy*/
> + NETDEV_TX_LOCKED = 2, /* driver tx lock was already taken */
> };
> typedef enum netdev_tx netdev_tx_t;
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index bf629ac..1f5752d 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1756,7 +1756,7 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
> struct netdev_queue *txq)
> {
> const struct net_device_ops *ops = dev->netdev_ops;
> - int rc;
> + int rc = NETDEV_TX_OK;
Isn't it enough to add this in one place only: before this
"goto out_kfree_skb"?
>
> if (likely(!skb->next)) {
> if (!list_empty(&ptype_all))
> @@ -1804,6 +1804,8 @@ gso:
> nskb->next = NULL;
> rc = ops->ndo_start_xmit(nskb, dev);
> if (unlikely(rc != NETDEV_TX_OK)) {
> + if (rc & ~NETDEV_TX_MASK)
> + goto out_kfree_gso_skb;
If e.g. (rc == NETDEV_TX_OK | NET_XMIT_CN), why exactly is this freeing
necessary now?
Is e.g. (rc == NETDEV_TX_BUSY | NET_XMIT_CN) legal? If so, there would
be use after kfree, I guess. Otherwise, it should be documented above
(and maybe checked somewhere as well).
> nskb->next = skb->next;
> skb->next = nskb;
> return rc;
> @@ -1813,11 +1815,14 @@ gso:
> return NETDEV_TX_BUSY;
> } while (skb->next);
>
> - skb->destructor = DEV_GSO_CB(skb)->destructor;
> + rc = NETDEV_TX_OK;
When is (rc != NETDEV_TX_OK) possible in this place?
>
> +out_kfree_gso_skb:
> + if (likely(skb->next == NULL))
> + skb->destructor = DEV_GSO_CB(skb)->destructor;
> out_kfree_skb:
> kfree_skb(skb);
> - return NETDEV_TX_OK;
> + return rc;
> }
>
> static u32 skb_tx_hashrnd;
> @@ -1905,6 +1910,23 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
> return rc;
> }
>
> +static inline bool dev_xmit_complete(int rc)
> +{
> + /* successful transmission */
> + if (rc == NETDEV_TX_OK)
> + return true;
> +
> + /* error while transmitting, driver consumed skb */
> + if (rc < 0)
> + return true;
> +
> + /* error while queueing to a different device, driver consumed skb */
> + if (rc & NET_XMIT_MASK)
> + return true;
> +
> + return false;
> +}
> +
> /**
> * dev_queue_xmit - transmit a buffer
> * @skb: buffer to transmit
> @@ -2002,8 +2024,8 @@ gso:
> HARD_TX_LOCK(dev, txq, cpu);
>
> if (!netif_tx_queue_stopped(txq)) {
> - rc = NET_XMIT_SUCCESS;
> - if (!dev_hard_start_xmit(skb, dev, txq)) {
> + rc = dev_hard_start_xmit(skb, dev, txq);
> + if (dev_xmit_complete(rc)) {
> HARD_TX_UNLOCK(dev, txq);
> goto out;
> }
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index 4ae6aa5..b13821a 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -120,8 +120,15 @@ int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
>
> HARD_TX_LOCK(dev, txq, smp_processor_id());
> if (!netif_tx_queue_stopped(txq) &&
> - !netif_tx_queue_frozen(txq))
> + !netif_tx_queue_frozen(txq)) {
> ret = dev_hard_start_xmit(skb, dev, txq);
> +
> + /* an error implies that the skb was consumed */
> + if (ret < 0)
> + ret = NETDEV_TX_OK;
+ else (?)
Jarek P.
> + /* all NET_XMIT codes map to NETDEV_TX_OK */
> + ret &= ~NET_XMIT_MASK;
> + }
> HARD_TX_UNLOCK(dev, txq);
>
> spin_lock(root_lock);
^ permalink raw reply
* Re: next tree for October 27: znet build failure
From: Randy Dunlap @ 2009-11-10 17:19 UTC (permalink / raw)
To: Stephen Rothwell
Cc: Sachin Sant, linux-next, netdev, David Miller, John W. Linville,
Greg KH
In-Reply-To: <20091028013721.049c0146.sfr@canb.auug.org.au>
On Wed, 28 Oct 2009 01:37:21 +1100 Stephen Rothwell wrote:
> Just adding John and Greg to the cc list as this is caused by patches in
> the staging tree.
ping ping. still a problem.
> On Tue, 27 Oct 2009 15:05:03 +0530 Sachin Sant <sachinp@in.ibm.com> wrote:
> >
> > Fails to build on x86_32 with lots of errors related to znet
> >
> > drivers/net/znet.c:107:29: error: wireless/i82593.h: No such file or directory
> > drivers/net/znet.c:133: error: field 'i593_init' has incomplete type
> > drivers/net/znet.c: In function 'znet_set_multicast_list':
> > drivers/net/znet.c:235: error: invalid application of 'sizeof' to incomplete type 'struct i82593_conf_block'
> > drivers/net/znet.c:247: error: dereferencing pointer to incomplete type
> > .....
> >
> > Commit 879e9304... moved wireless/i82593.h to drivers/staging/wavelan/i82593.h
> >
> > Thanks
> > -Sachin
---
~Randy
^ permalink raw reply
* [PATCH] netdev: fold name hash properly (v3)
From: Stephen Hemminger @ 2009-11-10 17:20 UTC (permalink / raw)
To: David Miller; +Cc: netdev
The full_name_hash function does not produce well distributed values in
the lower bits, so most code uses hash_32() to fold it. This is really
a bug introduced when name hashing was added, back in 2.5 when I added
name hashing.
hash_32 is all that is needed since full_name_hash returns unsigned int
which is only 32 bits on 64 bit platforms.
Also, there is no point in using hash_32 on ifindex, because the is naturally
sequential and usually well distributed.
Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
---
Patch against net-next-2.6
--- a/net/core/dev.c 2009-11-09 22:19:08.731481514 -0800
+++ b/net/core/dev.c 2009-11-10 08:42:23.755003727 -0800
@@ -79,6 +79,7 @@
#include <linux/cpu.h>
#include <linux/types.h>
#include <linux/kernel.h>
+#include <linux/hash.h>
#include <linux/sched.h>
#include <linux/mutex.h>
#include <linux/string.h>
@@ -196,7 +197,7 @@ EXPORT_SYMBOL(dev_base_lock);
static inline struct hlist_head *dev_name_hash(struct net *net, const char *name)
{
unsigned hash = full_name_hash(name, strnlen(name, IFNAMSIZ));
- return &net->dev_name_head[hash & (NETDEV_HASHENTRIES - 1)];
+ return &net->dev_name_head[hash_32(hash, NETDEV_HASHBITS)];
}
static inline struct hlist_head *dev_index_hash(struct net *net, int ifindex)
^ permalink raw reply
* DM9000: Wake on LAN support
From: Ben Dooks @ 2009-11-10 17:22 UTC (permalink / raw)
To: netdev; +Cc: davem, Simtec Linux Team
[-- Attachment #1: simtec/ready/dm9000-wakeonlan.patch --]
[-- Type: text/plain, Size: 7418 bytes --]
Add support for Wake on LAN (WOL) reception and waking the device up from
this signal via the ethtool interface. Currently we are only supporting
the magic-packet variant of wakeup.
WOL is enabled by specifying a second interrupt resource to the driver
which indicates where the interrupt for the WOL is being signalled. This
then enables the necessary ethtool calls to leave the device in a state
to receive WOL frames when going into suspend.
Signed-off-by: Ben Dooks <ben@simtec.co.uk>
Signed-off-by: Simtec Linux Team <linux@simtec.co.uk>
---
drivers/net/dm9000.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++++---
drivers/net/dm9000.h | 7 ++
2 files changed, 142 insertions(+), 8 deletions(-)
Index: b/drivers/net/dm9000.c
===================================================================
--- a/drivers/net/dm9000.c 2009-11-10 10:22:37.000000000 +0000
+++ b/drivers/net/dm9000.c 2009-11-10 10:26:21.000000000 +0000
@@ -100,6 +100,7 @@ typedef struct board_info {
unsigned int flags;
unsigned int in_suspend :1;
+ unsigned int wake_supported :1;
int debug_level;
enum dm9000_type type;
@@ -116,6 +117,8 @@ typedef struct board_info {
struct resource *data_req;
struct resource *irq_res;
+ int irq_wake;
+
struct mutex addr_lock; /* phy and eeprom access lock */
struct delayed_work phy_poll;
@@ -125,6 +128,7 @@ typedef struct board_info {
struct mii_if_info mii;
u32 msg_enable;
+ u32 wake_state;
int rx_csum;
int can_csum;
@@ -568,6 +572,54 @@ static int dm9000_set_eeprom(struct net_
return 0;
}
+static void dm9000_get_wol(struct net_device *dev, struct ethtool_wolinfo *w)
+{
+ board_info_t *dm = to_dm9000_board(dev);
+
+ memset(w, 0, sizeof(struct ethtool_wolinfo));
+
+ /* note, we could probably support wake-phy too */
+ w->supported = dm->wake_supported ? WAKE_MAGIC : 0;
+ w->wolopts = dm->wake_state;
+}
+
+static int dm9000_set_wol(struct net_device *dev, struct ethtool_wolinfo *w)
+{
+ board_info_t *dm = to_dm9000_board(dev);
+ unsigned long flags;
+ u32 opts = w->wolopts;
+ u32 wcr = 0;
+
+ if (!dm->wake_supported)
+ return -EOPNOTSUPP;
+
+ if (opts & ~WAKE_MAGIC)
+ return -EINVAL;
+
+ if (opts & WAKE_MAGIC)
+ wcr |= WCR_MAGICEN;
+
+ mutex_lock(&dm->addr_lock);
+
+ spin_lock_irqsave(&dm->lock, flags);
+ iow(dm, DM9000_WCR, wcr);
+ spin_unlock_irqrestore(&dm->lock, flags);
+
+ mutex_unlock(&dm->addr_lock);
+
+ if (dm->wake_state != opts) {
+ /* change in wol state, update IRQ state */
+
+ if (!dm->wake_state)
+ set_irq_wake(dm->irq_wake, 1);
+ else if (dm->wake_state & !opts)
+ set_irq_wake(dm->irq_wake, 0);
+ }
+
+ dm->wake_state = opts;
+ return 0;
+}
+
static const struct ethtool_ops dm9000_ethtool_ops = {
.get_drvinfo = dm9000_get_drvinfo,
.get_settings = dm9000_get_settings,
@@ -576,6 +628,8 @@ static const struct ethtool_ops dm9000_e
.set_msglevel = dm9000_set_msglevel,
.nway_reset = dm9000_nway_reset,
.get_link = dm9000_get_link,
+ .get_wol = dm9000_get_wol,
+ .set_wol = dm9000_set_wol,
.get_eeprom_len = dm9000_get_eeprom_len,
.get_eeprom = dm9000_get_eeprom,
.set_eeprom = dm9000_set_eeprom,
@@ -722,6 +776,7 @@ dm9000_init_dm9000(struct net_device *de
{
board_info_t *db = netdev_priv(dev);
unsigned int imr;
+ unsigned int ncr;
dm9000_dbg(db, 1, "entering %s\n", __func__);
@@ -736,8 +791,15 @@ dm9000_init_dm9000(struct net_device *de
iow(db, DM9000_GPCR, GPCR_GEP_CNTL); /* Let GPIO0 output */
iow(db, DM9000_GPR, 0); /* Enable PHY */
- if (db->flags & DM9000_PLATF_EXT_PHY)
- iow(db, DM9000_NCR, NCR_EXT_PHY);
+ ncr = (db->flags & DM9000_PLATF_EXT_PHY) ? NCR_EXT_PHY : 0;
+
+ /* if wol is needed, then always set NCR_WAKEEN otherwise we end
+ * up dumping the wake events if we disable this. There is already
+ * a wake-mask in DM9000_WCR */
+ if (db->wake_supported)
+ ncr |= NCR_WAKEEN;
+
+ iow(db, DM9000_NCR, ncr);
/* Program operating register */
iow(db, DM9000_TCR, 0); /* TX Polling clear */
@@ -1045,6 +1107,41 @@ static irqreturn_t dm9000_interrupt(int
return IRQ_HANDLED;
}
+static irqreturn_t dm9000_wol_interrupt(int irq, void *dev_id)
+{
+ struct net_device *dev = dev_id;
+ board_info_t *db = netdev_priv(dev);
+ unsigned long flags;
+ unsigned nsr, wcr;
+
+ spin_lock_irqsave(&db->lock, flags);
+
+ nsr = ior(db, DM9000_NSR);
+ wcr = ior(db, DM9000_WCR);
+
+ dev_dbg(db->dev, "%s: NSR=0x%02x, WCR=0x%02x\n", __func__, nsr, wcr);
+
+ if (nsr & NSR_WAKEST) {
+ /* clear, so we can avoid */
+ iow(db, DM9000_NSR, NSR_WAKEST);
+
+ if (wcr & WCR_LINKST)
+ dev_info(db->dev, "wake by link status change\n");
+ if (wcr & WCR_SAMPLEST)
+ dev_info(db->dev, "wake by sample packet\n");
+ if (wcr & WCR_MAGICST )
+ dev_info(db->dev, "wake by magic packet\n");
+ if (!(wcr & (WCR_LINKST | WCR_SAMPLEST | WCR_MAGICST)))
+ dev_err(db->dev, "wake signalled with no reason? "
+ "NSR=0x%02x, WSR=0x%02x\n", nsr, wcr);
+
+ }
+
+ spin_unlock_irqrestore(&db->lock, flags);
+
+ return (nsr & NSR_WAKEST) ? IRQ_HANDLED : IRQ_NONE;
+}
+
#ifdef CONFIG_NET_POLL_CONTROLLER
/*
*Used by netconsole
@@ -1299,6 +1396,29 @@ dm9000_probe(struct platform_device *pde
goto out;
}
+ db->irq_wake = platform_get_irq(pdev, 1);
+ if (db->irq_wake >= 0) {
+ dev_dbg(db->dev, "wakeup irq %d\n", db->irq_wake);
+
+ ret = request_irq(db->irq_wake, dm9000_wol_interrupt,
+ IRQF_SHARED, dev_name(db->dev), ndev);
+ if (ret) {
+ dev_err(db->dev, "cannot get wakeup irq (%d)\n", ret);
+ } else {
+
+ /* test to see if irq is really wakeup capable */
+ ret = set_irq_wake(db->irq_wake, 1);
+ if (ret) {
+ dev_err(db->dev, "irq %d cannot set wakeup (%d)\n",
+ db->irq_wake, ret);
+ ret = 0;
+ } else {
+ set_irq_wake(db->irq_wake, 0);
+ db->wake_supported = 1;
+ }
+ }
+ }
+
iosize = resource_size(db->addr_res);
db->addr_req = request_mem_region(db->addr_res->start, iosize,
pdev->name);
@@ -1490,10 +1610,14 @@ dm9000_drv_suspend(struct device *dev)
db = netdev_priv(ndev);
db->in_suspend = 1;
- if (netif_running(ndev)) {
- netif_device_detach(ndev);
+ if (!netif_running(ndev))
+ return 0;
+
+ netif_device_detach(ndev);
+
+ /* only shutdown if not using WoL */
+ if (!db->wake_state)
dm9000_shutdown(ndev);
- }
}
return 0;
}
@@ -1506,10 +1630,13 @@ dm9000_drv_resume(struct device *dev)
board_info_t *db = netdev_priv(ndev);
if (ndev) {
-
if (netif_running(ndev)) {
- dm9000_reset(db);
- dm9000_init_dm9000(ndev);
+ /* reset if we were not in wake mode to ensure if
+ * the device was powered off it is in a known state */
+ if (!db->wake_state) {
+ dm9000_reset(db);
+ dm9000_init_dm9000(ndev);
+ }
netif_device_attach(ndev);
}
Index: b/drivers/net/dm9000.h
===================================================================
--- a/drivers/net/dm9000.h 2009-11-10 10:22:37.000000000 +0000
+++ b/drivers/net/dm9000.h 2009-11-10 10:25:44.000000000 +0000
@@ -111,6 +111,13 @@
#define RSR_CE (1<<1)
#define RSR_FOE (1<<0)
+#define WCR_LINKEN (1 << 5)
+#define WCR_SAMPLEEN (1 << 4)
+#define WCR_MAGICEN (1 << 3)
+#define WCR_LINKST (1 << 2)
+#define WCR_SAMPLEST (1 << 1)
+#define WCR_MAGICST (1 << 0)
+
#define FCTR_HWOT(ot) (( ot & 0xf ) << 4 )
#define FCTR_LWOT(ot) ( ot & 0xf )
--
Ben (ben@fluff.org, http://www.fluff.org/)
'a smiley only costs 4 bytes'
^ permalink raw reply
* Re: PATCH: Network Device Naming mechanism and policy
From: Stephen Hemminger @ 2009-11-10 17:23 UTC (permalink / raw)
To: Narendra_K
Cc: md, Matt_Domsch, bryan, dannf, bhutchings, netdev, linux-hotplug,
Jordan_Hargrave, Charles_Rose, Sandeep_K_Shandilya
In-Reply-To: <EDA0A4495861324DA2618B4C45DCB3EE589727@blrx3m08.blr.amer.dell.com>
On Mon, 9 Nov 2009 20:11:47 +0530
<Narendra_K@Dell.com> wrote:
>
> >> > As a distribution developer I highly value solutions like
> >this which
> >> > do not require patching every application which deals with
> >interface
> >> > names and then teaching users about aliases which only
> >work in some
> >> > places and are unknown to the kernel.
> >> Fair enough - but would you object if we changed the naming scheme
> >> from eth%d to something else?
> >I suppose that this would depend on what else. :-) Since you
> >want radical changes I recommend that you design the new
> >persistent naming infrastructure in a way that will allow root
> >to choose to use the classic naming scheme, or many users will
> >scream a lot and at least some distributions will do it anyway.
> >I also expect that providing choice at the beginning of
> >development may lead to more acceptance later if and when the
> >new scheme will have proved itself to be superior (at least in
> >some situations).
> >You have tought about this for a long time and if so far you
> >have not found a solution which is widely considered superior
> >then I doubt that one will appear soon. Providing your
> >favourite naming scheme as an optional add on will immediately
> >benefit those who like it and greatly reduce opposition from
> >those who do not.
>
> In that way, I suppose char device node solution fits the scheme
> perfectly. It doesn't change or interfere with the kernel's default
> naming scheme (ethN) in any way. Also, the applications continue to work
> the way they did and in addition to supporting traditional names, they
> would also support pathnames. Whether all the user space applications
> need to be patched can be discussed and debated. But, we can patch
> applications like, installers and firewall code, which when don't see
> determinism ("eth0 mapping to integrated port 1"), fail and cause very
> high impact could be patched. Since users are already familiar with
> pathnames like /dev/disk/by-id{label, uuid}, I suppose it might not be
> very difficult to get used to pathnames like
> /dev/netdev/by-chassis-label/Embedded_NIC_1. Would that be acceptable ?
>
IFNAMSIZ = 16 is hardwired as part of the kernel binary user space API.
Have you observed that the only developers arguing for this come from
outside the normal circle of networking? It seems to be favored only
by those who come to networking from a system or disk point of view.
--
^ permalink raw reply
* Re: RFC: net: allow to propagate errors through ->ndo_hard_start_xmit()
From: Patrick McHardy @ 2009-11-10 17:31 UTC (permalink / raw)
To: Jarek Poplawski
Cc: Linux Netdev List, Herbert Xu, David S. Miller, Stephen Hemminger
In-Reply-To: <20091110170838.GA4195@ami.dom.local>
Jarek Poplawski wrote:
> On Mon, Nov 09, 2009 at 08:41:36PM +0100, Patrick McHardy wrote:
>> I've updated my patch to propagate error values (errno and NET_XMIT
>> codes) through ndo_hard_start_xmit() and incorporated the suggestions
>> made last time, namely:
>>
>> - move slightly complicated return value check to inline function and
>> add a few comments
>>
>> - fix error handling while in the middle of transmitting GSO skbs
>>
>> I've also audited the tree once again for invalid return values and
>> found a single remaining instance in a Wimax driver, I'll take care
>> of that later.
>>
>> Two questions remain:
>>
>> - I'm not sure the error handling in dev_hard_start_xmit() for GSO
>> skbs is optimal. When the driver returns an error, it is assumed
>> the current segment has been freed. The patch then frees the
>> entire GSO skb, including all remaining segments. Alternatively
>> it could try to transmit the remaining segments later.
>
> Anyway, it seems this freeing should be described in the changelog,
> if not moved to a separate patch, since it fixes another problem,
> unless I forgot something.
What other problem are you refering to? I'm not aware of any
problems in the existing function.
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index bf629ac..1f5752d 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -1756,7 +1756,7 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
>> struct netdev_queue *txq)
>> {
>> const struct net_device_ops *ops = dev->netdev_ops;
>> - int rc;
>> + int rc = NETDEV_TX_OK;
>
> Isn't it enough to add this in one place only: before this
> "goto out_kfree_skb"?
Its only exists once in the version I sent out earlier.
>> if (likely(!skb->next)) {
>> if (!list_empty(&ptype_all))
>> @@ -1804,6 +1804,8 @@ gso:
>> nskb->next = NULL;
>> rc = ops->ndo_start_xmit(nskb, dev);
>> if (unlikely(rc != NETDEV_TX_OK)) {
>> + if (rc & ~NETDEV_TX_MASK)
>> + goto out_kfree_gso_skb;
>
> If e.g. (rc == NETDEV_TX_OK | NET_XMIT_CN), why exactly is this freeing
> necessary now?
>
> Is e.g. (rc == NETDEV_TX_BUSY | NET_XMIT_CN) legal? If so, there would
> be use after kfree, I guess. Otherwise, it should be documented above
> (and maybe checked somewhere as well).
NET_XMIT_CN is a valid return value, yes. But its not freeing the
transmitted segment but the remaining ones. Its not strictly
necessary, but its the easiest way to treat all errors similar.
Otherwise you get complicated cases, f.i. when the driver returns
NET_XMIT_CN for the first segment and NETDEV_TX_OK for the
remaining ones.
>> nskb->next = skb->next;
>> skb->next = nskb;
>> return rc;
>> @@ -1813,11 +1815,14 @@ gso:
>> return NETDEV_TX_BUSY;
>> } while (skb->next);
>>
>> - skb->destructor = DEV_GSO_CB(skb)->destructor;
>> + rc = NETDEV_TX_OK;
>
> When is (rc != NETDEV_TX_OK) possible in this place?
Its gone in the current version.
>> +out_kfree_gso_skb:
>> + if (likely(skb->next == NULL))
>> + skb->destructor = DEV_GSO_CB(skb)->destructor;
>> out_kfree_skb:
>> kfree_skb(skb);
>> - return NETDEV_TX_OK;
>> + return rc;
>> }
>> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
>> index 4ae6aa5..b13821a 100644
>> --- a/net/sched/sch_generic.c
>> +++ b/net/sched/sch_generic.c
>> @@ -120,8 +120,15 @@ int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
>>
>> HARD_TX_LOCK(dev, txq, smp_processor_id());
>> if (!netif_tx_queue_stopped(txq) &&
>> - !netif_tx_queue_frozen(txq))
>> + !netif_tx_queue_frozen(txq)) {
>> ret = dev_hard_start_xmit(skb, dev, txq);
>> +
>> + /* an error implies that the skb was consumed */
>> + if (ret < 0)
>> + ret = NETDEV_TX_OK;
>
> + else (?)
Another branch vs. an unconditional and operation. I chose the later.
>> + /* all NET_XMIT codes map to NETDEV_TX_OK */
>> + ret &= ~NET_XMIT_MASK;
>> + }
>> HARD_TX_UNLOCK(dev, txq);
>>
>> spin_lock(root_lock);
^ permalink raw reply
* Re: [RFC PATCH] net: add dataref destructor to sk_buff
From: Michael S. Tsirkin @ 2009-11-10 17:36 UTC (permalink / raw)
To: Gregory Haskins; +Cc: alacrityvm-devel, herbert.xu, linux-kernel, netdev
In-Reply-To: <4AF98A8C.9040201@novell.com>
On Tue, Nov 10, 2009 at 10:45:16AM -0500, Gregory Haskins wrote:
> Michael S. Tsirkin wrote:
> > On Tue, Nov 10, 2009 at 09:11:10AM -0500, Gregory Haskins wrote:
> >> Michael S. Tsirkin wrote:
> >>> On Tue, Nov 10, 2009 at 05:40:50AM -0700, Gregory Haskins wrote:
> >>>>>>> On 11/10/2009 at 6:53 AM, in message <20091110115335.GC6989@redhat.com>,
> >>>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >>
> >>>>> Last time this was tried, this is the objection that was voiced:
> >>>>>
> >>>>> The problem with this patch is that it's tracking skb's, while
> >>>>> you want use it to track pages for zero-copy. That just doesn't
> >>>>> work. Through mechanisms like splice, individual pages in the
> >>>>> skb can be detached and metastasize to other locations, e.g.,
> >>>>> the VFS.
> >>>> Right, and I don't think this applies here because I specifically chose the shinfo level to try to properly
> >>>> track the page level avoid this issue. Multiple skb's can point to a single shinfo, iiuc.
> >>> VFS does not know about shinfo either, does it?
> >> I do not follow the reference. Where does VFS come into play?
> >
> > "Through mechanisms like splice, individual pages in the
> > skb can be detached and metastasize to other locations, e.g.,
> > the VFS"
>
> Right, understood. What I mean is: How is that actually used in
> real-life in a way that is valid?
>
> What I am getting at is as follows: From a real basic perspective, you
> can look at all of this as a simple synchronous call (i.e. sendmsg()).
> The "app" (be it a userspace app, or a guest) prepares a buffer for
> transmission, and offers it to the next layer in the stack. The app
> must maintain the integrity of that buffer at least until the layer
> below it signifies that it is "consumed". This may mean its a
> synchronous call, like sendmsg(), or it may be asynchronous, like AIO.
>
> But the key thing here is that at some point, the lower layer has to
> signify that the buffer stability constraint has been met. In either
> case, we have a clear delineated event: the io-completes = the buffer is
> free to be reused.
>
> In the simple case, the buffer in question is copied to a kernel buffer,
> and the io completes immediately. In other cases (such as zero copy),
> the buffer is mapped into the skb, and we have to wait for even lower
> layers to signify the completion.
>
> I am not a stack expert, but I was under the impression that we use this
> model for userspace pages today as well using the wmem callbacks in
> skb->destructor(). If so, I do not see how you could do something like
> detach a page from a pskb and still expect to have a proper event that
> delineates the io-completion to the higher layers.
I think linux only cares about that for accounting purposes (stuff like
socket sndbuff size). If someone takes over the page, the socket can
stop worrying about it.
> So the questions are:
>
> 1) do we in fact map userspace pages to pskbs today?
I don't think so.
> 2a) if so, how do we delineate the completion event?
> 2b) and how do we prevent worrying about the get_page() issue you refer
> to.
>
>
> >>
> >>>>> In other words, this only *seems*
> >>>>> to work for you because you are not trying to do things like
> >>>>> guest to host communication, with host doing smart things.
> >>>> I am not following what you mean here, as I do use this for guest->host and guest->host->remote, and
> >>>> it works quite nicely. I map the guest pages in, and when the last reference to the pages are dropped,
> >>>> I release the pages back to the guest. It doesn't matter if the skb egresses out a physical adapter or is
> >>>> received locally. All that matters is the lifetime of the shinfo (and thus its pages) is handled correctly.
> >>> Not if someone else is referencing the pages without a reference to shinfo.
> >> I agree that if we can reference pages outside of the skb/shinfo then
> >> there is a problem. I wasn't aware that we could do this, tbh.
> >>
> >> However, it seems to me that this is a problem with the overall stack,
> >> if true....isn't it? For instance, if I do a sendmsg() from a userspace
> >> app and block until its consumed,
> >
> > consumed == memcpy_from_iovec?
>
> For non-zero-copy, sure why not.
>
> >
> >> how can the system function sanely if
> >> the app returns from the call but something is still referencing the
> >> page(s)?
> >
> > which pages?
>
> You said that there are paths that get_page() out of shinfo without
> holding a shinfo reference.
Without zero copy, application does not care about these,
they have been allocated by kernel.
--
MST
^ permalink raw reply
* Re: [PATCH] netdev: fold name hash properly (v3)
From: Eric Dumazet @ 2009-11-10 17:37 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: David Miller, netdev
In-Reply-To: <20091110092034.3d4ee9b0@nehalam>
Stephen Hemminger a écrit :
> The full_name_hash function does not produce well distributed values in
> the lower bits, so most code uses hash_32() to fold it. This is really
> a bug introduced when name hashing was added, back in 2.5 when I added
> name hashing.
>
> hash_32 is all that is needed since full_name_hash returns unsigned int
> which is only 32 bits on 64 bit platforms.
>
> Also, there is no point in using hash_32 on ifindex, because the is naturally
> sequential and usually well distributed.
>
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Thanks Stephen
^ 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