* Re: [GENETLINK]: Question: global lock (genl_mutex) possible refinement?
From: Richard MUSIL @ 2007-07-23 16:45 UTC (permalink / raw)
To: Thomas Graf; +Cc: Patrick McHardy, netdev
In-Reply-To: <20070723102909.GA9285@postel.suug.ch>
Thomas Graf wrote:
> Actually there is no reason to not use separate locks for the
> message serialization and the protection of the list of registered
> families. There is only one lock simply for the reason that I've
> never thought of anybody could think of registering a new genetlink
> family while processing a message.
Thomas,
I have been giving it a second thought and came up with something more
complex. The idea is to have locking granularity at the level of
individual families.
Message processing will lock only the family it currently processes
*and* global messaging lock, while family management routines will take
global family lock, and particular lock for family.
This should allow running registration/dereg. from message handler as
long as the family involved is not the same family in which context the
message is being processed.
On the other hand, using lock per family, ensures that message handlers
are not (accidentally) called with invalid references. This should not
be so much problem for dumpit handler, but doit uses for example family
attrs. So I added another patch which implements above described
functionality below.
> Alternatively you could also postpone the registration of the new
> genetlink family to a workqueue.
To be honest, I cannot judge whether the additional complexity I propose
outweighs the gains. In my book, it definitely does, since I like the
easiness of doit, dumpit handlers and implementation using those is
pretty straightforward.
In the long term I believe that refining the locking could also help
in situations where there is heavy traffic over genetlink, then
all family manipulations will not be blocked (which is currently the case).
Let me know, if you accept it as a patch, or should I eventually go for
plan B ;).
--
Richard
>From 63b3ee722402533aed6e137347e41ab1a1fa1127 Mon Sep 17 00:00:00 2001
From: Richard Musil <richard.musil@st.com>
Date: Mon, 23 Jul 2007 15:12:09 +0200
Subject: [PATCH] Added private mutex for each genetlink family (struct genl_family).
This mutex is used to synchronize access to particular family between message
processing handlers and management routines for families
(registering/unregistering families/ops).
This should ensure that another family can be registered or unregistered from
inside genetlink message handler. Trying to register or unregister family
from its own handler will still cause deadlock.
---
include/net/genetlink.h | 1 +
net/netlink/genetlink.c | 98 +++++++++++++++++++++++++++++++++--------------
2 files changed, 70 insertions(+), 29 deletions(-)
diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index b6eaca1..681ad13 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -25,6 +25,7 @@ struct genl_family
struct nlattr ** attrbuf; /* private */
struct list_head ops_list; /* private */
struct list_head family_list; /* private */
+ struct mutex lock; /* private */
};
/**
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 5ee18eb..0104267 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -40,16 +40,30 @@ static void genl_unlock(void)
static DEFINE_MUTEX(genl_fam_mutex); /* serialization for family list management */
-static inline void genl_fam_lock(void)
+static inline void genl_fam_lock(struct genl_family *family)
{
mutex_lock(&genl_fam_mutex);
+ if (family)
+ mutex_lock(&family->lock);
}
-static inline genl_fam_unlock(void)
+static inline void genl_fam_unlock(struct genl_family *family)
{
+ if (family)
+ mutex_unlock(&family->lock);
mutex_unlock(&genl_fam_mutex);
}
+static inline void genl_onefam_lock(struct genl_family *family)
+{
+ mutex_lock(&family->lock);
+}
+
+static inline void genl_onefam_unlock(struct genl_family *family)
+{
+ mutex_unlock(&family->lock);
+}
+
#define GENL_FAM_TAB_SIZE 16
#define GENL_FAM_TAB_MASK (GENL_FAM_TAB_SIZE - 1)
@@ -162,9 +176,9 @@ int genl_register_ops(struct genl_family *family, struct genl_ops *ops)
if (ops->policy)
ops->flags |= GENL_CMD_CAP_HASPOL;
- genl_fam_lock();
+ genl_fam_lock(family);
list_add_tail(&ops->ops_list, &family->ops_list);
- genl_fam_unlock();
+ genl_fam_unlock(family);
genl_ctrl_event(CTRL_CMD_NEWOPS, ops);
err = 0;
@@ -192,16 +206,16 @@ int genl_unregister_ops(struct genl_family *family, struct genl_ops *ops)
{
struct genl_ops *rc;
- genl_fam_lock();
+ genl_fam_lock(family);
list_for_each_entry(rc, &family->ops_list, ops_list) {
if (rc == ops) {
list_del(&ops->ops_list);
- genl_fam_unlock();
+ genl_fam_unlock(family);
genl_ctrl_event(CTRL_CMD_DELOPS, ops);
return 0;
}
}
- genl_fam_unlock();
+ genl_fam_unlock(family);
return -ENOENT;
}
@@ -228,8 +242,9 @@ int genl_register_family(struct genl_family *family)
goto errout;
INIT_LIST_HEAD(&family->ops_list);
+ mutex_init(&family->lock);
- genl_fam_lock();
+ genl_fam_lock(family);
if (genl_family_find_byname(family->name)) {
err = -EEXIST;
@@ -263,14 +278,14 @@ int genl_register_family(struct genl_family *family)
family->attrbuf = NULL;
list_add_tail(&family->family_list, genl_family_chain(family->id));
- genl_fam_unlock();
+ genl_fam_unlock(family);
genl_ctrl_event(CTRL_CMD_NEWFAMILY, family);
return 0;
errout_locked:
- genl_fam_unlock();
+ genl_fam_unlock(family);
errout:
return err;
}
@@ -287,7 +302,7 @@ int genl_unregister_family(struct genl_family *family)
{
struct genl_family *rc;
- genl_fam_lock();
+ genl_fam_lock(family);
list_for_each_entry(rc, genl_family_chain(family->id), family_list) {
if (family->id != rc->id || strcmp(rc->name, family->name))
@@ -295,14 +310,16 @@ int genl_unregister_family(struct genl_family *family)
list_del(&rc->family_list);
INIT_LIST_HEAD(&family->ops_list);
- genl_fam_unlock();
+
+ genl_fam_unlock(family);
+ mutex_destroy(&family->lock);
kfree(family->attrbuf);
genl_ctrl_event(CTRL_CMD_DELFAMILY, family);
return 0;
}
- genl_fam_unlock();
+ genl_fam_unlock(family);
return -ENOENT;
}
@@ -315,38 +332,57 @@ static int genl_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
struct genlmsghdr *hdr = nlmsg_data(nlh);
int hdrlen, err;
+ genl_fam_lock(NULL);
family = genl_family_find_byid(nlh->nlmsg_type);
- if (family == NULL)
+ if (family == NULL) {
+ genl_fam_unlock(NULL);
return -ENOENT;
+ }
+
+ /* get particular family lock, but release global family lock
+ * so registering operations for other families are possible */
+ genl_onefam_lock(family);
+ genl_fam_unlock(NULL);
hdrlen = GENL_HDRLEN + family->hdrsize;
- if (nlh->nlmsg_len < nlmsg_msg_size(hdrlen))
- return -EINVAL;
+ if (nlh->nlmsg_len < nlmsg_msg_size(hdrlen)) {
+ err = -EINVAL;
+ goto unlock_out;
+ }
ops = genl_get_cmd(hdr->cmd, family);
- if (ops == NULL)
- return -EOPNOTSUPP;
+ if (ops == NULL) {
+ err = -EOPNOTSUPP;
+ goto unlock_out;
+ }
if ((ops->flags & GENL_ADMIN_PERM) &&
- security_netlink_recv(skb, CAP_NET_ADMIN))
- return -EPERM;
+ security_netlink_recv(skb, CAP_NET_ADMIN)) {
+ err = -EPERM;
+ goto unlock_out;
+ }
if (nlh->nlmsg_flags & NLM_F_DUMP) {
- if (ops->dumpit == NULL)
- return -EOPNOTSUPP;
+ if (ops->dumpit == NULL) {
+ err = -EOPNOTSUPP;
+ goto unlock_out;
+ }
- return netlink_dump_start(genl_sock, skb, nlh,
+ err = netlink_dump_start(genl_sock, skb, nlh,
ops->dumpit, ops->done);
+ goto unlock_out;
}
- if (ops->doit == NULL)
- return -EOPNOTSUPP;
+ if (ops->doit == NULL) {
+ err = -EOPNOTSUPP;
+ goto unlock_out;
+ }
if (family->attrbuf) {
err = nlmsg_parse(nlh, hdrlen, family->attrbuf, family->maxattr,
ops->policy);
if (err < 0)
- return err;
+ goto unlock_out;
}
info.snd_seq = nlh->nlmsg_seq;
@@ -356,7 +392,11 @@ static int genl_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
info.userhdr = nlmsg_data(nlh) + GENL_HDRLEN;
info.attrs = family->attrbuf;
- return ops->doit(skb, &info);
+ err = ops->doit(skb, &info);
+
+unlock_out:
+ genl_onefam_unlock(family);
+ return err;
}
static void genl_rcv(struct sock *sk, int len)
@@ -437,7 +477,7 @@ static int ctrl_dumpfamily(struct sk_buff *skb, struct netlink_callback *cb)
int fams_to_skip = cb->args[1];
if (chains_to_skip != 0)
- genl_fam_lock();
+ genl_fam_lock(NULL);
for (i = 0; i < GENL_FAM_TAB_SIZE; i++) {
if (i < chains_to_skip)
@@ -457,7 +497,7 @@ static int ctrl_dumpfamily(struct sk_buff *skb, struct netlink_callback *cb)
errout:
if (chains_to_skip != 0)
- genl_fam_unlock();
+ genl_fam_unlock(NULL);
cb->args[0] = i;
cb->args[1] = n;
--
1.5.1.6
^ permalink raw reply related
* [PATCH net-2.6.22-rc7] xfrm state selection update to use inner addresses
From: Joakim Koskela @ 2007-07-23 16:19 UTC (permalink / raw)
To: netdev; +Cc: Patrick McHardy, David Miller
In-Reply-To: <469FBECB.6080204@trash.net>
This patch modifies the xfrm state selection logic to use the inner
addresses where the outer have been (incorrectly) used. This is
required for beet mode in general and interfamily setups in both
tunnel and beet mode.
Signed-off-by: Joakim Koskela <jookos@gmail.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: Diego Beltrami <diego.beltrami@gmail.com>
Signed-off-by: Miika Komu <miika@iki.fi>
---
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 157bfbd..75fdb7d 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -1299,7 +1299,8 @@ xfrm_tmpl_resolve_one(struct xfrm_policy *policy, struct flowi *fl,
xfrm_address_t *local = saddr;
struct xfrm_tmpl *tmpl = &policy->xfrm_vec[i];
- if (tmpl->mode == XFRM_MODE_TUNNEL) {
+ if (tmpl->mode == XFRM_MODE_TUNNEL ||
+ tmpl->mode == XFRM_MODE_BEET) {
remote = &tmpl->id.daddr;
local = &tmpl->saddr;
family = tmpl->encap_family;
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index e070c3f..f5d30c4 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -611,7 +611,7 @@ xfrm_state_find(xfrm_address_t *daddr, xfrm_address_t *saddr,
selector.
*/
if (x->km.state == XFRM_STATE_VALID) {
- if (!xfrm_selector_match(&x->sel, fl, family) ||
+ if (!xfrm_selector_match(&x->sel, fl, x->sel.family) ||
!security_xfrm_state_pol_flow_match(x, pol, fl))
continue;
if (!best ||
@@ -623,7 +623,7 @@ xfrm_state_find(xfrm_address_t *daddr, xfrm_address_t *saddr,
acquire_in_progress = 1;
} else if (x->km.state == XFRM_STATE_ERROR ||
x->km.state == XFRM_STATE_EXPIRED) {
- if (xfrm_selector_match(&x->sel, fl, family) &&
+ if (xfrm_selector_match(&x->sel, fl, x->sel.family) &&
security_xfrm_state_pol_flow_match(x, pol, fl))
error = -ESRCH;
}
^ permalink raw reply related
* Re: [2.6 patch] eepro100 resume patch
From: Pavel Machek @ 2007-07-23 7:44 UTC (permalink / raw)
To: Kok, Auke; +Cc: David Fries, NetDev, Linux Kernel Mailing List
In-Reply-To: <46984CF0.8080700@intel.com>
On Fri 2007-07-13 21:11:28, Kok, Auke wrote:
> [adding netdev]
>
> David Fries wrote:
> >When I did a software suspend to disk then resumed the Intel network
> >card using eepro100 driver would be unable to transmit packets. I
> >tracked this down and found a register write after the print message
> >"DP83840 specific setup" which wasn't being executed when the system
> >was restored. This fix moves that write and another write which
> >forces the link speed and duplex.
> >
> >After doing this work and preparing the patch I checked out the
> >mailing list only to find a patch that removes the eepro100. I then
> >updated Kconfig, though I wonder why it didn't have a similar message
> >in it long time ago.
> >
> >I too had tried the e100 driver some time ago and it didn't work,
>
> That argument is pretty useless right now. Please *test* e100 and *report
> issues*. I recently did some very intensive suspend/resume testing (and
> fixes) on e100 and I have yet to hear of any problems with it since... that
> was 2.6.18 or so even.
>
> >eepro100 did and I've been using it so long that I've almost forgotten
> >about that. I just gave the e100 driver a try and I've been running
> >for about an hour now without any problems and it does resume after a
> >suspend to disk operation.
> >
> >Signed-off-by: David Fries <David@Fries.net>
>
> I don't think I need to NAK this. I doubt that Jeff Garzik will apply this
> in the first place. eepro100 is on it's way out, so let's focus on what
> matters.
Well, I believe we should either remove eepro100 _now_ or fix issues
with it. "Keep it in the tree, and broken at the same time" is unnice.
> >@@ -743,20 +746,22 @@
> > phys[(eeprom[7]>>8)&7]);
> > if (((eeprom[6]>>8) & 0x3f) == DP83840
> > || ((eeprom[6]>>8) & 0x3f) == DP83840A) {
> >- int mdi_reg23 = mdio_read(dev, eeprom[6] & 0x1f, 23)
> >| 0x0422;
> >+ int mdi_reg23_orig = mdio_read(dev, eeprom[6] &
> >0x1f, 23);
> >+ int mdi_reg23 = mdi_reg23_orig | 0x0422;
> > if (congenb)
> > mdi_reg23 |= 0x0100;
> >- printk(KERN_INFO" DP83840 specific setup, setting
> >register 23 to %4.4x.\n",
> >- mdi_reg23);
> >- mdio_write(dev, eeprom[6] & 0x1f, 23, mdi_reg23);
> >+ /* Print the message here, write in speedo_resume,
> >which
> >+ * is called both on module load and from
> >+ * eepro100_resume.
> >+ */
> >+ printk(KERN_INFO" DP83840 specific setup, setting
> >register 23 to %4.4x, was %4.4x.\n",
> >+ mdi_reg23, mdi_reg23_orig);
> > }
> > if ((option >= 0) && (option & 0x70)) {
> >+ /* Print here, write in speedo_resume. */
> > printk(KERN_INFO " Forcing %dMbs %s-duplex
> > operation.\n",
> > (option & 0x20 ? 100 : 10),
> > (option & 0x10 ? "full" : "half"));
> >- mdio_write(dev, eeprom[6] & 0x1f, MII_BMCR,
> >- ((option & 0x20) ? 0x2000 : 0) |
> >/* 100mbps? */
> >- ((option & 0x10) ? 0x0100 : 0));
> >/* Full duplex? */
> > }
> >
> > /* Perform a system self-test. */
> >@@ -1050,6 +1055,24 @@
> > struct speedo_private *sp = netdev_priv(dev);
> > void __iomem *ioaddr = sp->regs;
> >
> >+ /* DP83840 specific setup, moved here from from speedo_found1 because
> >+ * it needs to called after resume, ie suspend to disk.
> >+ * 07-11-2007 David Fries <david@fries.net>
> >+ */
> >+ if (((sp->phy[0]>>8) & 0x3f) == DP83840
> >+ || ((sp->phy[0]>>8) & 0x3f) == DP83840A) {
> >+ int mdi_reg23 = mdio_read(dev, sp->phy[0] & 0x1f, 23) |
> >0x0422;
> >+ if (congenb)
> >+ mdi_reg23 |= 0x0100;
> >+ mdio_write(dev, sp->phy[0] & 0x1f, 23, mdi_reg23);
> >+ }
> >+ if ((sp->option >= 0) && (sp->option & 0x70)) {
> >+ mdio_write(dev, sp->phy[0] & 0x1f, MII_BMCR,
> >+ ((sp->option & 0x20) ? 0x2000 : 0) | /* 100mbps?
> >*/
> >+ ((sp->option & 0x10) ? 0x0100 : 0)); /* Full duplex?
> >*/
> >+ }
> >+
> >+
> > /* Start with a Tx threshold of 256 (0x..20.... 8 byte units). */
> > sp->tx_threshold = 0x01208000;
> >
> >
> >
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply
* [PATCH] eHEA: net_poll support
From: Jan-Bernd Themann @ 2007-07-23 14:05 UTC (permalink / raw)
To: Jeff Garzik
Cc: netdev, Christoph Raisch, Jan-Bernd Themann, linux-kernel,
linux-ppc, Marcus Eder, Thomas Klein, Stefan Roscher
net_poll support for eHEA added
Signed-off-by: Jan-Bernd Themann <themann@de.ibm.com>
---
drivers/net/ehea/ehea.h | 2 +-
drivers/net/ehea/ehea_main.c | 22 +++++++++++++++++++++-
2 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ehea/ehea.h b/drivers/net/ehea/ehea.h
index 489c8b2..8ee2c2c 100644
--- a/drivers/net/ehea/ehea.h
+++ b/drivers/net/ehea/ehea.h
@@ -39,7 +39,7 @@
#include <asm/io.h>
#define DRV_NAME "ehea"
-#define DRV_VERSION "EHEA_0071"
+#define DRV_VERSION "EHEA_0072"
/* eHEA capability flags */
#define DLPAR_PORT_ADD_REM 1
diff --git a/drivers/net/ehea/ehea_main.c b/drivers/net/ehea/ehea_main.c
index 4c70a93..58702f5 100644
--- a/drivers/net/ehea/ehea_main.c
+++ b/drivers/net/ehea/ehea_main.c
@@ -589,6 +589,23 @@ static int ehea_poll(struct net_device *dev, int *budget)
return 1;
}
+#ifdef CONFIG_NET_POLL_CONTROLLER
+static void ehea_netpoll(struct net_device *dev)
+{
+ struct ehea_port *port = netdev_priv(dev);
+
+ netif_rx_schedule(port->port_res[0].d_netdev);
+}
+#endif
+
+static int ehea_poll_firstqueue(struct net_device *dev, int *budget)
+{
+ struct ehea_port *port = netdev_priv(dev);
+ struct net_device *d_dev = port->port_res[0].d_netdev;
+
+ return ehea_poll(d_dev, budget);
+}
+
static irqreturn_t ehea_recv_irq_handler(int irq, void *param)
{
struct ehea_port_res *pr = param;
@@ -2626,7 +2643,10 @@ struct ehea_port *ehea_setup_single_port(struct ehea_adapter *adapter,
memcpy(dev->dev_addr, &port->mac_addr, ETH_ALEN);
dev->open = ehea_open;
- dev->poll = ehea_poll;
+ dev->poll = ehea_poll_firstqueue;
+#ifdef CONFIG_NET_POLL_CONTROLLER
+ dev->poll_controller = ehea_netpoll;
+#endif
dev->weight = 64;
dev->stop = ehea_stop;
dev->hard_start_xmit = ehea_start_xmit;
--
1.5.2
^ permalink raw reply related
* Re: [PATCH][12/37] Clean up duplicate includes in drivers/net/
From: John W. Linville @ 2007-07-23 13:22 UTC (permalink / raw)
To: Jesper Juhl
Cc: info, Luke Yang, Judy Fischbach, Patrick McHardy, bonding-devel,
Bryan Wu, kong.lai, Jay Vosburgh, James Morris, linuxppc-embedded,
Daniel Drake, Alex V Lasso, Ulrich Kunitz, Chad Tindel,
Amit S Kale, Jay Cliburn, Samuel Ortiz, Brian Pugh,
Alexey Kuznetsov, Chris Snook, alexandre.bounine,
Pantelis Antoniou, netdev, linux-wireless,
Linux Kernel Mailing List, Ralf Baechle
In-Reply-To: <200707211702.46375.jesper.juhl@gmail.com>
On Sat, Jul 21, 2007 at 05:02:45PM +0200, Jesper Juhl wrote:
> Hi,
>
> This patch cleans up duplicate includes in
> drivers/net/
>
>
> Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
> diff --git a/drivers/net/wireless/ipw2200.h b/drivers/net/wireless/ipw2200.h
> index 626a240..9c973b9 100644
> --- a/drivers/net/wireless/ipw2200.h
> +++ b/drivers/net/wireless/ipw2200.h
> @@ -45,7 +45,6 @@
>
> #include <linux/firmware.h>
> #include <linux/wireless.h>
> -#include <linux/dma-mapping.h>
> #include <linux/jiffies.h>
> #include <asm/io.h>
>
> diff --git a/drivers/net/wireless/zd1211rw/zd_def.h b/drivers/net/wireless/zd1211rw/zd_def.h
> index deb99d1..505b4d7 100644
> --- a/drivers/net/wireless/zd1211rw/zd_def.h
> +++ b/drivers/net/wireless/zd1211rw/zd_def.h
> @@ -21,7 +21,6 @@
> #include <linux/kernel.h>
> #include <linux/stringify.h>
> #include <linux/device.h>
> -#include <linux/kernel.h>
>
> typedef u16 __nocast zd_addr_t;
ACK
--
John W. Linville
linville@tuxdriver.com
^ permalink raw reply
* Re: [PATCH 00/10] Implement batching skb API
From: jamal @ 2007-07-23 12:32 UTC (permalink / raw)
To: Krishna Kumar2
Cc: davem, gaagaan, general, herbert, jagana, jeff, johnpol, kaber,
kumarkr, mcarlson, mchan, netdev, peter.p.waskiewicz.jr, rdreier,
rick.jones2, Robert.Olsson, sri, tgraf, xma
In-Reply-To: <OFFA8C2879.1D54F523-ON65257321.0010F240-65257321.001A8A31@in.ibm.com>
KK,
On Mon, 2007-23-07 at 10:19 +0530, Krishna Kumar2 wrote:
> Hmmm ? Evgeniy has not even tested my code to find some regression :) And
> you may possibly not find much improvement in E1000 when you run iperf
> (which is what I do) compared to pktgen.
Pktgen is the correct test (or the closest to correct) because it tests
the driver tx path. iperf/netperf test the effect of batching on
tcp/udp. Infact i would start with udp first. What you need to do if
testing end-2-end is see where the effects occur. For example, it is
feasible that batching is a little too aggressive and the receiver cant
keep up (netstat -s before and after will be helpful).
Maybe by such insight we can improve things.
> > My experiments show it is useful (in a very visible way using pktgen)
> > for e1000 to have the prep() interface.
>
> I meant : have you compared results of batching with prep on vs prep off,
> and
> what is the difference in BW ?
Yes, and these results were sent to you as well a while back.
When i get the time when i get back i will look em up in my test machine
and resend.
> No. I see value only in non-LLTX drivers which also gets the same TX lock
> in the RX path.
So _which_ non-LLTX driver doesnt do that? ;->
> > The value is also there in LLTX drivers even if in just formating a skb
> > ready for transmit. If this is not clear i could do a much longer
> > writeup on my thought evolution towards adding prep().
>
> In LLTX drivers, the driver does the 'prep' without holding the tx_lock in
> any case, so there should be no improvement. Could you send the write-up
I will - please give me sometime; i am overloaded at the moment.
> There is *nothing* IPoIB specific or focus in my code.
> I said adding prep
> doesn't
> work for IPoIB and so it is pointless to add bloat to the code until some
> code can
tun driver doesnt use it either - but i doubt that makes it "bloat"
> What I meant to say
> is that there isn't much point in saying that your code is not ready or
> you are using old code base, or has multiple restart functions, or is not
> tested enough, etc, and then say let's re-do/rethink the whole
> implementation when my code is already working and giving good results.
The suggestive hand gesturing is the kind of thing that bothers me. What
do you think: Would i be submitting patches in baed on 2.6.22-rc4? Would
it make sense to include parallel qdisc paths? For heavens sake, i have
told you i would be fine with accepting such changes when the qdisc
restart changes went in first.
You waltz in, have the luxury of looking at my code, presentations, many
discussions with me etc ...
When i ask for differences to code you produced, they now seem to sum up
to the two below. You dont think theres some honest issue with this
picture?
> OTOH, if you find some cases that are better handled with :
> 1. prep handler
> 2. xmit_win (which I don't have now),
> then please send me patches and I will also test out and incorporate.
>
And then of course you will end up adding those because they are both
useful, just calling them some other name. And then you will end up
incorporating all the drivers i invested many hours (as a gratitous
volunteer) to change and test - maybe you will change varibale names or
rearrange some function.
I am a very compromising person; i have no problem coauthoring these
patches if you actually invest useful time like fixing things up and
doing proper tests. But you are not doing that - instead you are being
extremely aggressive and hijacking the whole thing. It is courteous if
you find somebody else has a patch you point out whats wrong preferably
with some proof.
> > It sounds disingenuous but i may have misread you.
>
> ("lacking in frankness, candor, or sincerity; falsely or hypocritically
> ingenuous; insincere") ???? Sorry, no response to personal comments and
> have a flame-war :)
Give me a better description.
cheers,
jamal
^ permalink raw reply
* Re: [PATCH 4/4] Initialize and fill IPv6 route age
From: Stephen Hemminger @ 2007-07-23 11:34 UTC (permalink / raw)
To: Varun Chandramohan; +Cc: netdev, sri, dlstevens, varuncha
In-Reply-To: <20070723101318.c773f600.varunc@linux.vnet.ibm.com>
On Mon, 23 Jul 2007 10:13:18 +0530
Varun Chandramohan <varunc@linux.vnet.ibm.com> wrote:
> The age field of the ipv6 route structures are initilized with the current timeval at the time of route creation. When the route dump is called the route age value stored in the structure is subtracted from the present timeval and the difference is passed on as the route age.
>
> Signed-off-by: Varun Chandramohan <varunc@linux.vnet.ibm.com>
> ---
> include/net/ip6_fib.h | 1 +
> include/net/ip6_route.h | 3 +++
> net/ipv6/addrconf.c | 5 +++++
> net/ipv6/route.c | 23 +++++++++++++++++++----
> 4 files changed, 28 insertions(+), 4 deletions(-)
>
> diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
> index c48ea87..e30a1cf 100644
> --- a/include/net/ip6_fib.h
> +++ b/include/net/ip6_fib.h
> @@ -98,6 +98,7 @@ struct rt6_info
>
> u32 rt6i_flags;
> u32 rt6i_metric;
> + time_t rt6i_age;
> atomic_t rt6i_ref;
> struct fib6_table *rt6i_table;
>
> diff --git a/include/net/ip6_route.h b/include/net/ip6_route.h
> index 5456fdd..fc9716c 100644
> --- a/include/net/ip6_route.h
> +++ b/include/net/ip6_route.h
> @@ -36,6 +36,9 @@ struct route_info {
> #define RT6_LOOKUP_F_REACHABLE 0x2
> #define RT6_LOOKUP_F_HAS_SADDR 0x4
>
> +#define RT6_SET_ROUTE_INFO 0x0
> +#define RT6_GET_ROUTE_INFO 0x1
> +
> extern struct rt6_info ip6_null_entry;
>
> #ifdef CONFIG_IPV6_MULTIPLE_TABLES
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 5a5f8bd..715c766 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -4187,6 +4187,7 @@ EXPORT_SYMBOL(unregister_inet6addr_notif
>
> int __init addrconf_init(void)
> {
> + struct timeval tv;
> int err = 0;
>
> /* The addrconf netdev notifier requires that loopback_dev
> @@ -4214,10 +4215,14 @@ int __init addrconf_init(void)
> if (err)
> return err;
>
> + do_gettimeofday(&tv);
Better to use ktime_t or timespec in new code.
^ permalink raw reply
* Re: [PATCH 2/4] Add new timeval_to_sec function
From: Patrick McHardy @ 2007-07-23 11:24 UTC (permalink / raw)
To: Varun Chandramohan; +Cc: netdev, sri, dlstevens, varuncha
In-Reply-To: <20070723101159.32ef3bdd.varunc@linux.vnet.ibm.com>
Varun Chandramohan wrote:
> /**
> + * timeval_to_sec - Convert timeval to seconds
> + * @tv: pointer to the timeval variable to be converted
> + *
> + * Returns the seconds representation of timeval parameter.
> + */
> +static inline time_t timeval_to_sec(const struct timeval *tv)
> +{
> + return (tv->tv_sec + (tv->tv_usec + 500000)/1000000);
> +}
I don't think you should round down timeout values.
^ permalink raw reply
* [ofa-general] Re: [PATCH 11/12 -Rev2] IPoIB xmit API addition
From: Krishna Kumar2 @ 2007-07-23 11:17 UTC (permalink / raw)
To: Evgeniy Polyakov
Cc: jagana, mcarlson, herbert, gaagaan, Robert.Olsson, kumarkr,
rdreier, peter.p.waskiewicz.jr, hadi, kaber, jeff, general, mchan,
tgraf, netdev, davem, sri
In-Reply-To: <20070723104826.GD22877@2ka.mipt.ru>
Hi Evgeniy,
Evgeniy Polyakov <johnpol@2ka.mipt.ru> wrote on 07/23/2007 04:18:26 PM:
> > static void ipoib_ib_handle_tx_wc(struct net_device *dev, struct ib_wc
*wc)
> > {
> > struct ipoib_dev_priv *priv = netdev_priv(dev);
> > + int i = 0, num_completions;
> > + int tx_ring_index = priv->tx_tail & (ipoib_sendq_size - 1);
> > unsigned int wr_id = wc->wr_id;
> > - struct ipoib_tx_buf *tx_req;
> > unsigned long flags;
> >
> > ipoib_dbg_data(priv, "send completion: id %d, status: %d\n",
> > @@ -255,23 +256,57 @@ static void ipoib_ib_handle_tx_wc(struct
> > return;
> > }
> >
> > - tx_req = &priv->tx_ring[wr_id];
> > + num_completions = wr_id - tx_ring_index + 1;
> > + if (num_completions <= 0)
> > + num_completions += ipoib_sendq_size;
>
> Can this still be less than zero?
Should never happen, otherwise the TX code wrote on bad/unallocated
memory and would have crashed first.
Thanks,
- KK
^ permalink raw reply
* [ofa-general] Re: [PATCH 03/12 -Rev2] dev.c changes.
From: Krishna Kumar2 @ 2007-07-23 11:17 UTC (permalink / raw)
To: Evgeniy Polyakov
Cc: jagana, mcarlson, herbert, gaagaan, Robert.Olsson, kumarkr,
rdreier, peter.p.waskiewicz.jr, hadi, kaber, jeff, general, mchan,
tgraf, netdev, davem, sri
In-Reply-To: <20070723104428.GC22877@2ka.mipt.ru>
Hi Evgeniy,
Evgeniy Polyakov <johnpol@2ka.mipt.ru> wrote on 07/23/2007 04:14:28 PM:
> > +/*
> > + * dev_change_tx_batching - Enable or disable batching for a driver
that
> > + * supports batching.
> > + /* Check if new value is same as the current */
> > + if (!!(dev->features & NETIF_F_BATCH_ON) == !!new_batch_skb)
> > + goto out;
>
> o_O
>
> Scratched head for too long before understood what it means :)
Is there a easy way to do this ?
> > + spin_lock(&dev->queue_lock);
> > + if (new_batch_skb) {
> > + dev->features |= NETIF_F_BATCH_ON;
> > + dev->tx_queue_len >>= 1;
> > + } else {
> > + if (!skb_queue_empty(&dev->skb_blist))
> > + skb_queue_purge(&dev->skb_blist);
> > + dev->features &= ~NETIF_F_BATCH_ON;
> > + dev->tx_queue_len <<= 1;
> > + }
> > + spin_unlock(&dev->queue_lock);
>
> Hmm, should this also stop interrupts?
That is a good question, and I am not sure. I thought it
is not required, though adding it doesn't affect code
either. Can someone tell if disabling bh is required and
why (couldn't figure out the intention of bh for
dev_queue_xmit either, is this to disable preemption) ?
Thanks,
- KK
^ permalink raw reply
* [ofa-general] Re: [PATCH 11/12 -Rev2] IPoIB xmit API addition
From: Evgeniy Polyakov @ 2007-07-23 10:48 UTC (permalink / raw)
To: Krishna Kumar
Cc: jagana, Robert.Olsson, peter.p.waskiewicz.jr, herbert, gaagaan,
kumarkr, rdreier, mcarlson, kaber, jeff, general, netdev, tgraf,
hadi, davem, mchan, sri
In-Reply-To: <20070722090649.7787.47960.sendpatchset@K50wks273871wss.in.ibm.com>
On Sun, Jul 22, 2007 at 02:36:49PM +0530, Krishna Kumar (krkumar2@in.ibm.com) wrote:
> diff -ruNp org/drivers/infiniband/ulp/ipoib/ipoib_ib.c rev2/drivers/infiniband/ulp/ipoib/ipoib_ib.c
> --- org/drivers/infiniband/ulp/ipoib/ipoib_ib.c 2007-07-20 07:49:28.000000000 +0530
> +++ rev2/drivers/infiniband/ulp/ipoib/ipoib_ib.c 2007-07-22 00:08:37.000000000 +0530
> @@ -242,8 +242,9 @@ repost:
> static void ipoib_ib_handle_tx_wc(struct net_device *dev, struct ib_wc *wc)
> {
> struct ipoib_dev_priv *priv = netdev_priv(dev);
> + int i = 0, num_completions;
> + int tx_ring_index = priv->tx_tail & (ipoib_sendq_size - 1);
> unsigned int wr_id = wc->wr_id;
> - struct ipoib_tx_buf *tx_req;
> unsigned long flags;
>
> ipoib_dbg_data(priv, "send completion: id %d, status: %d\n",
> @@ -255,23 +256,57 @@ static void ipoib_ib_handle_tx_wc(struct
> return;
> }
>
> - tx_req = &priv->tx_ring[wr_id];
> + num_completions = wr_id - tx_ring_index + 1;
> + if (num_completions <= 0)
> + num_completions += ipoib_sendq_size;
Can this still be less than zero?
--
Evgeniy Polyakov
^ permalink raw reply
* [ofa-general] Re: [PATCH 03/12 -Rev2] dev.c changes.
From: Evgeniy Polyakov @ 2007-07-23 10:44 UTC (permalink / raw)
To: Krishna Kumar
Cc: jagana, Robert.Olsson, peter.p.waskiewicz.jr, herbert, gaagaan,
kumarkr, rdreier, mcarlson, kaber, jeff, general, netdev, tgraf,
hadi, davem, mchan, sri
In-Reply-To: <20070722090525.7787.10432.sendpatchset@K50wks273871wss.in.ibm.com>
Hi Krishna.
On Sun, Jul 22, 2007 at 02:35:25PM +0530, Krishna Kumar (krkumar2@in.ibm.com) wrote:
> diff -ruNp org/net/core/dev.c rev2/net/core/dev.c
> --- org/net/core/dev.c 2007-07-20 07:49:28.000000000 +0530
> +++ rev2/net/core/dev.c 2007-07-21 23:08:33.000000000 +0530
> @@ -875,6 +875,48 @@ void netdev_state_change(struct net_devi
> }
> }
>
> +/*
> + * dev_change_tx_batching - Enable or disable batching for a driver that
> + * supports batching.
> + */
> +int dev_change_tx_batching(struct net_device *dev, unsigned long new_batch_skb)
> +{
> + int ret;
> +
> + if (!dev->hard_start_xmit_batch) {
> + /* Driver doesn't support skb batching */
> + ret = -ENOTSUPP;
> + goto out;
> + }
> +
> + /* Handle invalid argument */
> + if (new_batch_skb < 0) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + ret = 0;
> +
> + /* Check if new value is same as the current */
> + if (!!(dev->features & NETIF_F_BATCH_ON) == !!new_batch_skb)
> + goto out;
o_O
Scratched head for too long before understood what it means :)
> + spin_lock(&dev->queue_lock);
> + if (new_batch_skb) {
> + dev->features |= NETIF_F_BATCH_ON;
> + dev->tx_queue_len >>= 1;
> + } else {
> + if (!skb_queue_empty(&dev->skb_blist))
> + skb_queue_purge(&dev->skb_blist);
> + dev->features &= ~NETIF_F_BATCH_ON;
> + dev->tx_queue_len <<= 1;
> + }
> + spin_unlock(&dev->queue_lock);
Hmm, should this also stop interrupts?
--
Evgeniy Polyakov
^ permalink raw reply
* Re: [RFC 1/1] lro: Generic Large Receive Offload for TCP traffic
From: Evgeniy Polyakov @ 2007-07-23 10:38 UTC (permalink / raw)
To: Jan-Bernd Themann
Cc: Thomas Klein, Jan-Bernd Themann, netdev, linux-kernel, linux-ppc,
Christoph Raisch, Marcus Eder, Stefan Roscher, David Miller
In-Reply-To: <200707201741.49290.ossthema@de.ibm.com>
Hi Jan-Bernd.
On Fri, Jul 20, 2007 at 05:41:48PM +0200, Jan-Bernd Themann (ossthema@de.ibm.com) wrote:
> Generic LRO patch
>
> Signed-off-by: Jan-Bernd Themann <themann@de.ibm.com>
Besides couple trivial codyng/formatting nits I did not found any
problematic places after review. Details below.
Thanks.
> +#define TCP_PAYLOAD_LENGTH(iph, tcph) \
> +(ntohs(iph->tot_len) - IP_HDR_LEN(iph) - TCP_HDR_LEN(tcph))
A tab?
> +static void lro_add_common(struct net_lro_desc *lro_desc, struct iphdr *iph,
> + struct tcphdr *tcph, int tcp_data_len)
> +{
> + struct sk_buff *parent = lro_desc->parent;
> + u32 *topt;
> +
> + lro_desc->pkt_aggr_cnt++;
> + lro_desc->ip_tot_len += tcp_data_len;
> + lro_desc->tcp_next_seq += tcp_data_len;
> + lro_desc->tcp_window = tcph->window;
> + lro_desc->tcp_ack = tcph->ack_seq;
> +
> + /* don't update tcp_rcv_tsval, would not work with PAWS */
> + if (lro_desc->tcp_saw_tstamp) {
> + topt = (u32 *) (tcph + 1);
> + lro_desc->tcp_rcv_tsecr = *(topt + 2);
> + }
> +
> + parent->len += tcp_data_len;
> + parent->data_len += tcp_data_len;
> +
> + lro_desc->data_csum = csum_add(lro_desc->data_csum,
> + lro_tcp_data_csum(iph, tcph,
> + tcp_data_len));
> + return;
> +}
return from void? And in other places too.
> +int __lro_proc_skb(struct net_lro_mgr *lro_mgr, struct sk_buff *skb,
> + struct vlan_group *vgrp, u16 vlan_tag, void *priv)
> +{
> + struct net_lro_desc *lro_desc;
> + struct iphdr *iph;
> + struct tcphdr *tcph;
> + u64 flags;
Broken tab and spaces.
> +struct sk_buff *lro_gen_skb(struct net_lro_mgr *lro_mgr,
> + struct skb_frag_struct *frags,
> + int len, int true_size,
> + void *mac_hdr,
> + int hlen)
> +{
> + struct sk_buff *skb;
> + struct skb_frag_struct *skb_frags;
> + int data_len = len;
The same.
> + skb = netdev_alloc_skb(lro_mgr->dev, hlen);
> + if (!skb)
> + return NULL;
> +
> + skb->len = len;
> + skb->data_len = len - hlen;
Here too.
There is number of such places, ommitted others.
--
Evgeniy Polyakov
^ permalink raw reply
* Re: [GENETLINK]: Question: global lock (genl_mutex) possible refinement?
From: Thomas Graf @ 2007-07-23 10:29 UTC (permalink / raw)
To: Richard MUSIL; +Cc: Patrick McHardy, netdev
In-Reply-To: <46A0DF91.6000508@st.com>
* Richard MUSIL <richard.musil@st.com> 2007-07-20 18:15
> Patrick McHardy wrote:
> > Export the lock/unlock/.. functions. You'll also need a new version
> > similar to __rtnl_unlock.
>
> Patrick, you might feel, I am not reading your lines, but in fact I do.
> The problem is that I do not feel competent to follow/propose such
> changes. So what I propose here (in included patch) is the least change
> scenario, which I can think of and on which I feel safe.
>
> If there are some other changes required, as you suggested for example
> exporting lock from genetlink module, I hope authors of genetlink will
> comment on that. Currently, I do not see any reason for that, but this
> could be due to my limited knowledge.
Actually there is no reason to not use separate locks for the
message serialization and the protection of the list of registered
families. There is only one lock simply for the reason that I've
never thought of anybody could think of registering a new genetlink
family while processing a message.
Alternatively you could also postpone the registration of the new
genetlink family to a workqueue.
^ permalink raw reply
* Re: [1/2] 2.6.23-rc1: known regressions
From: Al Viro @ 2007-07-23 10:21 UTC (permalink / raw)
To: Michal Piotrowski
Cc: Linus Torvalds, Andrew Morton, LKML, Andi Kleen, Ingo Molnar,
Markus, Andre Noll, acpi4asus-user, Corentin Chary, Karol Kozimor,
Gabriel C, Netdev, Stephen Hemminger, Thomas Meyer
In-Reply-To: <46A47932.8000709@googlemail.com>
On Mon, Jul 23, 2007 at 11:47:30AM +0200, Michal Piotrowski wrote:
> Subject : drivers/misc/asus-laptop.c:*: error: 'struct led_classdev' has no member named 'class_dev'
> References : http://lkml.org/lkml/2007/7/22/299
> Last known good : ?
> Submitter : Gabriel C <nix.or.die@googlemail.com>
> Caused-By : ?
> Handled-By : ?
> Status : unknown
>From 2a7e1148a9d3ee860dc2650c9a45288b120e250f Mon Sep 17 00:00:00 2001
From: Al Viro <viro@zeniv.linux.org.uk>
Date: Mon, 23 Jul 2007 06:20:22 -0400
Subject: [PATCH] Fix failure exits in asus-laptop
Fallout from f8a7c6fe14f556ca8eeddce258cb21392d0c3a2f. However, looking
at it shows that checks done in ASUS_LED_UNREGISTER() can't trigger
at all (we never get to asus_led_exit() if registration fails) and
if that registration fails, we actually leak stuff. IOW, it's worse
than just replacing class_dev with dev in there - the tests themselves
had been papering over the lousy cleanup logics.
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
drivers/misc/asus-laptop.c | 32 +++++++++++++++++++++-----------
1 files changed, 21 insertions(+), 11 deletions(-)
diff --git a/drivers/misc/asus-laptop.c b/drivers/misc/asus-laptop.c
index f753060..6b89854 100644
--- a/drivers/misc/asus-laptop.c
+++ b/drivers/misc/asus-laptop.c
@@ -1067,19 +1067,16 @@ static void asus_backlight_exit(void)
}
#define ASUS_LED_UNREGISTER(object) \
- if(object##_led.class_dev \
- && !IS_ERR(object##_led.class_dev)) \
- led_classdev_unregister(&object##_led)
+ led_classdev_unregister(&object##_led)
static void asus_led_exit(void)
{
+ destroy_workqueue(led_workqueue);
ASUS_LED_UNREGISTER(mled);
ASUS_LED_UNREGISTER(tled);
ASUS_LED_UNREGISTER(pled);
ASUS_LED_UNREGISTER(rled);
ASUS_LED_UNREGISTER(gled);
-
- destroy_workqueue(led_workqueue);
}
static void __exit asus_laptop_exit(void)
@@ -1135,29 +1132,42 @@ static int asus_led_init(struct device *dev)
rv = ASUS_LED_REGISTER(mled, dev);
if (rv)
- return rv;
+ goto out;
rv = ASUS_LED_REGISTER(tled, dev);
if (rv)
- return rv;
+ goto out1;
rv = ASUS_LED_REGISTER(rled, dev);
if (rv)
- return rv;
+ goto out2;
rv = ASUS_LED_REGISTER(pled, dev);
if (rv)
- return rv;
+ goto out3;
rv = ASUS_LED_REGISTER(gled, dev);
if (rv)
- return rv;
+ goto out4;
led_workqueue = create_singlethread_workqueue("led_workqueue");
if (!led_workqueue)
- return -ENOMEM;
+ goto out5;
return 0;
+out5:
+ rv = -ENOMEM;
+ ASUS_LED_UNREGISTER(gled);
+out4:
+ ASUS_LED_UNREGISTER(pled);
+out3:
+ ASUS_LED_UNREGISTER(rled);
+out2:
+ ASUS_LED_UNREGISTER(tled);
+out1:
+ ASUS_LED_UNREGISTER(mled);
+out:
+ return rv;
}
static int __init asus_laptop_init(void)
--
1.5.3.GIT
^ permalink raw reply related
* Re: [PATCH] [IrDA] KS959 USB IrDA dongle support
From: Alan Cox @ 2007-07-23 10:13 UTC (permalink / raw)
To: Samuel Ortiz
Cc: netdev@vger.kernel.org, irda-users@lists.sourceforge.net, davem,
linux-usb-devel@lists.sourceforge.net, a_villacis@palosanto.com
In-Reply-To: <kjg2gBqm.1185175506.5213420.samuel@sortiz.org>
> >Too late, Linus just closed the merge window, you had two
> >weeks to submit this :-)
> Too bad...
> I submitted it as soon as it was ready. Will we have to wait until the
> 2.6.24 merge window, or can it be applied earlier as it's a fully
> standalone driver ?
You can always send Linus a copy and ask but if its just finished then
far better to send it to Andrew so that it can get testing and picked at
in -mm then merged next time around when polished.
Alan
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
^ permalink raw reply
* Re: [PATCH]: Resurrect napi_poll patch.
From: Stephen Hemminger @ 2007-07-23 9:58 UTC (permalink / raw)
To: Andi Kleen; +Cc: David Miller, netdev, rusty, jgarzik
In-Reply-To: <p73lkd9vqfr.fsf@bingen.suse.de>
On 21 Jul 2007 15:26:00 +0200
Andi Kleen <andi@firstfloor.org> wrote:
> David Miller <davem@davemloft.net> writes:
> >
> > Good candidates for taking advantage of multi-napi are:
> >
> > 1) e1000
> > 2) ucc_geth
> > 3) ehea
> > 4) sunvnet
>
> s2io.c
sky2.c could use it because of issues with dual-port that share
one napi for status.
^ permalink raw reply
* Re: [PATCH 04/10] net-sysfs.c changes.
From: Stephen Hemminger @ 2007-07-23 9:56 UTC (permalink / raw)
To: Krishna Kumar2
Cc: davem, gaagaan, general, hadi, herbert, jagana, jeff, johnpol,
Patrick McHardy, kumarkr, mcarlson, mchan, netdev,
peter.p.waskiewicz.jr, rdreier, rick.jones2, Robert.Olsson, sri,
tgraf, xma
In-Reply-To: <OFFAA4E4C4.F1496A54-ON6525731F.0025194E-6525731F.0025379C@in.ibm.com>
On Sat, 21 Jul 2007 12:16:30 +0530
Krishna Kumar2 <krkumar2@in.ibm.com> wrote:
> Stephen Hemminger <shemminger@linux-foundation.org> wrote on 07/20/2007
> 09:52:03 PM:
> > Patrick McHardy <kaber@trash.net> wrote:
> >
> > > Krishna Kumar2 wrote:
> > > > Patrick McHardy <kaber@trash.net> wrote on 07/20/2007 03:37:20 PM:
> > > >
> > > >
> > > >
> > > >> rtnetlink support seems more important than sysfs to me.
> > > >>
> > > >
> > > > Thanks, I will add that as a patch. The reason to add to sysfs is
> that
> > > > it is easier to change for a user (and similar to tx_queue_len).
> > > >
> > >
> >
> > But since batching is so similar to TSO, i really should be part of the
> > flags and controlled by ethtool like other offload flags.
>
> So should I add all three interfaces (or which ones) :
>
> 1. /sys (like for tx_queue_len)
> 2. netlink
> 3. ethtool.
>
> Or only 2 & 3 are enough ?
>
Yes, please do #3 and maybe #2.
Sysfs api's are a long term ABI problem.
^ permalink raw reply
* Re: [PATCH 00/12 -Rev2] Implement batching skb API
From: Krishna Kumar2 @ 2007-07-23 9:53 UTC (permalink / raw)
To: Krishna Kumar2
Cc: davem, gaagaan, general, hadi, herbert, jagana, jeff, johnpol,
kaber, kumarkr, mcarlson, mchan, netdev, peter.p.waskiewicz.jr,
rdreier, rick.jones2, Robert.Olsson, sri, tgraf, xma
In-Reply-To: <20070722090457.7787.4601.sendpatchset@K50wks273871wss.in.ibm.com>
> I have started a 10 run test for various buffer sizes and processes, and
> will post the results on Monday.
The 10 iteration run results for Rev2 are (average) :
----------------------------------------------------------------------------------
Test Case Org New %Change
----------------------------------------------------------------------------------
TCP 1 Process
Size:32 2703 3063 13.31
Size:128 12948 12217 -5.64
Size:512 48108 55384 15.12
Size:4096 129089 132586 2.70
Average: 192848 203250 5.39
TCP 4 Processes
Size:32 10389 10768 3.64
Size:128 39694 42265 6.47
Size:512 159563 156373 -1.99
Size:4096 268094 256008 -4.50
Average: 477740 465414 -2.58
TCP No Delay 1 Process
Size:32 2606 2950 13.20
Size:128 8115 11864 46.19
Size:512 39113 42608 8.93
Size:4096 103966 105333 1.31
Average: 153800 162755 5.82
TCP No Delay 4 Processes
Size:32 4213 8727 107.14
Size:128 17579 35143 99.91
Size:512 70803 123936 75.04
Size:4096 203541 225259 10.67
Average: 296136 393065 32.73
--------------------------------------------------------------------------
Average: 1120524 1224484 9.28%
There are three cases that degrade a little (upto -5.6%), but there are 13
cases
that improve, and many of those are in the 13% to over 100% (7 cases).
Thanks,
- KK
Krishna Kumar2/India/IBM@IBMIN wrote on 07/22/2007 02:34:57 PM:
> This set of patches implements the batching API, and makes the following
> changes resulting from the review of the first set:
>
> Changes :
> ---------
> 1. Changed skb_blist from pointer to static as it saves only 12 bytes
> (i386), but bloats the code.
> 2. Removed requirement for driver to set "features & NETIF_F_BATCH_SKBS"
> in register_netdev to enable batching as it is redundant. Changed
this
> flag to NETIF_F_BATCH_ON and it is set by register_netdev, and other
> user changable calls can modify this bit to enable/disable batching.
> 3. Added ethtool support to enable/disable batching (not tested).
> 4. Added rtnetlink support to enable/disable batching (not tested).
> 5. Removed MIN_QUEUE_LEN_BATCH for batching as high performance drivers
> should not have a small queue anyway (adding bloat).
> 6. skbs are purged from dev_deactivate instead of from unregister_netdev
> to drop all references to the device.
> 7. Removed changelog in source code in sch_generic.c, and unrelated
renames
> from sch_generic.c (lockless, comments).
> 8. Removed xmit_slots entirely, as it was adding bloat (code and header)
> and not adding value (it is calculated and set twice in internal send
> routine and handle work completion, and referenced once in batch
xmit;
> and can instead be calculated once in xmit).
>
> Issues :
> --------
> 1. Remove /sysfs support completely ?
> 2. Whether rtnetlink support is required as GSO has only ethtool ?
>
> Patches are described as:
> Mail 0/12 : This mail.
> Mail 1/12 : HOWTO documentation.
> Mail 2/12 : Changes to netdevice.h
> Mail 3/12 : dev.c changes.
> Mail 4/12 : Ethtool changes.
> Mail 5/12 : sysfs changes.
> Mail 6/12 : rtnetlink changes.
> Mail 7/12 : Change in qdisc_run & qdisc_restart API, modify callers
> to use this API.
> Mail 8/12 : IPoIB include file changes.
> Mail 9/12 : IPoIB verbs changes
> Mail 10/12 : IPoIB multicast, CM changes
> Mail 11/12 : IPoIB xmit API addition
> Mail 12/12 : IPoIB xmit internals changes (ipoib_ib.c)
>
> I have started a 10 run test for various buffer sizes and processes, and
> will post the results on Monday.
>
> Please review and provide feedback/ideas; and consider for inclusion.
>
> Thanks,
>
> - KK
^ permalink raw reply
* [1/2] 2.6.23-rc1: known regressions
From: Michal Piotrowski @ 2007-07-23 9:47 UTC (permalink / raw)
To: Linus Torvalds, Andrew Morton, LKML, Andi Kleen, Ingo Molnar,
Markus, Andre Noll, acpi4asus-user, Corentin Chary, Karol Kozimor,
Gabriel C, Netdev, Stephen Hemminger, Thomas Meyer
Hi all,
Here is a list of some known regressions in 2.6.23-rc1.
Feel free to add new regressions/remove fixed etc.
http://kernelnewbies.org/known_regressions
List of Aces
Name Regressions fixed since 21-Jun-2007
Andi Kleen 4
Linus Torvalds 4
Adrian Bunk 3
Andrew Morton 3
Jens Axboe 3
Al Viro 2
David Woodhouse 2
Hugh Dickins 2
Tejun Heo 2
Unclassified
Subject : /usr/bin/ld: section .text [ffffffffff700500 -> ffffffffff7007e3] overlaps section .gnu.version_d [ffffffffff7004d8 -> ffffffffff70050f]
References : http://lkml.org/lkml/2007/7/22/239
Last known good : ?
Submitter : Andre Noll <maan@systemlinux.org>
Caused-By : ?
Handled-By : Andi Kleen <ak@suse.de>
Status : problem is being debugged
Subject : pcwd_init_module(): WARNING: at lib/kref.c:33 kref_get()
References : http://lkml.org/lkml/2007/7/22/94
Last known good : ?
Submitter : Ingo Molnar <mingo@elte.hu>
Caused-By : ?
Handled-By : ?
Status : problem is being debugged
Subject : konqueror suddenly vanishing, "konqueror: Fatal IO error: client killed"
References : http://lkml.org/lkml/2007/7/22/86
Last known good : ?
Submitter : Markus <lists4me@web.de>
Caused-By : ?
Handled-By : Ingo Molnar <mingo@elte.hu>
Status : problem is being debugged
ACPI
Subject : drivers/misc/asus-laptop.c:*: error: 'struct led_classdev' has no member named 'class_dev'
References : http://lkml.org/lkml/2007/7/22/299
Last known good : ?
Submitter : Gabriel C <nix.or.die@googlemail.com>
Caused-By : ?
Handled-By : ?
Status : unknown
Networking
Subject : New wake ups from sky2
References : http://lkml.org/lkml/2007/7/20/386
Last known good : ?
Submitter : Thomas Meyer <thomas@m3y3r.de>
Caused-By : Stephen Hemminger <shemminger@osdl.org>
commit eb35cf60e462491249166182e3e755d3d5d91a28
Handled-By : Stephen Hemminger <shemminger@osdl.org>
Status : unknown
Regards,
Michal
--
LOG
http://www.stardust.webpages.pl/log/
^ permalink raw reply
* Re: TCP and batching WAS(Re: [PATCH 00/10] Implement batching skb API
From: Stephen Hemminger @ 2007-07-23 9:44 UTC (permalink / raw)
To: hadi
Cc: Krishna Kumar, davem, rdreier, johnpol, Robert.Olsson,
peter.p.waskiewicz.jr, kumarkr, herbert, gaagaan, mcarlson, xma,
rick.jones2, jeff, general, mchan, tgraf, netdev, jagana, kaber,
sri
In-Reply-To: <1185025579.5192.68.camel@localhost>
On Sat, 21 Jul 2007 09:46:19 -0400
jamal <hadi@cyberus.ca> wrote:
> On Fri, 2007-20-07 at 08:18 +0100, Stephen Hemminger wrote:
>
> > You may see worse performance with batching in the real world when
> > running over WAN's. Like TSO, batching will generate back to back packet
> > trains that are subject to multi-packet synchronized loss.
>
> Has someone done any study on TSO effect?
Not that I have seen, TCP research tends to turn of NAPI and TSO because it
causes other effects which are too confusing for measurement. The discussion
of TSO usually shows up in discussions of pacing. I have seen argument both
pro and con for pacing. The most convincing arguments are that pacing doesn't
help in the general case (and therefore TSO would be ok).
> Doesnt ECN with a RED router
> help on something like this?
Yes, but RED is not deployed on backbone, and ECN only slightly.
Most common is over sized FIFO queues.
> I find it suprising that a single flow doing TSO would overwhelm a
> routers buffer. I actually think the value of batching as far as TCP is
> concerned is propotional to the number of flows. i.e the more flows you
> have the more batching you will end up doing. And if TCPs fairness is
> the legend talk it has been made to be, then i dont see this as
> problematic.
It is not that TSO would overwhelm the router by itself, just that any
congested link will have periods when there is only a small number of
available slots left. When this happens a TSO burst will get truncated.
The argument against pacing, and for TSO; is that the busy sender with
large congestion window is the one most likely to have send large bursts.
For fairness, the system works better if the busy sender gets penalized more,
and dropping the latter part of the burst does that. With pacing, the sender
may be able to saturate the router more and not detect that it is monopolizing
the bandwidth.
> BTW, something i noticed regards to GSO when testing batching:
> For TCP packets slightly above MDU (upto 2K), GSO gives worse
> performance than non-GSO. Actually has nothing to do with batching,
> rather it works the same way with or without batching changes.
>
> Another oddity:
> Looking at the flow rate from a purely packets/second (I know thats a
> router centric view, but i found it strange nevertheless) - you see that
> as packet size goes up, the pps also goes up. I tried mucking around
> with nagle etc, but saw no observable changes. Any insight?
> My expectation was that the pps would stay at least the same or get
> better with smaller packets (assuming theres less data to push around).
>
> cheers,
> jamal
>
>
>
^ permalink raw reply
* Re: 2.6.20->2.6.21 - networking dies after random time
From: Jarek Poplawski @ 2007-07-23 8:53 UTC (permalink / raw)
To: Marcin Ślusarz
Cc: Jean-Baptiste Vignaud, linux-kernel, shemminger, linux-net,
netdev, Ingo Molnar, Thomas Gleixner, Andrew Morton,
Linus Torvalds
In-Reply-To: <4bacf17f0707222244p664e7a6ap850b3357a57d73c@mail.gmail.com>
On Mon, Jul 23, 2007 at 07:44:58AM +0200, Marcin Ślusarz wrote:
> Ok, I've bisected this problem and found that this patch broke my NIC:
Congratulations!
>
> 76d2160147f43f982dfe881404cfde9fd0a9da21 is first bad commit
> commit 76d2160147f43f982dfe881404cfde9fd0a9da21
> Author: Ingo Molnar <mingo@elte.hu>
> Date: Fri Feb 16 01:28:24 2007 -0800
>
> [PATCH] genirq: do not mask interrupts by default
...
> So I cooked patch like below and everything is working fine (so far)
>
> Fix default_disable interrupt function (broken by [PATCH] genirq: do
> not mask interrupts by default) - revert removal of codepath which was
> invoked when removed flag (IRQ_DELAYED_DISABLE) wag NOT set
>
> Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
> ---
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index 76a9106..0bb23cd 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -230,6 +230,8 @@ static void default_enable(unsigned int irq)
> */
> static void default_disable(unsigned int irq)
> {
> + struct irq_desc *desc = irq_desc + irq;
> + desc->chip->mask(irq);
> }
>
> /*
I think your patch should very good point the source of the problem
and would help to many people, but it looks like too arbitrary for
those who didn't have such problems. It seems it was mainly with
x86_64, so maybe something like this below would be enough?
Cheers,
Jarek P.
PS: not tested!
---
diff -Nurp 2.6.22-/arch/x86_64/kernel/io_apic.c 2.6.22/arch/x86_64/kernel/io_apic.c
--- 2.6.22-/arch/x86_64/kernel/io_apic.c 2007-07-09 01:32:17.000000000 +0200
+++ 2.6.22/arch/x86_64/kernel/io_apic.c 2007-07-23 10:33:05.000000000 +0200
@@ -1427,6 +1427,7 @@ static struct irq_chip ioapic_chip __rea
.name = "IO-APIC",
.startup = startup_ioapic_irq,
.mask = mask_IO_APIC_irq,
+ .disable = mask_IO_APIC_irq,
.unmask = unmask_IO_APIC_irq,
.ack = ack_apic_edge,
.eoi = ack_apic_level,
^ permalink raw reply
* [2.6 patch] drivers/net/acenic.c: fix check-after-use
From: Adrian Bunk @ 2007-07-23 8:02 UTC (permalink / raw)
To: jes, Jeff Garzik; +Cc: linux-acenic, netdev, linux-kernel
The Coverity checker noted that we've already dereferenced "dev" when we
check whether it's NULL.
Since it's impossible that "dev" is NULL at this place this patch
removes the NULL check.
Signed-off-by: Adrian Bunk <bunk@stusta.de>
---
--- linux-2.6.22-rc6-mm1/drivers/net/acenic.c.old 2007-07-23 04:06:05.000000000 +0200
+++ linux-2.6.22-rc6-mm1/drivers/net/acenic.c 2007-07-23 04:08:11.000000000 +0200
@@ -3124,20 +3124,14 @@ static int __devinit read_eeprom_byte(st
struct ace_private *ap = netdev_priv(dev);
struct ace_regs __iomem *regs = ap->regs;
unsigned long flags;
u32 local;
int result = 0;
short i;
- if (!dev) {
- printk(KERN_ERR "No device!\n");
- result = -ENODEV;
- goto out;
- }
-
/*
* Don't take interrupts on this CPU will bit banging
* the %#%#@$ I2C device
*/
local_irq_save(flags);
eeprom_start(regs);
^ permalink raw reply
* [PATCH] [2.6.22] Fix a potential NULL pointer dereference in write_bulk_callback() in drivers/net/usb/pegasus.c
From: Micah Gruber @ 2007-07-23 8:05 UTC (permalink / raw)
To: petkan, linux-kernel, netdev, jgarzik
This patch fixes a potential null dereference bug where we dereference
pegasus before a null check. This patch simply moves the dereferencing
after the null check.
Signed-off-by: Micah Gruber <micah.gruber@gmail.com>
---
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c
@@ -768,11 +768,13 @@
static void write_bulk_callback(struct urb *urb)
{
pegasus_t *pegasus = urb->context;
- struct net_device *net = pegasus->net;
+ struct net_device *net;
if (!pegasus)
return;
+ net = pegasus->net;
+
if (!netif_device_present(net) || !netif_running(net))
return;
^ permalink raw reply
* Re: [PATCH 4/4] Initialize and fill IPv6 route age
From: Krishna Kumar2 @ 2007-07-23 7:56 UTC (permalink / raw)
To: Varun Chandramohan; +Cc: dlstevens, netdev, sri, Varun Chandramohan
In-Reply-To: <20070723101318.c773f600.varunc@linux.vnet.ibm.com>
> + if (dumpflg)
> + NLA_PUT_U32(skb, RTA_AGE, timeval_to_sec(&tv) - rt->rt6i_age);
> + else
> + NLA_PUT_U32(skb, RTA_AGE, rt->rt6i_age);
Makes more sense (and easy to understand) if you use :
if (dumpflg == RT6_GET_ROUTE_INFO)
...
so that your code does not break if someone changed the #define values.
- KK
netdev-owner@vger.kernel.org wrote on 07/23/2007 10:13:18 AM:
> The age field of the ipv6 route structures are initilized with the
current
> timeval at the time of route creation. When the route dump is
called the
> route age value stored in the structure is subtracted from the
present
> timeval and the difference is passed on as the route age.
>
> Signed-off-by: Varun Chandramohan <varunc@linux.vnet.ibm.com>
> ---
> include/net/ip6_fib.h | 1 +
> include/net/ip6_route.h | 3 +++
> net/ipv6/addrconf.c | 5 +++++
> net/ipv6/route.c | 23 +++++++++++++++++++----
> 4 files changed, 28 insertions(+), 4 deletions(-)
>
> diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
> index c48ea87..e30a1cf 100644
> --- a/include/net/ip6_fib.h
> +++ b/include/net/ip6_fib.h
> @@ -98,6 +98,7 @@ struct rt6_info
>
> u32 rt6i_flags;
> u32 rt6i_metric;
> + time_t rt6i_age;
> atomic_t rt6i_ref;
> struct fib6_table *rt6i_table;
>
> diff --git a/include/net/ip6_route.h b/include/net/ip6_route.h
> index 5456fdd..fc9716c 100644
> --- a/include/net/ip6_route.h
> +++ b/include/net/ip6_route.h
> @@ -36,6 +36,9 @@ struct route_info {
> #define RT6_LOOKUP_F_REACHABLE 0x2
> #define RT6_LOOKUP_F_HAS_SADDR 0x4
>
> +#define RT6_SET_ROUTE_INFO 0x0
> +#define RT6_GET_ROUTE_INFO 0x1
> +
> extern struct rt6_info ip6_null_entry;
>
> #ifdef CONFIG_IPV6_MULTIPLE_TABLES
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 5a5f8bd..715c766 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -4187,6 +4187,7 @@ EXPORT_SYMBOL(unregister_inet6addr_notif
>
> int __init addrconf_init(void)
> {
> + struct timeval tv;
> int err = 0;
>
> /* The addrconf netdev notifier requires that loopback_dev
> @@ -4214,10 +4215,14 @@ int __init addrconf_init(void)
> if (err)
> return err;
>
> + do_gettimeofday(&tv);
> ip6_null_entry.rt6i_idev = in6_dev_get(&loopback_dev);
> + ip6_null_entry.rt6i_age = timeval_to_sec(&tv);
> #ifdef CONFIG_IPV6_MULTIPLE_TABLES
> ip6_prohibit_entry.rt6i_idev = in6_dev_get(&loopback_dev);
> + ip6_prohibit_entry.rt6i_age = timeval_to_sec(&tv);
> ip6_blk_hole_entry.rt6i_idev = in6_dev_get(&loopback_dev);
> + ip6_blk_hole_entry.rt6i_age = timeval_to_sec(&tv);
> #endif
>
> register_netdevice_notifier(&ipv6_dev_notf);
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index fe8d983..9386c05 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -600,7 +600,14 @@ static int __ip6_ins_rt(struct rt6_info
> {
> int err;
> struct fib6_table *table;
> + struct timeval tv;
>
> + do_gettimeofday(&tv);
> + /* Update the timeval for new routes
> + * We add it here to make it common irrespective
> + * of how the new route is added.
> + */
> + rt->rt6i_age = timeval_to_sec(&tv);
> table = rt->rt6i_table;
> write_lock_bh(&table->tb6_lock);
> err = fib6_add(&table->tb6_root, rt, info);
> @@ -2111,6 +2118,7 @@ static inline size_t rt6_nlmsg_size(void
> + nla_total_size(4) /* RTA_IIF */
> + nla_total_size(4) /* RTA_OIF */
> + nla_total_size(4) /* RTA_PRIORITY */
> + + nla_total_size(4) /*RTA_AGE*/
> + RTAX_MAX * nla_total_size(4) /* RTA_METRICS */
> + nla_total_size(sizeof(struct rta_cacheinfo));
> }
> @@ -2118,10 +2126,11 @@ static inline size_t rt6_nlmsg_size(void
> static int rt6_fill_node(struct sk_buff *skb, struct rt6_info *rt,
> struct in6_addr *dst, struct in6_addr *src,
> int iif, int type, u32 pid, u32 seq,
> - int prefix, unsigned int flags)
> + int prefix, unsigned int flags, int dumpflg)
> {
> struct rtmsg *rtm;
> struct nlmsghdr *nlh;
> + struct timeval tv;
> long expires;
> u32 table;
>
> @@ -2185,6 +2194,12 @@ static int rt6_fill_node(struct sk_buff
> if (ipv6_get_saddr(&rt->u.dst, dst, &saddr_buf) == 0)
> NLA_PUT(skb, RTA_PREFSRC, 16, &saddr_buf);
> }
> +
> + do_gettimeofday(&tv);
> + if (dumpflg)
> + NLA_PUT_U32(skb, RTA_AGE, timeval_to_sec(&tv) - rt->rt6i_age);
> + else
> + NLA_PUT_U32(skb, RTA_AGE, rt->rt6i_age);
>
> if (rtnetlink_put_metrics(skb, rt->u.dst.metrics) < 0)
> goto nla_put_failure;
> @@ -2222,7 +2237,7 @@ int rt6_dump_route(struct rt6_info *rt,
>
> return rt6_fill_node(arg->skb, rt, NULL, NULL, 0, RTM_NEWROUTE,
> NETLINK_CB(arg->cb->skb).pid, arg->cb->nlh->nlmsg_seq,
> - prefix, NLM_F_MULTI);
> + prefix, NLM_F_MULTI, RT6_GET_ROUTE_INFO);
> }
>
> static int inet6_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr*
nlh, void *arg)
> @@ -2287,7 +2302,7 @@ static int inet6_rtm_getroute(struct sk_
>
> err = rt6_fill_node(skb, rt, &fl.fl6_dst, &fl.fl6_src, iif,
> RTM_NEWROUTE, NETLINK_CB(in_skb).pid,
> - nlh->nlmsg_seq, 0, 0);
> + nlh->nlmsg_seq, 0, 0, RT6_GET_ROUTE_INFO);
> if (err < 0) {
> kfree_skb(skb);
> goto errout;
> @@ -2316,7 +2331,7 @@ void inet6_rt_notify(int event, struct r
> if (skb == NULL)
> goto errout;
>
> - err = rt6_fill_node(skb, rt, NULL, NULL, 0, event, pid, seq, 0, 0);
> + err = rt6_fill_node(skb, rt, NULL, NULL, 0, event, pid, seq, 0, 0,
> RT6_SET_ROUTE_INFO);
> if (err < 0) {
> /* -EMSGSIZE implies BUG in rt6_nlmsg_size() */
> WARN_ON(err == -EMSGSIZE);
> --
> 1.4.3.4
>
> -
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
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