* Re: TCP SACK issue, hung connection, tcpdump included
From: Willy Tarreau @ 2007-07-29 16:07 UTC (permalink / raw)
To: Ilpo Järvinen; +Cc: Darryl L. Miles, linux-kernel, Netdev
In-Reply-To: <Pine.LNX.4.64.0707291208230.10340@kivilampi-30.cs.helsinki.fi>
On Sun, Jul 29, 2007 at 12:28:04PM +0300, Ilpo Järvinen wrote:
(...)
> > > Limitation for 48 byte segments? You have to be kidding... :-) But yes,
> > > it seems that one of the directions is dropping packets for some reason
> > > though I would not assume MTU limitation... Or did you mean some other
> > > segment?
> >
> > No, I was talking about the 1448 bytes segments. But in fact I don't
> > believe it much because the SACKs are always retransmitted just afterwards.
>
> Ah, but it's ACKed correctly right below it...:
>
> [...snip...]
> > > > > 09:21:39.490740 IP SERVER.ssh > CLIENT.50727: P 18200:18464(264) ack 2991
> > > > > win 2728 <nop,nop,timestamp 7692910 800001727>
> > > > > 09:21:39.490775 IP CLIENT.50727 > SERVER.ssh: . ack 18464 win 378
> > > > > <nop,nop,timestamp 800001755 7692910>
> > > > > 09:21:39.860245 IP SERVER.ssh > CLIENT.50727: . 12408:13856(1448) ack 2991
> > > > > win 2728 <nop,nop,timestamp 7693293 800001749>
>
> ...segment below snd_una arrived => snd_una remains 18464, receiver
> generates a duplicate ACK:
>
> > > > > 09:21:39.860302 IP CLIENT.50727 > SERVER.ssh: . ack 18464 win 378
> > > > > <nop,nop,timestamp 800001847 7692910,nop,nop,sack sack 1 {12408:13856} >
>
> The cumulative ACK field of it covers _everything_ below 18464 (i.e., it
> ACKs them), including the 1448 bytes in 12408:13856... In addition, the
> SACK block is DSACK information [RFC2883] telling explicitly the address
> of the received duplicate block. However, if this ACK doesn't reach the
> SERVER TCP, RTO is triggered and the first not yet cumulatively ACKed
> segment is retransmitted (I guess cumulative ACKs up to 12408 arrived
> without problems to the SERVER):
Oh yes, you're damn right. I did not notice that the ACK was higher than
the SACK, I'm more used to read traces with absolute rather than relative
seq/acks.
So I agree, it is this ACK which is lost between client and server,
reinforcing the supposition about the location of the capture (client side).
> [...snip...]
>
> > BTW, some information are missing. It would have been better if the trace
> > had been read with tcpdump -Svv. We would have got seq numbers and ttl.
> > Also, we do not know if there's a firewall between both sides. Sometimes,
> > some IDS identify attacks in crypted traffic and kill connections. It
> > might have been the case here, with the connection closed one way on an
> > intermediate firewall.
>
> Yeah, firewall or some other issue, I'd say it's quite unlikely a bug in
> TCP because behavior to both directions indicate client -> sender
> blackhole independently of each other...
It would also be possible that something stupid between both ends simply
drops packets with the SACK option set. Also something which sometimes
happen is that some firewalls automatically translate sequence numbers
but not necessarily SACK values, which could pretty well lead to this
packet being received but ignored on the server side.
I'm pretty sure that the same trace taken on the server side will reveal
the reason for the problem.
Willy
^ permalink raw reply
* Re: TCP SACK issue, hung connection, tcpdump included
From: Ilpo Järvinen @ 2007-07-29 16:28 UTC (permalink / raw)
To: Willy Tarreau; +Cc: Ilpo Järvinen, Darryl L. Miles, linux-kernel, Netdev
In-Reply-To: <20070729160721.GA31276@1wt.eu>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 1861 bytes --]
On Sun, 29 Jul 2007, Willy Tarreau wrote:
> On Sun, Jul 29, 2007 at 12:28:04PM +0300, Ilpo Järvinen wrote:
>
> > [...snip...]
> >
> > > BTW, some information are missing. It would have been better if the trace
> > > had been read with tcpdump -Svv. We would have got seq numbers and ttl.
> > > Also, we do not know if there's a firewall between both sides. Sometimes,
> > > some IDS identify attacks in crypted traffic and kill connections. It
> > > might have been the case here, with the connection closed one way on an
> > > intermediate firewall.
> >
> > Yeah, firewall or some other issue, I'd say it's quite unlikely a bug in
> > TCP because behavior to both directions indicate client -> sender
> > blackhole independently of each other...
>
> It would also be possible that something stupid between both ends simply
> drops packets with the SACK option set. Also something which sometimes
> happen is that some firewalls automatically translate sequence numbers
> but not necessarily SACK values, which could pretty well lead to this
> packet being received but ignored on the server side.
...One can toggle those off with /proc/sys/net/ipv4/tcp_dsack but I
suspect DSACKs are not the cause because these retransmissions neither
are making it through (there are many of them also earlier in the log,
just quoted the easiest ones :-) ):
> > > > 09:36:44.335591 IP CLIENT.50727 > SERVER.ssh: P 2991:3039(48) ack 18464 win
> > > > 378 <nop,nop,timestamp 800227942 7692910>
> > > > 09:38:44.351950 IP CLIENT.50727 > SERVER.ssh: P 2991:3039(48) ack 18464 win
> > > > 378 <nop,nop,timestamp 800257942 7692910>
> > > > 09:40:44.368172 IP CLIENT.50727 > SERVER.ssh: P 2991:3039(48) ack 18464 win
> > > > 378 <nop,nop,timestamp 800287943 7692910>
...there are no SACKs involved in them, yet no cumulative ACK ever
arrives from SERVER...
--
i.
^ permalink raw reply
* Re: [PATCH] USB Pegasus driver - avoid a potential NULL pointer dereference.
From: Satyam Sharma @ 2007-07-29 18:18 UTC (permalink / raw)
To: Oliver Neukum
Cc: linux-usb-devel, Petko Manolov, Greg Kroah-Hartman, Jesper Juhl,
Linux Kernel Mailing List, netdev
In-Reply-To: <200707291049.26619.oliver@neukum.org>
On Sun, 29 Jul 2007, Oliver Neukum wrote:
> Am Sonntag 29 Juli 2007 schrieb Jesper Juhl:
> > On 29/07/07, Satyam Sharma <satyam.sharma@gmail.com> wrote:
> > > Hi,
> > >
> > > On 7/29/07, Jesper Juhl <jesper.juhl@gmail.com> wrote:
> > > > Hello,
> > > >
> > > > This patch makes sure we don't dereference a NULL pointer in
> > > > drivers/net/usb/pegasus.c::write_bulk_callback() in the initial
> > > > struct net_device *net = pegasus->net; assignment.
> > > > The existing code checks if 'pegasus' is NULL and bails out if
> > > > it is, so we better not touch that pointer until after that check.
> > > > [...]
> > >
> > > Is it really possible that we're called into this function with
> > > urb->context == NULL? If not, I'd suggest let's just get rid of
> > > the check itself, instead.
> > >
> > I'm not sure. I am not very familiar with this code. I just figured
> > that moving the assignment is potentially a little safer and it is
> > certainly no worse than the current code, so that's a safe and
> > potentially benneficial change. Removing the check may be safe but I'm
> > not certain enough to make that call...
>
> pegasus == NULL there would be a kernel bug. Silently ignoring
> it, like the code now wants to do is bad. As the oops has never been
> reported, I figure turning it into an explicit debugging test is overkill,
> so removal seems to be the best option.
Ok, thanks. Updated patch below.
[PATCH] pegasus: Remove bogus checks in urb->complete() callbacks
urb->complete() callbacks registered in drivers/net/usb/pegasus.c needlessly
check for urb->context != NULL, but that is not possible (the only way that
can be possible would be a kernel bug elsewhere, and these checks would
simply end up hiding that). So let's remove the bogus checks.
Signed-off-by: Satyam Sharma <satyam@infradead.org>
---
drivers/net/usb/pegasus.c | 9 +--------
1 files changed, 1 insertions(+), 8 deletions(-)
diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index a05fd97..439ef9f 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c
@@ -770,9 +770,6 @@ static void write_bulk_callback(struct urb *urb)
pegasus_t *pegasus = urb->context;
struct net_device *net = pegasus->net;
- if (!pegasus)
- return;
-
if (!netif_device_present(net) || !netif_running(net))
return;
@@ -805,13 +802,9 @@ static void write_bulk_callback(struct urb *urb)
static void intr_callback(struct urb *urb)
{
pegasus_t *pegasus = urb->context;
- struct net_device *net;
+ struct net_device *net = pegasus->net;
int status;
- if (!pegasus)
- return;
- net = pegasus->net;
-
switch (urb->status) {
case 0:
break;
-------------------------------------------------------------------------
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 related
* Re: [PATCH] sb1000: prevent a potential NULL pointer dereference in sb1000_dev_ioctl()
From: Satyam Sharma @ 2007-07-29 18:34 UTC (permalink / raw)
To: Domen Puncer
Cc: Jesper Juhl, Linux Kernel Mailing List, netdev, Steven Hirsch,
David S. Miller
In-Reply-To: <alpine.LFD.0.999.0707291106500.30928@enigma.security.iitk.ac.in>
> On Sun, 29 Jul 2007, Domen Puncer wrote:
>
> > On 29/07/07 00:02 +0200, Jesper Juhl wrote:
> > > Hi,
> > >
> > > Here's a small patch, prompted by a find by the Coverity checker,
> > > that removes a potential NULL pointer dereference from
> > > drivers/net/sb1000.c::sb1000_dev_ioctl().
> > > The checker spotted that we do a NULL test of 'dev', yet we
> > > dereference the pointer prior to that check.
> > > This patch simply moves the dereference after the NULL test.
> >
> > But... it can't be called without a valid 'dev', no?
> > A quick 'grep do_ioctl net/' confirms that all calls are in
> > the form of 'dev->do_ioctl(dev, ...'.
>
> Yup, I think so too ...
>
>
> > > @@ -991,11 +991,13 @@ static int sb1000_dev_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
> > > short PID[4];
> > > int ioaddr[2], status, frequency;
> > > unsigned int stats[5];
> > > - struct sb1000_private *lp = netdev_priv(dev);
> > > + struct sb1000_private *lp;
> > >
> > > if (!(dev && dev->flags & IFF_UP))
> > > return -ENODEV;
>
> I think we could get rid of the !dev check itself. Actually, the IFF_UP
> check /also/ looks suspect to me for two reasons: (1) I remember Stephen
> Hemminger once telling me dev->flags is legacy and unsafe, and one of
> the netif_xxx() functions be used instead, and, (2) I wonder if we really
> require the interface to be up and *running* when we do this ioctl.
Updated patch below.
[PATCH] sb1000: Remove bogus checks
In net_device->do_ioctl() of the sb1000 driver (sb1000_dev_ioctl):
(1) !dev condition is always false -- this function cannot be called with
NULL net_device.
(2) !(dev->flags & IFF_UP) is bogus because the functions of this ioctl
can (and should) be allowed even when the interface is not up and running.
So let's remove these checks.
Signed-off-by: Satyam Sharma <satyam@infradead.org>
Cc: Jesper Juhl <jesper.juhl@gmail.com>
Cc: Domen Puncer <domen@coderock.org>
---
drivers/net/sb1000.c | 3 ---
1 files changed, 0 insertions(+), 3 deletions(-)
diff --git a/drivers/net/sb1000.c b/drivers/net/sb1000.c
index 1de3eec..f60fe98 100644
--- a/drivers/net/sb1000.c
+++ b/drivers/net/sb1000.c
@@ -993,9 +993,6 @@ static int sb1000_dev_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
unsigned int stats[5];
struct sb1000_private *lp = netdev_priv(dev);
- if (!(dev && dev->flags & IFF_UP))
- return -ENODEV;
-
ioaddr[0] = dev->base_addr;
/* mem_start holds the second I/O address */
ioaddr[1] = dev->mem_start;
^ permalink raw reply related
* Re: [PATCH 4/7] Add "depth".
From: Michael Buesch @ 2007-07-29 18:41 UTC (permalink / raw)
To: Corey Hickey; +Cc: netdev
In-Reply-To: <11856929362692-git-send-email-bugfood-ml@fatooh.org>
On Sunday 29 July 2007 09:08:51 Corey Hickey wrote:
> p = d;
> n = q->dep[d].next;
> @@ -215,7 +216,7 @@ static unsigned int sfq_drop(struct Qdisc *sch)
> drop a packet from it */
>
> if (d > 1) {
> - sfq_index x = q->dep[d+SFQ_DEPTH].next;
> + sfq_index x = q->dep[d+q->depth].next;
Please q->dep[d + q->depth]
Makes it _much_ more readable. And doesn't confuse my brain with a
minus and a BiggerThan sign ;)
> @@ -383,6 +384,16 @@ static void sfq_perturbation(unsigned long arg)
> static void sfq_q_destroy(struct sfq_sched_data *q)
> {
> del_timer(&q->perturb_timer);
> + if(q->dep)
> + kfree(q->dep);
> + if(q->next)
> + kfree(q->next);
> + if(q->allot)
> + kfree(q->allot);
> + if(q->hash)
> + kfree(q->hash);
> + if(q->qs)
> + kfree(q->qs);
No need to check for !=NULL. kfree handles NULL.
> }
>
> static void sfq_destroy(struct Qdisc *sch)
> @@ -394,6 +405,7 @@ static void sfq_destroy(struct Qdisc *sch)
> static int sfq_q_init(struct sfq_sched_data *q, struct rtattr *opt)
> {
> struct tc_sfq_qopt *ctl = RTA_DATA(opt);
> + sfq_index p = ~0U/2;
> int i;
>
> if (opt && opt->rta_len < RTA_LENGTH(sizeof(*ctl)))
> @@ -401,30 +413,53 @@ static int sfq_q_init(struct sfq_sched_data *q, struct rtattr *opt)
>
> q->perturbation = 0;
> q->max_depth = 0;
> - q->tail = q->limit = SFQ_DEPTH;
> if (opt == NULL) {
> q->perturb_period = 0;
> + q->tail = q->limit = q->depth = SFQ_DEPTH_DEFAULT;
> } else {
> struct tc_sfq_qopt *ctl = RTA_DATA(opt);
> if (ctl->quantum)
> q->quantum = ctl->quantum;
> q->perturb_period = ctl->perturb_period*HZ;
> + q->tail = q->limit = q->depth = ctl->flows ? : SFQ_DEPTH_DEFAULT;
> +
> + if (q->depth > p - 1)
> + return -EINVAL;
Compare depth against (~0U/2)-1? What's that doing? Should probably add a comment.
>
> if (ctl->limit)
> - q->limit = min_t(u32, ctl->limit, SFQ_DEPTH);
> + q->limit = min_t(u32, ctl->limit, q->depth);
> }
>
> + q->dep = kmalloc((1+q->depth*2)*sizeof(struct sfq_head), GFP_KERNEL);
> + if (!q->dep)
> + goto err_case;
> + q->next = kmalloc(q->depth*sizeof(sfq_index), GFP_KERNEL);
> + if (!q->next)
> + goto err_case;
> + q->allot = kmalloc(q->depth*sizeof(short), GFP_KERNEL);
> + if (!q->allot)
> + goto err_case;
> + q->hash = kmalloc(q->depth*sizeof(unsigned short), GFP_KERNEL);
> + if (!q->hash)
> + goto err_case;
> + q->qs = kmalloc(q->depth*sizeof(struct sk_buff_head), GFP_KERNEL);
> + if (!q->qs)
> + goto err_case;
You may chose to use kcalloc for array allocations.
> for (i=0; i<SFQ_HASH_DIVISOR; i++)
> - q->ht[i] = SFQ_DEPTH;
> - for (i=0; i<SFQ_DEPTH; i++) {
> + q->ht[i] = q->depth;
> + for (i=0; i<q->depth; i++) {
> skb_queue_head_init(&q->qs[i]);
> - q->dep[i+SFQ_DEPTH].next = i+SFQ_DEPTH;
> - q->dep[i+SFQ_DEPTH].prev = i+SFQ_DEPTH;
> + q->dep[i+q->depth].next = i+q->depth;
> + q->dep[i+q->depth].prev = i+q->depth;
> }
>
> - for (i=0; i<SFQ_DEPTH; i++)
> + for (i=0; i<q->depth; i++)
> sfq_link(q, i);
> return 0;
> +err_case:
This leaks a few kmallocs.
> + sfq_q_destroy(q);
> + return -ENOBUFS;
> }
>
> static int sfq_init(struct Qdisc *sch, struct rtattr *opt)
> @@ -458,7 +493,7 @@ static int sfq_dump(struct Qdisc *sch, struct sk_buff *skb)
>
> opt.limit = q->limit;
> opt.divisor = SFQ_HASH_DIVISOR;
> - opt.flows = q->limit;
> + opt.flows = q->depth;
>
> RTA_PUT(skb, TCA_OPTIONS, sizeof(opt), &opt);
>
--
Greetings Michael.
^ permalink raw reply
* Re: [PATCH] sb1000: prevent a potential NULL pointer dereference in sb1000_dev_ioctl()
From: Michael Buesch @ 2007-07-29 18:48 UTC (permalink / raw)
To: Satyam Sharma
Cc: Domen Puncer, Jesper Juhl, Linux Kernel Mailing List, netdev,
Steven Hirsch, David S. Miller
In-Reply-To: <alpine.LFD.0.999.0707300003440.30928@enigma.security.iitk.ac.in>
On Sunday 29 July 2007 20:34:46 Satyam Sharma wrote:
> (2) !(dev->flags & IFF_UP) is bogus because the functions of this ioctl
> can (and should) be allowed even when the interface is not up and running.
Are you _sure_? This function does poke with the device hardware.
It might return crap or even machinecheck when not initialized.
Hardware is probably powered down, if not IFF_UP. (I don't know if that's
the case here, though).
> drivers/net/sb1000.c | 3 ---
> 1 files changed, 0 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/sb1000.c b/drivers/net/sb1000.c
> index 1de3eec..f60fe98 100644
> --- a/drivers/net/sb1000.c
> +++ b/drivers/net/sb1000.c
> @@ -993,9 +993,6 @@ static int sb1000_dev_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
> unsigned int stats[5];
> struct sb1000_private *lp = netdev_priv(dev);
>
> - if (!(dev && dev->flags & IFF_UP))
> - return -ENODEV;
> -
--
Greetings Michael.
^ permalink raw reply
* Re: [PATCH] sb1000: prevent a potential NULL pointer dereference in sb1000_dev_ioctl()
From: Satyam Sharma @ 2007-07-29 19:09 UTC (permalink / raw)
To: Michael Buesch
Cc: Domen Puncer, Jesper Juhl, Linux Kernel Mailing List, netdev,
Steven Hirsch, David S. Miller, Stephen Hemminger
In-Reply-To: <200707292048.20531.mb@bu3sch.de>
Hi Michael,
On Sun, 29 Jul 2007, Michael Buesch wrote:
> On Sunday 29 July 2007 20:34:46 Satyam Sharma wrote:
> > (2) !(dev->flags & IFF_UP) is bogus because the functions of this ioctl
> > can (and should) be allowed even when the interface is not up and running.
>
> Are you _sure_? This function does poke with the device hardware.
> It might return crap or even machinecheck when not initialized.
> Hardware is probably powered down, if not IFF_UP. (I don't know if that's
> the case here, though).
IFF_UP checks if the _interface_ is up -- the hardware / card could still
be powered up, but the interface down (ifconfing eth0 down or ip link set
eth0 down).
Probably what we want here is netif_device_present()? -- I think that
should return true only when the *device* itself is up (as in powered)
but the interface itself could be down ...
Let's wait for comments from the netdev people Cc:'ed here, in that case.
> > drivers/net/sb1000.c | 3 ---
> > 1 files changed, 0 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/sb1000.c b/drivers/net/sb1000.c
> > index 1de3eec..f60fe98 100644
> > --- a/drivers/net/sb1000.c
> > +++ b/drivers/net/sb1000.c
> > @@ -993,9 +993,6 @@ static int sb1000_dev_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
> > unsigned int stats[5];
> > struct sb1000_private *lp = netdev_priv(dev);
> >
> > - if (!(dev && dev->flags & IFF_UP))
> > - return -ENODEV;
> > -
Satyam
^ permalink raw reply
* [PATCH] ethtool_perm_addr only has one implementation
From: Matthew Wilcox @ 2007-07-29 20:08 UTC (permalink / raw)
To: netdev
All drivers implement ethtool get_perm_addr the same way -- by calling
the generic function. So we can inline the generic function into the
caller and avoid going through the drivers.
Signed-off-by: Matthew Wilcox <matthew@wil.cx>
diff -u b/net/core/ethtool.c b/net/core/ethtool.c
--- b/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -95,18 +95,6 @@
return 0;
}
-int ethtool_op_get_perm_addr(struct net_device *dev, struct ethtool_perm_addr *addr, u8 *data)
-{
- unsigned char len = dev->addr_len;
- if ( addr->size < len )
- return -ETOOSMALL;
-
- addr->size = len;
- memcpy(data, dev->perm_addr, len);
- return 0;
-}
-
-
u32 ethtool_op_get_ufo(struct net_device *dev)
{
return (dev->features & NETIF_F_UFO) != 0;
@@ -779,34 +767,20 @@
static int ethtool_get_perm_addr(struct net_device *dev, void __user *useraddr)
{
struct ethtool_perm_addr epaddr;
- u8 *data;
- int ret;
- if (!dev->ethtool_ops->get_perm_addr)
- return -EOPNOTSUPP;
-
- if (copy_from_user(&epaddr,useraddr,sizeof(epaddr)))
+ if (copy_from_user(&epaddr, useraddr, sizeof(epaddr)))
return -EFAULT;
- data = kmalloc(epaddr.size, GFP_USER);
- if (!data)
- return -ENOMEM;
-
- ret = dev->ethtool_ops->get_perm_addr(dev,&epaddr,data);
- if (ret)
- return ret;
+ if (epaddr.size < dev->addr_len)
+ return -ETOOSMALL;
+ epaddr.size = dev->addr_len;
- ret = -EFAULT;
if (copy_to_user(useraddr, &epaddr, sizeof(epaddr)))
- goto out;
+ return -EFAULT;
useraddr += sizeof(epaddr);
- if (copy_to_user(useraddr, data, epaddr.size))
- goto out;
- ret = 0;
-
- out:
- kfree(data);
- return ret;
+ if (copy_to_user(useraddr, dev->perm_addr, epaddr.size))
+ return -EFAULT;
+ return 0;
}
/* The main entry point in this file. Called from net/core/dev.c */
@@ -976,7 +950,6 @@
EXPORT_SYMBOL(dev_ethtool);
EXPORT_SYMBOL(ethtool_op_get_link);
-EXPORT_SYMBOL_GPL(ethtool_op_get_perm_addr);
EXPORT_SYMBOL(ethtool_op_get_sg);
EXPORT_SYMBOL(ethtool_op_get_tso);
EXPORT_SYMBOL(ethtool_op_get_tx_csum);
only in patch2:
unchanged:
--- a/drivers/net/3c59x.c
+++ b/drivers/net/3c59x.c
@@ -2886,7 +2886,6 @@ static const struct ethtool_ops vortex_ethtool_ops = {
.set_settings = vortex_set_settings,
.get_link = ethtool_op_get_link,
.nway_reset = vortex_nway_reset,
- .get_perm_addr = ethtool_op_get_perm_addr,
};
#ifdef CONFIG_PCI
only in patch2:
unchanged:
--- a/drivers/net/8139cp.c
+++ b/drivers/net/8139cp.c
@@ -1578,7 +1578,6 @@ static const struct ethtool_ops cp_ethtool_ops = {
.set_wol = cp_set_wol,
.get_strings = cp_get_strings,
.get_ethtool_stats = cp_get_ethtool_stats,
- .get_perm_addr = ethtool_op_get_perm_addr,
.get_eeprom_len = cp_get_eeprom_len,
.get_eeprom = cp_get_eeprom,
.set_eeprom = cp_set_eeprom,
only in patch2:
unchanged:
--- a/drivers/net/8139too.c
+++ b/drivers/net/8139too.c
@@ -2452,7 +2452,6 @@ static const struct ethtool_ops rtl8139_ethtool_ops = {
.get_strings = rtl8139_get_strings,
.get_stats_count = rtl8139_get_stats_count,
.get_ethtool_stats = rtl8139_get_ethtool_stats,
- .get_perm_addr = ethtool_op_get_perm_addr,
};
static int netdev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
only in patch2:
unchanged:
--- a/drivers/net/ax88796.c
+++ b/drivers/net/ax88796.c
@@ -580,7 +580,6 @@ static const struct ethtool_ops ax_ethtool_ops = {
.set_settings = ax_set_settings,
.nway_reset = ax_nway_reset,
.get_link = ax_get_link,
- .get_perm_addr = ethtool_op_get_perm_addr,
};
/* setup code */
only in patch2:
unchanged:
--- a/drivers/net/b44.c
+++ b/drivers/net/b44.c
@@ -2033,7 +2033,6 @@ static const struct ethtool_ops b44_ethtool_ops = {
.get_strings = b44_get_strings,
.get_stats_count = b44_get_stats_count,
.get_ethtool_stats = b44_get_ethtool_stats,
- .get_perm_addr = ethtool_op_get_perm_addr,
};
static int b44_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
only in patch2:
unchanged:
--- a/drivers/net/bnx2.c
+++ b/drivers/net/bnx2.c
@@ -6269,7 +6269,6 @@ static const struct ethtool_ops bnx2_ethtool_ops = {
.phys_id = bnx2_phys_id,
.get_stats_count = bnx2_get_stats_count,
.get_ethtool_stats = bnx2_get_ethtool_stats,
- .get_perm_addr = ethtool_op_get_perm_addr,
};
/* Called with rtnl_lock */
only in patch2:
unchanged:
--- a/drivers/net/cxgb3/cxgb3_main.c
+++ b/drivers/net/cxgb3/cxgb3_main.c
@@ -1583,7 +1583,6 @@ static const struct ethtool_ops cxgb_ethtool_ops = {
.get_wol = get_wol,
.get_tso = ethtool_op_get_tso,
.set_tso = ethtool_op_set_tso,
- .get_perm_addr = ethtool_op_get_perm_addr
};
static int in_range(int val, int lo, int hi)
only in patch2:
unchanged:
--- a/drivers/net/e100.c
+++ b/drivers/net/e100.c
@@ -2506,7 +2506,6 @@ static const struct ethtool_ops e100_ethtool_ops = {
.phys_id = e100_phys_id,
.get_stats_count = e100_get_stats_count,
.get_ethtool_stats = e100_get_ethtool_stats,
- .get_perm_addr = ethtool_op_get_perm_addr,
};
static int e100_do_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
only in patch2:
unchanged:
--- a/drivers/net/e1000/e1000_ethtool.c
+++ b/drivers/net/e1000/e1000_ethtool.c
@@ -1973,7 +1973,6 @@ static const struct ethtool_ops e1000_ethtool_ops = {
.phys_id = e1000_phys_id,
.get_stats_count = e1000_get_stats_count,
.get_ethtool_stats = e1000_get_ethtool_stats,
- .get_perm_addr = ethtool_op_get_perm_addr,
};
void e1000_set_ethtool_ops(struct net_device *netdev)
only in patch2:
unchanged:
--- a/drivers/net/forcedeth.c
+++ b/drivers/net/forcedeth.c
@@ -4706,7 +4706,6 @@ static const struct ethtool_ops ops = {
.get_regs_len = nv_get_regs_len,
.get_regs = nv_get_regs,
.nway_reset = nv_nway_reset,
- .get_perm_addr = ethtool_op_get_perm_addr,
.get_tso = ethtool_op_get_tso,
.set_tso = nv_set_tso,
.get_ringparam = nv_get_ringparam,
only in patch2:
unchanged:
--- a/drivers/net/ixgb/ixgb_ethtool.c
+++ b/drivers/net/ixgb/ixgb_ethtool.c
@@ -724,7 +724,6 @@ static const struct ethtool_ops ixgb_ethtool_ops = {
.phys_id = ixgb_phys_id,
.get_stats_count = ixgb_get_stats_count,
.get_ethtool_stats = ixgb_get_ethtool_stats,
- .get_perm_addr = ethtool_op_get_perm_addr,
};
void ixgb_set_ethtool_ops(struct net_device *netdev)
only in patch2:
unchanged:
--- a/drivers/net/ne2k-pci.c
+++ b/drivers/net/ne2k-pci.c
@@ -638,7 +638,6 @@ static const struct ethtool_ops ne2k_pci_ethtool_ops = {
.get_drvinfo = ne2k_pci_get_drvinfo,
.get_tx_csum = ethtool_op_get_tx_csum,
.get_sg = ethtool_op_get_sg,
- .get_perm_addr = ethtool_op_get_perm_addr,
};
static void __devexit ne2k_pci_remove_one (struct pci_dev *pdev)
only in patch2:
unchanged:
--- a/drivers/net/netxen/netxen_nic_ethtool.c
+++ b/drivers/net/netxen/netxen_nic_ethtool.c
@@ -755,5 +755,4 @@ struct ethtool_ops netxen_nic_ethtool_ops = {
.get_strings = netxen_nic_get_strings,
.get_stats_count = netxen_nic_get_stats_count,
.get_ethtool_stats = netxen_nic_get_ethtool_stats,
- .get_perm_addr = ethtool_op_get_perm_addr,
};
only in patch2:
unchanged:
--- a/drivers/net/pcnet32.c
+++ b/drivers/net/pcnet32.c
@@ -1515,7 +1515,6 @@ static const struct ethtool_ops pcnet32_ethtool_ops = {
.phys_id = pcnet32_phys_id,
.get_regs_len = pcnet32_get_regs_len,
.get_regs = pcnet32_get_regs,
- .get_perm_addr = ethtool_op_get_perm_addr,
};
/* only probes for non-PCI devices, the rest are handled by
only in patch2:
unchanged:
--- a/drivers/net/qla3xxx.c
+++ b/drivers/net/qla3xxx.c
@@ -1904,7 +1904,6 @@ static void ql_get_pauseparam(struct net_device *ndev,
static const struct ethtool_ops ql3xxx_ethtool_ops = {
.get_settings = ql_get_settings,
.get_drvinfo = ql_get_drvinfo,
- .get_perm_addr = ethtool_op_get_perm_addr,
.get_link = ethtool_op_get_link,
.get_msglevel = ql_get_msglevel,
.set_msglevel = ql_set_msglevel,
only in patch2:
unchanged:
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -1066,7 +1066,6 @@ static const struct ethtool_ops rtl8169_ethtool_ops = {
.get_strings = rtl8169_get_strings,
.get_stats_count = rtl8169_get_stats_count,
.get_ethtool_stats = rtl8169_get_ethtool_stats,
- .get_perm_addr = ethtool_op_get_perm_addr,
};
static void rtl8169_write_gmii_reg_bit(void __iomem *ioaddr, int reg,
only in patch2:
unchanged:
--- a/drivers/net/sc92031.c
+++ b/drivers/net/sc92031.c
@@ -1402,7 +1402,6 @@ static struct ethtool_ops sc92031_ethtool_ops = {
.get_strings = sc92031_ethtool_get_strings,
.get_stats_count = sc92031_ethtool_get_stats_count,
.get_ethtool_stats = sc92031_ethtool_get_ethtool_stats,
- .get_perm_addr = ethtool_op_get_perm_addr,
.get_ufo = ethtool_op_get_ufo,
};
only in patch2:
unchanged:
--- a/drivers/net/skge.c
+++ b/drivers/net/skge.c
@@ -821,7 +821,6 @@ static const struct ethtool_ops skge_ethtool_ops = {
.phys_id = skge_phys_id,
.get_stats_count = skge_get_stats_count,
.get_ethtool_stats = skge_get_ethtool_stats,
- .get_perm_addr = ethtool_op_get_perm_addr,
};
/*
only in patch2:
unchanged:
--- a/drivers/net/sky2.c
+++ b/drivers/net/sky2.c
@@ -3548,7 +3548,6 @@ static const struct ethtool_ops sky2_ethtool_ops = {
.phys_id = sky2_phys_id,
.get_stats_count = sky2_get_stats_count,
.get_ethtool_stats = sky2_get_ethtool_stats,
- .get_perm_addr = ethtool_op_get_perm_addr,
};
#ifdef CONFIG_SKY2_DEBUG
only in patch2:
unchanged:
--- a/drivers/net/sundance.c
+++ b/drivers/net/sundance.c
@@ -1586,7 +1586,6 @@ static const struct ethtool_ops ethtool_ops = {
.get_link = get_link,
.get_msglevel = get_msglevel,
.set_msglevel = set_msglevel,
- .get_perm_addr = ethtool_op_get_perm_addr,
};
static int netdev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
only in patch2:
unchanged:
--- a/drivers/net/sunvnet.c
+++ b/drivers/net/sunvnet.c
@@ -906,7 +906,6 @@ static const struct ethtool_ops vnet_ethtool_ops = {
.get_msglevel = vnet_get_msglevel,
.set_msglevel = vnet_set_msglevel,
.get_link = ethtool_op_get_link,
- .get_perm_addr = ethtool_op_get_perm_addr,
};
static void vnet_port_free_tx_bufs(struct vnet_port *port)
only in patch2:
unchanged:
--- a/drivers/net/tc35815.c
+++ b/drivers/net/tc35815.c
@@ -2198,7 +2198,6 @@ static const struct ethtool_ops tc35815_ethtool_ops = {
.get_strings = tc35815_get_strings,
.get_stats_count = tc35815_get_stats_count,
.get_ethtool_stats = tc35815_get_ethtool_stats,
- .get_perm_addr = ethtool_op_get_perm_addr,
};
static int tc35815_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
only in patch2:
unchanged:
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -9294,7 +9294,6 @@ static const struct ethtool_ops tg3_ethtool_ops = {
.get_ethtool_stats = tg3_get_ethtool_stats,
.get_coalesce = tg3_get_coalesce,
.set_coalesce = tg3_set_coalesce,
- .get_perm_addr = ethtool_op_get_perm_addr,
};
static void __devinit tg3_get_eeprom_size(struct tg3 *tp)
only in patch2:
unchanged:
--- a/drivers/net/via-rhine.c
+++ b/drivers/net/via-rhine.c
@@ -1805,7 +1805,6 @@ static const struct ethtool_ops netdev_ethtool_ops = {
.set_wol = rhine_set_wol,
.get_sg = ethtool_op_get_sg,
.get_tx_csum = ethtool_op_get_tx_csum,
- .get_perm_addr = ethtool_op_get_perm_addr,
};
static int netdev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
only in patch2:
unchanged:
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -270,8 +270,6 @@ u32 ethtool_op_get_sg(struct net_device *dev);
int ethtool_op_set_sg(struct net_device *dev, u32 data);
u32 ethtool_op_get_tso(struct net_device *dev);
int ethtool_op_set_tso(struct net_device *dev, u32 data);
-int ethtool_op_get_perm_addr(struct net_device *dev,
- struct ethtool_perm_addr *addr, u8 *data);
u32 ethtool_op_get_ufo(struct net_device *dev);
int ethtool_op_set_ufo(struct net_device *dev, u32 data);
@@ -309,7 +307,6 @@ int ethtool_op_set_ufo(struct net_device *dev, u32 data);
* get_strings: Return a set of strings that describe the requested objects
* phys_id: Identify the device
* get_stats: Return statistics about the device
- * get_perm_addr: Gets the permanent hardware address
*
* Description:
*
@@ -368,7 +365,6 @@ struct ethtool_ops {
int (*phys_id)(struct net_device *, u32);
int (*get_stats_count)(struct net_device *);
void (*get_ethtool_stats)(struct net_device *, struct ethtool_stats *, u64 *);
- int (*get_perm_addr)(struct net_device *, struct ethtool_perm_addr *, u8 *);
int (*begin)(struct net_device *);
void (*complete)(struct net_device *);
u32 (*get_ufo)(struct net_device *);
--
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
^ permalink raw reply
* [PATCH 2.6.23 1/2] Make the iw_cxgb3 module parameters writable.
From: Steve Wise @ 2007-07-29 20:12 UTC (permalink / raw)
To: rdreier; +Cc: general, linux-kernel, netdev
Make the iw_cxgb3 module parameters writable.
Signed-off-by: Steve Wise <swise@opengridcomputing.com>
---
drivers/infiniband/hw/cxgb3/iwch_cm.c | 16 ++++++++--------
1 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/infiniband/hw/cxgb3/iwch_cm.c b/drivers/infiniband/hw/cxgb3/iwch_cm.c
index 9574088..fa95dce 100644
--- a/drivers/infiniband/hw/cxgb3/iwch_cm.c
+++ b/drivers/infiniband/hw/cxgb3/iwch_cm.c
@@ -63,37 +63,37 @@ static char *states[] = {
};
static int ep_timeout_secs = 10;
-module_param(ep_timeout_secs, int, 0444);
+module_param(ep_timeout_secs, int, 0644);
MODULE_PARM_DESC(ep_timeout_secs, "CM Endpoint operation timeout "
"in seconds (default=10)");
static int mpa_rev = 1;
-module_param(mpa_rev, int, 0444);
+module_param(mpa_rev, int, 0644);
MODULE_PARM_DESC(mpa_rev, "MPA Revision, 0 supports amso1100, "
"1 is spec compliant. (default=1)");
static int markers_enabled = 0;
-module_param(markers_enabled, int, 0444);
+module_param(markers_enabled, int, 0644);
MODULE_PARM_DESC(markers_enabled, "Enable MPA MARKERS (default(0)=disabled)");
static int crc_enabled = 1;
-module_param(crc_enabled, int, 0444);
+module_param(crc_enabled, int, 0644);
MODULE_PARM_DESC(crc_enabled, "Enable MPA CRC (default(1)=enabled)");
static int rcv_win = 256 * 1024;
-module_param(rcv_win, int, 0444);
+module_param(rcv_win, int, 0644);
MODULE_PARM_DESC(rcv_win, "TCP receive window in bytes (default=256)");
static int snd_win = 32 * 1024;
-module_param(snd_win, int, 0444);
+module_param(snd_win, int, 0644);
MODULE_PARM_DESC(snd_win, "TCP send window in bytes (default=32KB)");
static unsigned int nocong = 0;
-module_param(nocong, uint, 0444);
+module_param(nocong, uint, 0644);
MODULE_PARM_DESC(nocong, "Turn off congestion control (default=0)");
static unsigned int cong_flavor = 1;
-module_param(cong_flavor, uint, 0444);
+module_param(cong_flavor, uint, 0644);
MODULE_PARM_DESC(cong_flavor, "TCP Congestion control flavor (default=1)");
static void process_work(struct work_struct *work);
^ permalink raw reply related
* [ofa-general] [PATCH 2.6.23 2/2] iw_cxgb3: Always call low level send function via cxgb3_ofld_send().
From: Steve Wise @ 2007-07-29 20:12 UTC (permalink / raw)
To: rdreier; +Cc: netdev, linux-kernel, general
In-Reply-To: <20070729201226.31659.85900.stgit@dell3.ogc.int>
iw_cxgb3: Always call low level send function via cxgb3_ofld_send().
Avoids deadlocks.
Signed-off-by: Steve Wise <swise@opengridcomputing.com>
---
drivers/infiniband/hw/cxgb3/iwch_cm.c | 16 ++++++++--------
1 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/infiniband/hw/cxgb3/iwch_cm.c b/drivers/infiniband/hw/cxgb3/iwch_cm.c
index fa95dce..20ba372 100644
--- a/drivers/infiniband/hw/cxgb3/iwch_cm.c
+++ b/drivers/infiniband/hw/cxgb3/iwch_cm.c
@@ -139,7 +139,7 @@ static void release_tid(struct t3cdev *t
req->wr.wr_hi = htonl(V_WR_OP(FW_WROPCODE_FORWARD));
OPCODE_TID(req) = htonl(MK_OPCODE_TID(CPL_TID_RELEASE, hwtid));
skb->priority = CPL_PRIORITY_SETUP;
- tdev->send(tdev, skb);
+ cxgb3_ofld_send(tdev, skb);
return;
}
@@ -161,7 +161,7 @@ int iwch_quiesce_tid(struct iwch_ep *ep)
req->val = cpu_to_be64(1 << S_TCB_RX_QUIESCE);
skb->priority = CPL_PRIORITY_DATA;
- ep->com.tdev->send(ep->com.tdev, skb);
+ cxgb3_ofld_send(ep->com.tdev, skb);
return 0;
}
@@ -183,7 +183,7 @@ int iwch_resume_tid(struct iwch_ep *ep)
req->val = 0;
skb->priority = CPL_PRIORITY_DATA;
- ep->com.tdev->send(ep->com.tdev, skb);
+ cxgb3_ofld_send(ep->com.tdev, skb);
return 0;
}
@@ -784,7 +784,7 @@ static int update_rx_credits(struct iwch
OPCODE_TID(req) = htonl(MK_OPCODE_TID(CPL_RX_DATA_ACK, ep->hwtid));
req->credit_dack = htonl(V_RX_CREDITS(credits) | V_RX_FORCE_ACK(1));
skb->priority = CPL_PRIORITY_ACK;
- ep->com.tdev->send(ep->com.tdev, skb);
+ cxgb3_ofld_send(ep->com.tdev, skb);
return credits;
}
@@ -1152,7 +1152,7 @@ static int listen_start(struct iwch_list
req->opt1 = htonl(V_CONN_POLICY(CPL_CONN_POLICY_ASK));
skb->priority = 1;
- ep->com.tdev->send(ep->com.tdev, skb);
+ cxgb3_ofld_send(ep->com.tdev, skb);
return 0;
}
@@ -1186,7 +1186,7 @@ static int listen_stop(struct iwch_liste
req->cpu_idx = 0;
OPCODE_TID(req) = htonl(MK_OPCODE_TID(CPL_CLOSE_LISTSRV_REQ, ep->stid));
skb->priority = 1;
- ep->com.tdev->send(ep->com.tdev, skb);
+ cxgb3_ofld_send(ep->com.tdev, skb);
return 0;
}
@@ -1264,7 +1264,7 @@ static void reject_cr(struct t3cdev *tde
rpl->opt0l_status = htonl(CPL_PASS_OPEN_REJECT);
rpl->opt2 = 0;
rpl->rsvd = rpl->opt2;
- tdev->send(tdev, skb);
+ cxgb3_ofld_send(tdev, skb);
}
}
@@ -1557,7 +1557,7 @@ static int peer_abort(struct t3cdev *tde
rpl->wr.wr_lo = htonl(V_WR_TID(ep->hwtid));
OPCODE_TID(rpl) = htonl(MK_OPCODE_TID(CPL_ABORT_RPL, ep->hwtid));
rpl->cmd = CPL_ABORT_NO_RST;
- ep->com.tdev->send(ep->com.tdev, rpl_skb);
+ cxgb3_ofld_send(ep->com.tdev, rpl_skb);
if (state != ABORTING) {
state_set(&ep->com, DEAD);
release_ep_resources(ep);
^ permalink raw reply related
* Re: [PATCH 4/7] Add "depth".
From: Corey Hickey @ 2007-07-29 20:21 UTC (permalink / raw)
To: Michael Buesch; +Cc: Linux Netdev List
In-Reply-To: <200707292041.45495.mb@bu3sch.de>
Michael Buesch wrote:
> On Sunday 29 July 2007 09:08:51 Corey Hickey wrote:
>> p = d;
>> n = q->dep[d].next;
>> @@ -215,7 +216,7 @@ static unsigned int sfq_drop(struct Qdisc *sch)
>> drop a packet from it */
>>
>> if (d > 1) {
>> - sfq_index x = q->dep[d+SFQ_DEPTH].next;
>> + sfq_index x = q->dep[d+q->depth].next;
>
> Please q->dep[d + q->depth]
> Makes it _much_ more readable. And doesn't confuse my brain with a
> minus and a BiggerThan sign ;)
Ok.
>> @@ -383,6 +384,16 @@ static void sfq_perturbation(unsigned long arg)
>> static void sfq_q_destroy(struct sfq_sched_data *q)
>> {
>> del_timer(&q->perturb_timer);
>> + if(q->dep)
>> + kfree(q->dep);
>> + if(q->next)
>> + kfree(q->next);
>> + if(q->allot)
>> + kfree(q->allot);
>> + if(q->hash)
>> + kfree(q->hash);
>> + if(q->qs)
>> + kfree(q->qs);
>
> No need to check for !=NULL. kfree handles NULL.
Ok. Thanks.
>> }
>>
>> static void sfq_destroy(struct Qdisc *sch)
>> @@ -394,6 +405,7 @@ static void sfq_destroy(struct Qdisc *sch)
>> static int sfq_q_init(struct sfq_sched_data *q, struct rtattr *opt)
>> {
>> struct tc_sfq_qopt *ctl = RTA_DATA(opt);
>> + sfq_index p = ~0U/2;
>> int i;
>>
>> if (opt && opt->rta_len < RTA_LENGTH(sizeof(*ctl)))
>> @@ -401,30 +413,53 @@ static int sfq_q_init(struct sfq_sched_data *q, struct rtattr *opt)
>>
>> q->perturbation = 0;
>> q->max_depth = 0;
>> - q->tail = q->limit = SFQ_DEPTH;
>> if (opt == NULL) {
>> q->perturb_period = 0;
>> + q->tail = q->limit = q->depth = SFQ_DEPTH_DEFAULT;
>> } else {
>> struct tc_sfq_qopt *ctl = RTA_DATA(opt);
>> if (ctl->quantum)
>> q->quantum = ctl->quantum;
>> q->perturb_period = ctl->perturb_period*HZ;
>> + q->tail = q->limit = q->depth = ctl->flows ? : SFQ_DEPTH_DEFAULT;
>> +
>> + if (q->depth > p - 1)
>> + return -EINVAL;
>
> Compare depth against (~0U/2)-1? What's that doing? Should probably add a comment.
~0U/2 - 1 is the maximum value depth can be, based on how it is used in
indexing q->dep. I agree, though, that deserves a comment. Actually,
I'll also change it to '#define SFQ_DEPTH_MAX (~0U/2 - 1)' and put it
near the top of the file next to the 'typedef unsigned int sfq_index;'.
I could also include limits.h and use UINT_MAX instead of ~0U; would
that be preferable?
>>
>> if (ctl->limit)
>> - q->limit = min_t(u32, ctl->limit, SFQ_DEPTH);
>> + q->limit = min_t(u32, ctl->limit, q->depth);
>> }
>>
>> + q->dep = kmalloc((1+q->depth*2)*sizeof(struct sfq_head), GFP_KERNEL);
>> + if (!q->dep)
>> + goto err_case;
>> + q->next = kmalloc(q->depth*sizeof(sfq_index), GFP_KERNEL);
>> + if (!q->next)
>> + goto err_case;
>> + q->allot = kmalloc(q->depth*sizeof(short), GFP_KERNEL);
>> + if (!q->allot)
>> + goto err_case;
>> + q->hash = kmalloc(q->depth*sizeof(unsigned short), GFP_KERNEL);
>> + if (!q->hash)
>> + goto err_case;
>> + q->qs = kmalloc(q->depth*sizeof(struct sk_buff_head), GFP_KERNEL);
>> + if (!q->qs)
>> + goto err_case;
>
> You may chose to use kcalloc for array allocations.
The arrays in the original code don't get zeroed either, so that
shouldn't be necessary (and I haven't heard of any problems so far). Do
you suggest I use kcalloc() anyway, just as a good practice?
>> for (i=0; i<SFQ_HASH_DIVISOR; i++)
>> - q->ht[i] = SFQ_DEPTH;
>> - for (i=0; i<SFQ_DEPTH; i++) {
>> + q->ht[i] = q->depth;
>> + for (i=0; i<q->depth; i++) {
>> skb_queue_head_init(&q->qs[i]);
>> - q->dep[i+SFQ_DEPTH].next = i+SFQ_DEPTH;
>> - q->dep[i+SFQ_DEPTH].prev = i+SFQ_DEPTH;
>> + q->dep[i+q->depth].next = i+q->depth;
>> + q->dep[i+q->depth].prev = i+q->depth;
>> }
>>
>> - for (i=0; i<SFQ_DEPTH; i++)
>> + for (i=0; i<q->depth; i++)
>> sfq_link(q, i);
>> return 0;
>> +err_case:
>
> This leaks a few kmallocs.
Are you saying that the 'err_case:' leaks kmallocs? It calls
sfq_q_destroy(q), which kfrees each of the arrays: dep, next, allot,
hash, and qs. Is that sufficient, or am I missing something or
misunderstanding you?
>> + sfq_q_destroy(q);
>> + return -ENOBUFS;
>> }
Thank you for your review. Could you please clarify the questions I
have? I'll make, test, and submit a revision of this patch after that.
-Corey
^ permalink raw reply
* Re: [PATCH] ethtool_perm_addr only has one implementation
From: Kok, Auke @ 2007-07-29 20:39 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: netdev
In-Reply-To: <20070729200815.GB21219@parisc-linux.org>
Matthew Wilcox wrote:
> All drivers implement ethtool get_perm_addr the same way -- by calling
> the generic function. So we can inline the generic function into the
> caller and avoid going through the drivers.
>
> Signed-off-by: Matthew Wilcox <matthew@wil.cx>
For the e100, e1000, ixgb parts:
Acked-by: Auke Kok <auke-jan.h.kok@intel.com>
>
> diff -u b/net/core/ethtool.c b/net/core/ethtool.c
> --- b/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -95,18 +95,6 @@
> return 0;
> }
>
> -int ethtool_op_get_perm_addr(struct net_device *dev, struct ethtool_perm_addr *addr, u8 *data)
> -{
> - unsigned char len = dev->addr_len;
> - if ( addr->size < len )
> - return -ETOOSMALL;
> -
> - addr->size = len;
> - memcpy(data, dev->perm_addr, len);
> - return 0;
> -}
> -
> -
> u32 ethtool_op_get_ufo(struct net_device *dev)
> {
> return (dev->features & NETIF_F_UFO) != 0;
> @@ -779,34 +767,20 @@
> static int ethtool_get_perm_addr(struct net_device *dev, void __user *useraddr)
> {
> struct ethtool_perm_addr epaddr;
> - u8 *data;
> - int ret;
>
> - if (!dev->ethtool_ops->get_perm_addr)
> - return -EOPNOTSUPP;
> -
> - if (copy_from_user(&epaddr,useraddr,sizeof(epaddr)))
> + if (copy_from_user(&epaddr, useraddr, sizeof(epaddr)))
> return -EFAULT;
>
> - data = kmalloc(epaddr.size, GFP_USER);
> - if (!data)
> - return -ENOMEM;
> -
> - ret = dev->ethtool_ops->get_perm_addr(dev,&epaddr,data);
> - if (ret)
> - return ret;
> + if (epaddr.size < dev->addr_len)
> + return -ETOOSMALL;
> + epaddr.size = dev->addr_len;
>
> - ret = -EFAULT;
> if (copy_to_user(useraddr, &epaddr, sizeof(epaddr)))
> - goto out;
> + return -EFAULT;
> useraddr += sizeof(epaddr);
> - if (copy_to_user(useraddr, data, epaddr.size))
> - goto out;
> - ret = 0;
> -
> - out:
> - kfree(data);
> - return ret;
> + if (copy_to_user(useraddr, dev->perm_addr, epaddr.size))
> + return -EFAULT;
> + return 0;
> }
>
> /* The main entry point in this file. Called from net/core/dev.c */
> @@ -976,7 +950,6 @@
>
> EXPORT_SYMBOL(dev_ethtool);
> EXPORT_SYMBOL(ethtool_op_get_link);
> -EXPORT_SYMBOL_GPL(ethtool_op_get_perm_addr);
> EXPORT_SYMBOL(ethtool_op_get_sg);
> EXPORT_SYMBOL(ethtool_op_get_tso);
> EXPORT_SYMBOL(ethtool_op_get_tx_csum);
> only in patch2:
> unchanged:
> --- a/drivers/net/3c59x.c
> +++ b/drivers/net/3c59x.c
> @@ -2886,7 +2886,6 @@ static const struct ethtool_ops vortex_ethtool_ops = {
> .set_settings = vortex_set_settings,
> .get_link = ethtool_op_get_link,
> .nway_reset = vortex_nway_reset,
> - .get_perm_addr = ethtool_op_get_perm_addr,
> };
>
> #ifdef CONFIG_PCI
> only in patch2:
> unchanged:
> --- a/drivers/net/8139cp.c
> +++ b/drivers/net/8139cp.c
> @@ -1578,7 +1578,6 @@ static const struct ethtool_ops cp_ethtool_ops = {
> .set_wol = cp_set_wol,
> .get_strings = cp_get_strings,
> .get_ethtool_stats = cp_get_ethtool_stats,
> - .get_perm_addr = ethtool_op_get_perm_addr,
> .get_eeprom_len = cp_get_eeprom_len,
> .get_eeprom = cp_get_eeprom,
> .set_eeprom = cp_set_eeprom,
> only in patch2:
> unchanged:
> --- a/drivers/net/8139too.c
> +++ b/drivers/net/8139too.c
> @@ -2452,7 +2452,6 @@ static const struct ethtool_ops rtl8139_ethtool_ops = {
> .get_strings = rtl8139_get_strings,
> .get_stats_count = rtl8139_get_stats_count,
> .get_ethtool_stats = rtl8139_get_ethtool_stats,
> - .get_perm_addr = ethtool_op_get_perm_addr,
> };
>
> static int netdev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
> only in patch2:
> unchanged:
> --- a/drivers/net/ax88796.c
> +++ b/drivers/net/ax88796.c
> @@ -580,7 +580,6 @@ static const struct ethtool_ops ax_ethtool_ops = {
> .set_settings = ax_set_settings,
> .nway_reset = ax_nway_reset,
> .get_link = ax_get_link,
> - .get_perm_addr = ethtool_op_get_perm_addr,
> };
>
> /* setup code */
> only in patch2:
> unchanged:
> --- a/drivers/net/b44.c
> +++ b/drivers/net/b44.c
> @@ -2033,7 +2033,6 @@ static const struct ethtool_ops b44_ethtool_ops = {
> .get_strings = b44_get_strings,
> .get_stats_count = b44_get_stats_count,
> .get_ethtool_stats = b44_get_ethtool_stats,
> - .get_perm_addr = ethtool_op_get_perm_addr,
> };
>
> static int b44_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
> only in patch2:
> unchanged:
> --- a/drivers/net/bnx2.c
> +++ b/drivers/net/bnx2.c
> @@ -6269,7 +6269,6 @@ static const struct ethtool_ops bnx2_ethtool_ops = {
> .phys_id = bnx2_phys_id,
> .get_stats_count = bnx2_get_stats_count,
> .get_ethtool_stats = bnx2_get_ethtool_stats,
> - .get_perm_addr = ethtool_op_get_perm_addr,
> };
>
> /* Called with rtnl_lock */
> only in patch2:
> unchanged:
> --- a/drivers/net/cxgb3/cxgb3_main.c
> +++ b/drivers/net/cxgb3/cxgb3_main.c
> @@ -1583,7 +1583,6 @@ static const struct ethtool_ops cxgb_ethtool_ops = {
> .get_wol = get_wol,
> .get_tso = ethtool_op_get_tso,
> .set_tso = ethtool_op_set_tso,
> - .get_perm_addr = ethtool_op_get_perm_addr
> };
>
> static int in_range(int val, int lo, int hi)
> only in patch2:
> unchanged:
> --- a/drivers/net/e100.c
> +++ b/drivers/net/e100.c
> @@ -2506,7 +2506,6 @@ static const struct ethtool_ops e100_ethtool_ops = {
> .phys_id = e100_phys_id,
> .get_stats_count = e100_get_stats_count,
> .get_ethtool_stats = e100_get_ethtool_stats,
> - .get_perm_addr = ethtool_op_get_perm_addr,
> };
>
> static int e100_do_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
> only in patch2:
> unchanged:
> --- a/drivers/net/e1000/e1000_ethtool.c
> +++ b/drivers/net/e1000/e1000_ethtool.c
> @@ -1973,7 +1973,6 @@ static const struct ethtool_ops e1000_ethtool_ops = {
> .phys_id = e1000_phys_id,
> .get_stats_count = e1000_get_stats_count,
> .get_ethtool_stats = e1000_get_ethtool_stats,
> - .get_perm_addr = ethtool_op_get_perm_addr,
> };
>
> void e1000_set_ethtool_ops(struct net_device *netdev)
> only in patch2:
> unchanged:
> --- a/drivers/net/forcedeth.c
> +++ b/drivers/net/forcedeth.c
> @@ -4706,7 +4706,6 @@ static const struct ethtool_ops ops = {
> .get_regs_len = nv_get_regs_len,
> .get_regs = nv_get_regs,
> .nway_reset = nv_nway_reset,
> - .get_perm_addr = ethtool_op_get_perm_addr,
> .get_tso = ethtool_op_get_tso,
> .set_tso = nv_set_tso,
> .get_ringparam = nv_get_ringparam,
> only in patch2:
> unchanged:
> --- a/drivers/net/ixgb/ixgb_ethtool.c
> +++ b/drivers/net/ixgb/ixgb_ethtool.c
> @@ -724,7 +724,6 @@ static const struct ethtool_ops ixgb_ethtool_ops = {
> .phys_id = ixgb_phys_id,
> .get_stats_count = ixgb_get_stats_count,
> .get_ethtool_stats = ixgb_get_ethtool_stats,
> - .get_perm_addr = ethtool_op_get_perm_addr,
> };
>
> void ixgb_set_ethtool_ops(struct net_device *netdev)
> only in patch2:
> unchanged:
> --- a/drivers/net/ne2k-pci.c
> +++ b/drivers/net/ne2k-pci.c
> @@ -638,7 +638,6 @@ static const struct ethtool_ops ne2k_pci_ethtool_ops = {
> .get_drvinfo = ne2k_pci_get_drvinfo,
> .get_tx_csum = ethtool_op_get_tx_csum,
> .get_sg = ethtool_op_get_sg,
> - .get_perm_addr = ethtool_op_get_perm_addr,
> };
>
> static void __devexit ne2k_pci_remove_one (struct pci_dev *pdev)
> only in patch2:
> unchanged:
> --- a/drivers/net/netxen/netxen_nic_ethtool.c
> +++ b/drivers/net/netxen/netxen_nic_ethtool.c
> @@ -755,5 +755,4 @@ struct ethtool_ops netxen_nic_ethtool_ops = {
> .get_strings = netxen_nic_get_strings,
> .get_stats_count = netxen_nic_get_stats_count,
> .get_ethtool_stats = netxen_nic_get_ethtool_stats,
> - .get_perm_addr = ethtool_op_get_perm_addr,
> };
> only in patch2:
> unchanged:
> --- a/drivers/net/pcnet32.c
> +++ b/drivers/net/pcnet32.c
> @@ -1515,7 +1515,6 @@ static const struct ethtool_ops pcnet32_ethtool_ops = {
> .phys_id = pcnet32_phys_id,
> .get_regs_len = pcnet32_get_regs_len,
> .get_regs = pcnet32_get_regs,
> - .get_perm_addr = ethtool_op_get_perm_addr,
> };
>
> /* only probes for non-PCI devices, the rest are handled by
> only in patch2:
> unchanged:
> --- a/drivers/net/qla3xxx.c
> +++ b/drivers/net/qla3xxx.c
> @@ -1904,7 +1904,6 @@ static void ql_get_pauseparam(struct net_device *ndev,
> static const struct ethtool_ops ql3xxx_ethtool_ops = {
> .get_settings = ql_get_settings,
> .get_drvinfo = ql_get_drvinfo,
> - .get_perm_addr = ethtool_op_get_perm_addr,
> .get_link = ethtool_op_get_link,
> .get_msglevel = ql_get_msglevel,
> .set_msglevel = ql_set_msglevel,
> only in patch2:
> unchanged:
> --- a/drivers/net/r8169.c
> +++ b/drivers/net/r8169.c
> @@ -1066,7 +1066,6 @@ static const struct ethtool_ops rtl8169_ethtool_ops = {
> .get_strings = rtl8169_get_strings,
> .get_stats_count = rtl8169_get_stats_count,
> .get_ethtool_stats = rtl8169_get_ethtool_stats,
> - .get_perm_addr = ethtool_op_get_perm_addr,
> };
>
> static void rtl8169_write_gmii_reg_bit(void __iomem *ioaddr, int reg,
> only in patch2:
> unchanged:
> --- a/drivers/net/sc92031.c
> +++ b/drivers/net/sc92031.c
> @@ -1402,7 +1402,6 @@ static struct ethtool_ops sc92031_ethtool_ops = {
> .get_strings = sc92031_ethtool_get_strings,
> .get_stats_count = sc92031_ethtool_get_stats_count,
> .get_ethtool_stats = sc92031_ethtool_get_ethtool_stats,
> - .get_perm_addr = ethtool_op_get_perm_addr,
> .get_ufo = ethtool_op_get_ufo,
> };
>
> only in patch2:
> unchanged:
> --- a/drivers/net/skge.c
> +++ b/drivers/net/skge.c
> @@ -821,7 +821,6 @@ static const struct ethtool_ops skge_ethtool_ops = {
> .phys_id = skge_phys_id,
> .get_stats_count = skge_get_stats_count,
> .get_ethtool_stats = skge_get_ethtool_stats,
> - .get_perm_addr = ethtool_op_get_perm_addr,
> };
>
> /*
> only in patch2:
> unchanged:
> --- a/drivers/net/sky2.c
> +++ b/drivers/net/sky2.c
> @@ -3548,7 +3548,6 @@ static const struct ethtool_ops sky2_ethtool_ops = {
> .phys_id = sky2_phys_id,
> .get_stats_count = sky2_get_stats_count,
> .get_ethtool_stats = sky2_get_ethtool_stats,
> - .get_perm_addr = ethtool_op_get_perm_addr,
> };
>
> #ifdef CONFIG_SKY2_DEBUG
> only in patch2:
> unchanged:
> --- a/drivers/net/sundance.c
> +++ b/drivers/net/sundance.c
> @@ -1586,7 +1586,6 @@ static const struct ethtool_ops ethtool_ops = {
> .get_link = get_link,
> .get_msglevel = get_msglevel,
> .set_msglevel = set_msglevel,
> - .get_perm_addr = ethtool_op_get_perm_addr,
> };
>
> static int netdev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
> only in patch2:
> unchanged:
> --- a/drivers/net/sunvnet.c
> +++ b/drivers/net/sunvnet.c
> @@ -906,7 +906,6 @@ static const struct ethtool_ops vnet_ethtool_ops = {
> .get_msglevel = vnet_get_msglevel,
> .set_msglevel = vnet_set_msglevel,
> .get_link = ethtool_op_get_link,
> - .get_perm_addr = ethtool_op_get_perm_addr,
> };
>
> static void vnet_port_free_tx_bufs(struct vnet_port *port)
> only in patch2:
> unchanged:
> --- a/drivers/net/tc35815.c
> +++ b/drivers/net/tc35815.c
> @@ -2198,7 +2198,6 @@ static const struct ethtool_ops tc35815_ethtool_ops = {
> .get_strings = tc35815_get_strings,
> .get_stats_count = tc35815_get_stats_count,
> .get_ethtool_stats = tc35815_get_ethtool_stats,
> - .get_perm_addr = ethtool_op_get_perm_addr,
> };
>
> static int tc35815_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
> only in patch2:
> unchanged:
> --- a/drivers/net/tg3.c
> +++ b/drivers/net/tg3.c
> @@ -9294,7 +9294,6 @@ static const struct ethtool_ops tg3_ethtool_ops = {
> .get_ethtool_stats = tg3_get_ethtool_stats,
> .get_coalesce = tg3_get_coalesce,
> .set_coalesce = tg3_set_coalesce,
> - .get_perm_addr = ethtool_op_get_perm_addr,
> };
>
> static void __devinit tg3_get_eeprom_size(struct tg3 *tp)
> only in patch2:
> unchanged:
> --- a/drivers/net/via-rhine.c
> +++ b/drivers/net/via-rhine.c
> @@ -1805,7 +1805,6 @@ static const struct ethtool_ops netdev_ethtool_ops = {
> .set_wol = rhine_set_wol,
> .get_sg = ethtool_op_get_sg,
> .get_tx_csum = ethtool_op_get_tx_csum,
> - .get_perm_addr = ethtool_op_get_perm_addr,
> };
>
> static int netdev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
> only in patch2:
> unchanged:
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -270,8 +270,6 @@ u32 ethtool_op_get_sg(struct net_device *dev);
> int ethtool_op_set_sg(struct net_device *dev, u32 data);
> u32 ethtool_op_get_tso(struct net_device *dev);
> int ethtool_op_set_tso(struct net_device *dev, u32 data);
> -int ethtool_op_get_perm_addr(struct net_device *dev,
> - struct ethtool_perm_addr *addr, u8 *data);
> u32 ethtool_op_get_ufo(struct net_device *dev);
> int ethtool_op_set_ufo(struct net_device *dev, u32 data);
>
> @@ -309,7 +307,6 @@ int ethtool_op_set_ufo(struct net_device *dev, u32 data);
> * get_strings: Return a set of strings that describe the requested objects
> * phys_id: Identify the device
> * get_stats: Return statistics about the device
> - * get_perm_addr: Gets the permanent hardware address
> *
> * Description:
> *
> @@ -368,7 +365,6 @@ struct ethtool_ops {
> int (*phys_id)(struct net_device *, u32);
> int (*get_stats_count)(struct net_device *);
> void (*get_ethtool_stats)(struct net_device *, struct ethtool_stats *, u64 *);
> - int (*get_perm_addr)(struct net_device *, struct ethtool_perm_addr *, u8 *);
> int (*begin)(struct net_device *);
> void (*complete)(struct net_device *);
> u32 (*get_ufo)(struct net_device *);
>
^ permalink raw reply
* Re: [PATCH] sb1000: prevent a potential NULL pointer dereference in sb1000_dev_ioctl()
From: Michael Buesch @ 2007-07-29 20:49 UTC (permalink / raw)
To: Satyam Sharma
Cc: Domen Puncer, Jesper Juhl, Linux Kernel Mailing List, netdev,
Steven Hirsch, David S. Miller, Stephen Hemminger
In-Reply-To: <alpine.LFD.0.999.0707300034460.30928@enigma.security.iitk.ac.in>
On Sunday 29 July 2007 21:09, Satyam Sharma wrote:
> Hi Michael,
>
>
> On Sun, 29 Jul 2007, Michael Buesch wrote:
>
> > On Sunday 29 July 2007 20:34:46 Satyam Sharma wrote:
> > > (2) !(dev->flags & IFF_UP) is bogus because the functions of this ioctl
> > > can (and should) be allowed even when the interface is not up and running.
> >
> > Are you _sure_? This function does poke with the device hardware.
> > It might return crap or even machinecheck when not initialized.
> > Hardware is probably powered down, if not IFF_UP. (I don't know if that's
> > the case here, though).
>
> IFF_UP checks if the _interface_ is up -- the hardware / card could still
> be powered up, but the interface down (ifconfing eth0 down or ip link set
> eth0 down).
Well, that is device/driver dependent and I don't know what's
the case for this driver. It's encouraged to shutdown hardware
completely (except the WOL parts) when the interface is down.
Dunno if this driver does it. But _if_ it does it, it could cause
problems to poke with the hardware while down.
^ permalink raw reply
* Re: [PATCH] SMSC LAN911x and LAN921x vendor driver
From: Peter Korsgaard @ 2007-07-29 20:53 UTC (permalink / raw)
To: Steve Glendinning
Cc: netdev, Ian Saturley, Bahadir Balban, Dustin Mcintire,
Bill Gatliff
In-Reply-To: <11846120522745-git-send-email-steve.glendinning@smsc.com>
>>>>> "Steve" == Steve Glendinning <steve.glendinning@smsc.com> writes:
Hi,
Steve> Attached is a driver for SMSC's LAN911x and LAN921x families
Steve> of embedded ethernet controllers.
Steve> There is an existing smc911x driver in the tree; this is intended to
Steve> replace it. Dustin McIntire (the author of the smc911x driver) has
Steve> expressed his support for switching to this driver.
What's the problem with Dustin's driver? It seems to work fine here
with a lan9117. Why not just add lan921x support to the existing
driver?
--
Bye, Peter Korsgaard
^ permalink raw reply
* Re: [PATCH 4/7] Add "depth".
From: Michael Buesch @ 2007-07-29 20:54 UTC (permalink / raw)
To: Corey Hickey; +Cc: Linux Netdev List
In-Reply-To: <46ACF6BB.7010703@fatooh.org>
On Sunday 29 July 2007 22:21, Corey Hickey wrote:
> > Compare depth against (~0U/2)-1? What's that doing? Should probably add a comment.
>
> ~0U/2 - 1 is the maximum value depth can be, based on how it is used in
> indexing q->dep. I agree, though, that deserves a comment. Actually,
> I'll also change it to '#define SFQ_DEPTH_MAX (~0U/2 - 1)' and put it
> near the top of the file next to the 'typedef unsigned int sfq_index;'.
>
> I could also include limits.h and use UINT_MAX instead of ~0U; would
> that be preferable?
Seems like a good idea.
> >>
> >> if (ctl->limit)
> >> - q->limit = min_t(u32, ctl->limit, SFQ_DEPTH);
> >> + q->limit = min_t(u32, ctl->limit, q->depth);
> >> }
> >>
> >> + q->dep = kmalloc((1+q->depth*2)*sizeof(struct sfq_head), GFP_KERNEL);
> >> + if (!q->dep)
> >> + goto err_case;
> >> + q->next = kmalloc(q->depth*sizeof(sfq_index), GFP_KERNEL);
> >> + if (!q->next)
> >> + goto err_case;
> >> + q->allot = kmalloc(q->depth*sizeof(short), GFP_KERNEL);
> >> + if (!q->allot)
> >> + goto err_case;
> >> + q->hash = kmalloc(q->depth*sizeof(unsigned short), GFP_KERNEL);
> >> + if (!q->hash)
> >> + goto err_case;
> >> + q->qs = kmalloc(q->depth*sizeof(struct sk_buff_head), GFP_KERNEL);
> >> + if (!q->qs)
> >> + goto err_case;
> >
> > You may chose to use kcalloc for array allocations.
>
> The arrays in the original code don't get zeroed either, so that
> shouldn't be necessary (and I haven't heard of any problems so far). Do
> you suggest I use kcalloc() anyway, just as a good practice?
Well, I think we don't have strict rules on that, so it depends
on the developer's taste. The advantage of kcalloc is, that it might
catch errors in the args better than this opencoded multiplication.
(There's some BUG_ON logic in kcalloc)
> >> for (i=0; i<SFQ_HASH_DIVISOR; i++)
> >> - q->ht[i] = SFQ_DEPTH;
> >> - for (i=0; i<SFQ_DEPTH; i++) {
> >> + q->ht[i] = q->depth;
> >> + for (i=0; i<q->depth; i++) {
> >> skb_queue_head_init(&q->qs[i]);
> >> - q->dep[i+SFQ_DEPTH].next = i+SFQ_DEPTH;
> >> - q->dep[i+SFQ_DEPTH].prev = i+SFQ_DEPTH;
> >> + q->dep[i+q->depth].next = i+q->depth;
> >> + q->dep[i+q->depth].prev = i+q->depth;
> >> }
> >>
> >> - for (i=0; i<SFQ_DEPTH; i++)
> >> + for (i=0; i<q->depth; i++)
> >> sfq_link(q, i);
> >> return 0;
> >> +err_case:
> >
> > This leaks a few kmallocs.
>
> Are you saying that the 'err_case:' leaks kmallocs? It calls
> sfq_q_destroy(q), which kfrees each of the arrays: dep, next, allot,
> hash, and qs. Is that sufficient, or am I missing something or
> misunderstanding you?
Ok, I didn't see that. So this should be ok.
> >> + sfq_q_destroy(q);
> >> + return -ENOBUFS;
> >> }
^ permalink raw reply
* Re: netdevice queueing / sendmsg issue?
From: Krzysztof Halasa @ 2007-07-29 21:03 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20070728.224959.74562336.davem@davemloft.net>
David Miller <davem@davemloft.net> writes:
> Software interrupts might be getting lost, dev_kfree_skb_irq() has to
> queue the kfree_skb() to soft IRQ.
>
> Therefore, dev_kfree_skb_irq() will only work properly from hardware
> interrupt context, where we will return and thus run the scheduled
> software interrupt.
>
> So some things to check out are whether the driver is invoking
> dev_kfree_skb_irq() in the right context, whether ARM might have some
> software interrupt processing preculiarity, etc.
I see. I call dev_kfree_skb_irq() from hardware IRQ handler, so
the main suspect is soft IRQ processing. Should be easy now.
Thanks.
--
Krzysztof Halasa
^ permalink raw reply
* IPUtils and uClibc
From: Rafał Bilski @ 2007-07-29 22:50 UTC (permalink / raw)
To: YOSHIFUJI Hideaki; +Cc: netdev
Hi,
Today I was trying to update my router based on Gentoo and uClibc.
Unfortunatly build fails because of b* functions. Linker can't
find them later. At first gcc is complaining that b* functions
are impilicity declared. Acording to man pages these functions
are deprecated anyway.
Patch is for Gentoo's iputils-20070202.
---
clockdiff.c | 4 ++--
ping.c | 13 +++++++------
rdisc.c | 6 +++---
traceroute6.c | 6 +++---
4 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/clockdiff.c b/clockdiff.c
index e17e0b8..ee7bb76 100644
--- a/clockdiff.c
+++ b/clockdiff.c
@@ -594,7 +594,7 @@ main(int argc, char *argv[])
memset(&server, 0, sizeof(server));
server.sin_family = hp->h_addrtype;
- bcopy(hp->h_addr, &(server.sin_addr.s_addr), 4);
+ memcpy(&(server.sin_addr.s_addr), hp->h_addr, 4);
if (connect(sock_raw, (struct sockaddr*)&server, sizeof(server)) == -1) {
perror("connect");
@@ -605,7 +605,7 @@ main(int argc, char *argv[])
socklen_t addrlen = sizeof(myaddr);
unsigned char rspace[ip_opt_len];
- bzero(rspace, sizeof(rspace));
+ memset(rspace, 0, sizeof(rspace));
rspace[0] = IPOPT_TIMESTAMP;
rspace[1] = ip_opt_len;
rspace[2] = 5;
diff --git a/ping.c b/ping.c
index 561e71a..6148fd9 100644
--- a/ping.c
+++ b/ping.c
@@ -58,6 +58,7 @@ char copyright[] =
* This program has to run SUID to ROOT to access the ICMP socket.
*/
+#include <string.h>
#include "ping_common.h"
#include <netinet/ip.h>
@@ -235,7 +236,7 @@ main(int argc, char **argv)
while (argc > 0) {
target = *argv;
- bzero((char *)&whereto, sizeof(whereto));
+ memset((char *)&whereto, 0, sizeof(whereto));
whereto.sin_family = AF_INET;
if (inet_aton(target, &whereto.sin_addr) == 1) {
hostname = target;
@@ -393,7 +394,7 @@ main(int argc, char **argv)
/* record route option */
if (options & F_RROUTE) {
- bzero(rspace, sizeof(rspace));
+ memset(rspace, 0, sizeof(rspace));
rspace[0] = IPOPT_NOP;
rspace[1+IPOPT_OPTVAL] = IPOPT_RR;
rspace[1+IPOPT_OLEN] = sizeof(rspace)-1;
@@ -405,7 +406,7 @@ main(int argc, char **argv)
}
}
if (options & F_TIMESTAMP) {
- bzero(rspace, sizeof(rspace));
+ memset(rspace, 0, sizeof(rspace));
rspace[0] = IPOPT_TIMESTAMP;
rspace[1] = (ts_type==IPOPT_TS_TSONLY ? 40 : 36);
rspace[2] = 5;
@@ -427,7 +428,7 @@ main(int argc, char **argv)
}
if (options & F_SOURCEROUTE) {
int i;
- bzero(rspace, sizeof(rspace));
+ memset(rspace, 0, sizeof(rspace));
rspace[0] = IPOPT_NOOP;
rspace[1+IPOPT_OPTVAL] = (options & F_SO_DONTROUTE) ? IPOPT_SSRR
: IPOPT_LSRR;
@@ -1009,7 +1010,7 @@ void pr_options(unsigned char * cp, int hlen)
if (i <= 0)
continue;
if (i == old_rrlen
- && !bcmp((char *)cp, old_rr, i)
+ && !strncmp((char *)cp, old_rr, i)
&& !(options & F_FLOOD)) {
printf("\t(same route)");
i = ((i + 3) / 4) * 4;
@@ -1017,7 +1018,7 @@ void pr_options(unsigned char * cp, int hlen)
break;
}
old_rrlen = i;
- bcopy((char *)cp, old_rr, i);
+ memcpy(old_rr, (char *)cp, i);
printf("\nRR: ");
cp++;
for (;;) {
diff --git a/rdisc.c b/rdisc.c
index bb223bc..b1eab40 100644
--- a/rdisc.c
+++ b/rdisc.c
@@ -409,11 +409,11 @@ next:
forever = 1;
}
- bzero( (char *)&whereto, sizeof(struct sockaddr_in) );
+ memset( (char *)&whereto, 0, sizeof(struct sockaddr_in) );
to->sin_family = AF_INET;
to->sin_addr.s_addr = inet_addr(sendaddress);
- bzero( (char *)&joinaddr, sizeof(struct sockaddr_in) );
+ memset( (char *)&joinaddr, 0, sizeof(struct sockaddr_in) );
joinaddr.sin_family = AF_INET;
joinaddr.sin_addr.s_addr = inet_addr(recvaddress);
@@ -1468,7 +1468,7 @@ rtioctl(struct in_addr addr, int op)
struct rtentry rt;
struct sockaddr_in *sin;
- bzero((char *)&rt, sizeof(struct rtentry));
+ memset((char *)&rt, 0, sizeof(struct rtentry));
rt.rt_dst.sa_family = AF_INET;
rt.rt_gateway.sa_family = AF_INET;
rt.rt_genmask.sa_family = AF_INET;
diff --git a/traceroute6.c b/traceroute6.c
index 114cb0a..2dd5c42 100644
--- a/traceroute6.c
+++ b/traceroute6.c
@@ -276,7 +276,7 @@ char copyright[] =
#define FD_SET(n, p) ((p)->fds_bits[(n)/NFDBITS] |= (1 << ((n) % NFDBITS)))
#define FD_CLR(n, p) ((p)->fds_bits[(n)/NFDBITS] &= ~(1 << ((n) % NFDBITS)))
#define FD_ISSET(n, p) ((p)->fds_bits[(n)/NFDBITS] & (1 << ((n) % NFDBITS)))
-#define FD_ZERO(p) bzero((char *)(p), sizeof(*(p)))
+#define FD_ZERO(p) memset((char *)(p), 0, sizeof(*(p)))
#endif
#define Fprintf (void)fprintf
@@ -422,7 +422,7 @@ int main(int argc, char *argv[])
setlinebuf (stdout);
- (void) bzero((char *)&whereto, sizeof(whereto));
+ (void) memset((char *)&whereto, 0, sizeof(whereto));
to->sin6_family = AF_INET6;
to->sin6_port = htons(port);
@@ -534,7 +534,7 @@ int main(int argc, char *argv[])
saddr.sin6_port = 0;
close(probe_fd);
} else {
- (void) bzero((char *)&saddr, sizeof(saddr));
+ (void) memset((char *)&saddr, 0, sizeof(saddr));
saddr.sin6_family = AF_INET6;
if (inet_pton(AF_INET6, source, &saddr.sin6_addr) <= 0)
{
--
----------------------------------------------------------------------
Wszystko czego potrzebujesz latem: kremy do opalania,
stroje kapielowe, maly romans
>>>http://link.interia.pl/f1b15
^ permalink raw reply related
* [PATCH 3/7] Move two functions.
From: Corey Hickey @ 2007-07-30 0:21 UTC (permalink / raw)
To: netdev; +Cc: Corey Hickey
In-Reply-To: <11857548771998-git-send-email-bugfood-ml@fatooh.org>
Move sfq_q_destroy() to above sfq_q_init() so that it can be used
by an error case in a later patch.
Move sfq_destroy() as well, for clarity.
Signed-off-by: Corey Hickey <bugfood-ml@fatooh.org>
---
net/sched/sch_sfq.c | 24 ++++++++++++------------
1 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index 0c46938..583f925 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -380,6 +380,17 @@ static void sfq_perturbation(unsigned long arg)
}
}
+static void sfq_q_destroy(struct sfq_sched_data *q)
+{
+ del_timer(&q->perturb_timer);
+}
+
+static void sfq_destroy(struct Qdisc *sch)
+{
+ struct sfq_sched_data *q = qdisc_priv(sch);
+ sfq_q_destroy(q);
+}
+
static int sfq_q_init(struct sfq_sched_data *q, struct rtattr *opt)
{
struct tc_sfq_qopt *ctl = RTA_DATA(opt);
@@ -420,7 +431,7 @@ static int sfq_init(struct Qdisc *sch, struct rtattr *opt)
{
struct sfq_sched_data *q = qdisc_priv(sch);
int err;
-
+
q->quantum = psched_mtu(sch->dev); /* default */
if ((err = sfq_q_init(q, opt)))
return err;
@@ -436,17 +447,6 @@ static int sfq_init(struct Qdisc *sch, struct rtattr *opt)
return 0;
}
-static void sfq_q_destroy(struct sfq_sched_data *q)
-{
- del_timer(&q->perturb_timer);
-}
-
-static void sfq_destroy(struct Qdisc *sch)
-{
- struct sfq_sched_data *q = qdisc_priv(sch);
- sfq_q_destroy(q);
-}
-
static int sfq_dump(struct Qdisc *sch, struct sk_buff *skb)
{
struct sfq_sched_data *q = qdisc_priv(sch);
--
1.5.2.4
^ permalink raw reply related
* [PATCH 2/7] Preparatory refactoring part 2.
From: Corey Hickey @ 2007-07-30 0:21 UTC (permalink / raw)
To: netdev; +Cc: Corey Hickey
In-Reply-To: <11857548771998-git-send-email-bugfood-ml@fatooh.org>
Factor code out of sfq_init() and sfq_destroy(), again so that the
new functions can be used by sfq_change() later.
Actually, as the diff itself shows, most of the sfq_q_init() code
comes from the original sfq_change(), but sfq_change() is only
called by sfq_init() right now. Thus, it is safe to remove
sfq_change(); "tc qdisc change" doesn't yet work for sfq anyway.
The sfq_destroy() --> sfq_q_destroy() change looks pointless here,
but it's cleaner to split now and add code to sfq_q_destroy() in a
later patch.
Signed-off-by: Corey Hickey <bugfood-ml@fatooh.org>
---
net/sched/sch_sfq.c | 80 +++++++++++++++++++++++++-------------------------
1 files changed, 40 insertions(+), 40 deletions(-)
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index 8ae077f..0c46938 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -380,71 +380,71 @@ static void sfq_perturbation(unsigned long arg)
}
}
-static int sfq_change(struct Qdisc *sch, struct rtattr *opt)
+static int sfq_q_init(struct sfq_sched_data *q, struct rtattr *opt)
{
- struct sfq_sched_data *q = qdisc_priv(sch);
struct tc_sfq_qopt *ctl = RTA_DATA(opt);
- unsigned int qlen;
+ int i;
- if (opt->rta_len < RTA_LENGTH(sizeof(*ctl)))
+ if (opt && opt->rta_len < RTA_LENGTH(sizeof(*ctl)))
return -EINVAL;
- sch_tree_lock(sch);
- q->quantum = ctl->quantum ? : psched_mtu(sch->dev);
- q->perturb_period = ctl->perturb_period*HZ;
- if (ctl->limit)
- q->limit = min_t(u32, ctl->limit, SFQ_DEPTH);
+ q->perturbation = 0;
+ q->max_depth = 0;
+ q->tail = q->limit = SFQ_DEPTH;
+ if (opt == NULL) {
+ q->perturb_period = 0;
+ } else {
+ struct tc_sfq_qopt *ctl = RTA_DATA(opt);
+ if (ctl->quantum)
+ q->quantum = ctl->quantum;
+ q->perturb_period = ctl->perturb_period*HZ;
- qlen = sch->q.qlen;
- while (sch->q.qlen >= q->limit-1)
- sfq_drop(sch);
- qdisc_tree_decrease_qlen(sch, qlen - sch->q.qlen);
+ if (ctl->limit)
+ q->limit = min_t(u32, ctl->limit, SFQ_DEPTH);
+ }
- del_timer(&q->perturb_timer);
- if (q->perturb_period) {
- q->perturb_timer.expires = jiffies + q->perturb_period;
- add_timer(&q->perturb_timer);
+ for (i=0; i<SFQ_HASH_DIVISOR; i++)
+ q->ht[i] = SFQ_DEPTH;
+ for (i=0; i<SFQ_DEPTH; i++) {
+ skb_queue_head_init(&q->qs[i]);
+ q->dep[i+SFQ_DEPTH].next = i+SFQ_DEPTH;
+ q->dep[i+SFQ_DEPTH].prev = i+SFQ_DEPTH;
}
- sch_tree_unlock(sch);
+
+ for (i=0; i<SFQ_DEPTH; i++)
+ sfq_link(q, i);
return 0;
}
static int sfq_init(struct Qdisc *sch, struct rtattr *opt)
{
struct sfq_sched_data *q = qdisc_priv(sch);
- int i;
+ int err;
+
+ q->quantum = psched_mtu(sch->dev); /* default */
+ if ((err = sfq_q_init(q, opt)))
+ return err;
init_timer(&q->perturb_timer);
q->perturb_timer.data = (unsigned long)sch;
q->perturb_timer.function = sfq_perturbation;
-
- for (i=0; i<SFQ_HASH_DIVISOR; i++)
- q->ht[i] = SFQ_DEPTH;
- for (i=0; i<SFQ_DEPTH; i++) {
- skb_queue_head_init(&q->qs[i]);
- q->dep[i+SFQ_DEPTH].next = i+SFQ_DEPTH;
- q->dep[i+SFQ_DEPTH].prev = i+SFQ_DEPTH;
- }
- q->limit = SFQ_DEPTH;
- q->max_depth = 0;
- q->tail = SFQ_DEPTH;
- if (opt == NULL) {
- q->quantum = psched_mtu(sch->dev);
- q->perturb_period = 0;
- } else {
- int err = sfq_change(sch, opt);
- if (err)
- return err;
+ if (q->perturb_period) {
+ q->perturb_timer.expires = jiffies + q->perturb_period;
+ add_timer(&q->perturb_timer);
}
- for (i=0; i<SFQ_DEPTH; i++)
- sfq_link(q, i);
+
return 0;
}
+static void sfq_q_destroy(struct sfq_sched_data *q)
+{
+ del_timer(&q->perturb_timer);
+}
+
static void sfq_destroy(struct Qdisc *sch)
{
struct sfq_sched_data *q = qdisc_priv(sch);
- del_timer(&q->perturb_timer);
+ sfq_q_destroy(q);
}
static int sfq_dump(struct Qdisc *sch, struct sk_buff *skb)
--
1.5.2.4
^ permalink raw reply related
* SFQ: backport some features from ESFQ (try 2)
From: Corey Hickey @ 2007-07-30 0:21 UTC (permalink / raw)
To: netdev
Hello,
Patchset try 2 addresses the review by Michael Buesch.
This set of patches adds some of ESFQ's modifications to the original
SFQ. Thus far, I have received support for this approach rather than for
trying to get ESFQ included as a separate qdisc.
http://mailman.ds9a.nl/pipermail/lartc/2007q2/021056.html
My patches here implement "tc qdisc change", user-configurable depth
(number of flows), and user-configurable divisor (for setting hash table
size). I've left out the remaining ESFQ features (usage of jhash and
different hashing methods) because Patrick McHardy intends to submit a
patch that will supersede that functionality; see the URL above.
Default values remain the same, and SFQ's default behavior remains the
same, so there should be no user disruption.
A patch for iproute2 is included after the end of the kernel patch series.
Thanks for your consideration,
Corey
include/linux/pkt_sched.h | 8 --
net/sched/sch_sfq.c | 296 ++++++++++++++++++++++++++++-----------------
2 files changed, 187 insertions(+), 117 deletions(-)
[PATCH 1/7] Preparatory refactoring part 1.
[PATCH 2/7] Preparatory refactoring part 2.
[PATCH 3/7] Move two functions.
[PATCH 4/7] Add "depth".
[PATCH 5/7] Add divisor.
[PATCH 6/7] Make qdisc changeable.
[PATCH 7/7] Remove comments about hardcoded values.
[PATCH] [iproute2] SFQ: Support changing depth and divisor.
^ permalink raw reply
* [PATCH 1/7] Preparatory refactoring part 1.
From: Corey Hickey @ 2007-07-30 0:21 UTC (permalink / raw)
To: netdev; +Cc: Corey Hickey
In-Reply-To: <11857548771998-git-send-email-bugfood-ml@fatooh.org>
Make a new function sfq_q_enqueue() that operates directly on the
queue data. This will be useful for implementing sfq_change() in
a later patch. A pleasant side-effect is reducing most of the
duplicate code in sfq_enqueue() and sfq_requeue().
Similarly, make a new function sfq_q_dequeue().
Signed-off-by: Corey Hickey <bugfood-ml@fatooh.org>
---
net/sched/sch_sfq.c | 70 ++++++++++++++++++++++++++------------------------
1 files changed, 36 insertions(+), 34 deletions(-)
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index 9579573..8ae077f 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -77,6 +77,9 @@
#define SFQ_DEPTH 128
#define SFQ_HASH_DIVISOR 1024
+#define SFQ_HEAD 0
+#define SFQ_TAIL 1
+
/* This type should contain at least SFQ_DEPTH*2 values */
typedef unsigned char sfq_index;
@@ -244,10 +247,8 @@ static unsigned int sfq_drop(struct Qdisc *sch)
return 0;
}
-static int
-sfq_enqueue(struct sk_buff *skb, struct Qdisc* sch)
+static void sfq_q_enqueue(struct sk_buff *skb, struct sfq_sched_data *q, unsigned int end)
{
- struct sfq_sched_data *q = qdisc_priv(sch);
unsigned hash = sfq_hash(q, skb);
sfq_index x;
@@ -256,8 +257,12 @@ sfq_enqueue(struct sk_buff *skb, struct Qdisc* sch)
q->ht[hash] = x = q->dep[SFQ_DEPTH].next;
q->hash[x] = hash;
}
- sch->qstats.backlog += skb->len;
- __skb_queue_tail(&q->qs[x], skb);
+
+ if (end == SFQ_TAIL)
+ __skb_queue_tail(&q->qs[x], skb);
+ else
+ __skb_queue_head(&q->qs[x], skb);
+
sfq_inc(q, x);
if (q->qs[x].qlen == 1) { /* The flow is new */
if (q->tail == SFQ_DEPTH) { /* It is the first flow */
@@ -270,12 +275,21 @@ sfq_enqueue(struct sk_buff *skb, struct Qdisc* sch)
q->tail = x;
}
}
+}
+
+static int
+sfq_enqueue(struct sk_buff *skb, struct Qdisc* sch)
+{
+ struct sfq_sched_data *q = qdisc_priv(sch);
+ sfq_q_enqueue(skb, q, SFQ_TAIL);
+ sch->qstats.backlog += skb->len;
if (++sch->q.qlen < q->limit-1) {
sch->bstats.bytes += skb->len;
sch->bstats.packets++;
return 0;
}
+ sch->qstats.drops++;
sfq_drop(sch);
return NET_XMIT_CN;
}
@@ -284,28 +298,8 @@ static int
sfq_requeue(struct sk_buff *skb, struct Qdisc* sch)
{
struct sfq_sched_data *q = qdisc_priv(sch);
- unsigned hash = sfq_hash(q, skb);
- sfq_index x;
-
- x = q->ht[hash];
- if (x == SFQ_DEPTH) {
- q->ht[hash] = x = q->dep[SFQ_DEPTH].next;
- q->hash[x] = hash;
- }
+ sfq_q_enqueue(skb, q, SFQ_HEAD);
sch->qstats.backlog += skb->len;
- __skb_queue_head(&q->qs[x], skb);
- sfq_inc(q, x);
- if (q->qs[x].qlen == 1) { /* The flow is new */
- if (q->tail == SFQ_DEPTH) { /* It is the first flow */
- q->tail = x;
- q->next[x] = x;
- q->allot[x] = q->quantum;
- } else {
- q->next[x] = q->next[q->tail];
- q->next[q->tail] = x;
- q->tail = x;
- }
- }
if (++sch->q.qlen < q->limit - 1) {
sch->qstats.requeues++;
return 0;
@@ -316,13 +310,8 @@ sfq_requeue(struct sk_buff *skb, struct Qdisc* sch)
return NET_XMIT_CN;
}
-
-
-
-static struct sk_buff *
-sfq_dequeue(struct Qdisc* sch)
+static struct sk_buff *sfq_q_dequeue(struct sfq_sched_data *q)
{
- struct sfq_sched_data *q = qdisc_priv(sch);
struct sk_buff *skb;
sfq_index a, old_a;
@@ -335,8 +324,6 @@ sfq_dequeue(struct Qdisc* sch)
/* Grab packet */
skb = __skb_dequeue(&q->qs[a]);
sfq_dec(q, a);
- sch->q.qlen--;
- sch->qstats.backlog -= skb->len;
/* Is the slot empty? */
if (q->qs[a].qlen == 0) {
@@ -353,6 +340,21 @@ sfq_dequeue(struct Qdisc* sch)
a = q->next[a];
q->allot[a] += q->quantum;
}
+
+ return skb;
+}
+
+static struct sk_buff
+*sfq_dequeue(struct Qdisc* sch)
+{
+ struct sfq_sched_data *q = qdisc_priv(sch);
+ struct sk_buff *skb;
+
+ skb = sfq_q_dequeue(q);
+ if (skb == NULL)
+ return NULL;
+ sch->q.qlen--;
+ sch->qstats.backlog -= skb->len;
return skb;
}
--
1.5.2.4
^ permalink raw reply related
* [PATCH 5/7] Add divisor.
From: Corey Hickey @ 2007-07-30 0:21 UTC (permalink / raw)
To: netdev; +Cc: Corey Hickey
In-Reply-To: <11857548771998-git-send-email-bugfood-ml@fatooh.org>
Make hash divisor user-configurable.
Signed-off-by: Corey Hickey <bugfood-ml@fatooh.org>
---
net/sched/sch_sfq.c | 17 ++++++++++++-----
1 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index 70124ac..e6a6a21 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -75,7 +75,7 @@
It is easy to increase these values, but not in flight. */
#define SFQ_DEPTH_DEFAULT 128
-#define SFQ_HASH_DIVISOR 1024
+#define SFQ_DIVISOR_DEFAULT 1024
#define SFQ_HEAD 0
#define SFQ_TAIL 1
@@ -98,6 +98,7 @@ struct sfq_sched_data
unsigned quantum; /* Allotment per round: MUST BE >= MTU */
int limit;
unsigned depth;
+ unsigned hash_divisor;
/* Variables */
struct timer_list perturb_timer;
@@ -105,7 +106,7 @@ struct sfq_sched_data
sfq_index tail; /* Index of current slot in round */
sfq_index max_depth; /* Maximal depth */
- sfq_index ht[SFQ_HASH_DIVISOR]; /* Hash table */
+ sfq_index *ht; /* Hash table */
sfq_index *next; /* Active slots link */
short *allot; /* Current allotment per slot */
unsigned short *hash; /* Hash value indexed by slots */
@@ -120,7 +121,7 @@ static __inline__ unsigned sfq_fold_hash(struct sfq_sched_data *q, u32 h, u32 h1
/* Have we any rotation primitives? If not, WHY? */
h ^= (h1<<pert) ^ (h1>>(0x1F - pert));
h ^= h>>10;
- return h & 0x3FF;
+ return h & (q->hash_divisor-1);
}
static unsigned sfq_hash(struct sfq_sched_data *q, struct sk_buff *skb)
@@ -386,6 +387,7 @@ static void sfq_perturbation(unsigned long arg)
static void sfq_q_destroy(struct sfq_sched_data *q)
{
del_timer(&q->perturb_timer);
+ kfree(q->ht);
kfree(q->dep);
kfree(q->next);
kfree(q->allot);
@@ -411,12 +413,14 @@ static int sfq_q_init(struct sfq_sched_data *q, struct rtattr *opt)
q->max_depth = 0;
if (opt == NULL) {
q->perturb_period = 0;
+ q->hash_divisor = SFQ_DIVISOR_DEFAULT;
q->tail = q->limit = q->depth = SFQ_DEPTH_DEFAULT;
} else {
struct tc_sfq_qopt *ctl = RTA_DATA(opt);
if (ctl->quantum)
q->quantum = ctl->quantum;
q->perturb_period = ctl->perturb_period*HZ;
+ q->hash_divisor = ctl->divisor ? : SFQ_DIVISOR_DEFAULT;
q->tail = q->limit = q->depth = ctl->flows ? : SFQ_DEPTH_DEFAULT;
if (q->depth > SFQ_MAX_DEPTH)
@@ -426,6 +430,9 @@ static int sfq_q_init(struct sfq_sched_data *q, struct rtattr *opt)
q->limit = min_t(u32, ctl->limit, q->depth);
}
+ q->ht = kcalloc(q->hash_divisor, sizeof(sfq_index), GFP_KERNEL);
+ if (!q->ht)
+ goto err_case;
q->dep = kcalloc(1 + q->depth*2, sizeof(struct sfq_head), GFP_KERNEL);
if (!q->dep)
goto err_case;
@@ -442,7 +449,7 @@ static int sfq_q_init(struct sfq_sched_data *q, struct rtattr *opt)
if (!q->qs)
goto err_case;
- for (i=0; i<SFQ_HASH_DIVISOR; i++)
+ for (i=0; i<q->hash_divisor; i++)
q->ht[i] = q->depth;
for (i=0; i < q->depth; i++) {
skb_queue_head_init(&q->qs[i]);
@@ -488,7 +495,7 @@ static int sfq_dump(struct Qdisc *sch, struct sk_buff *skb)
opt.perturb_period = q->perturb_period/HZ;
opt.limit = q->limit;
- opt.divisor = SFQ_HASH_DIVISOR;
+ opt.divisor = q->hash_divisor;
opt.flows = q->depth;
RTA_PUT(skb, TCA_OPTIONS, sizeof(opt), &opt);
--
1.5.2.4
^ permalink raw reply related
* [PATCH 6/7] Make qdisc changeable.
From: Corey Hickey @ 2007-07-30 0:21 UTC (permalink / raw)
To: netdev; +Cc: Corey Hickey
In-Reply-To: <11857548771998-git-send-email-bugfood-ml@fatooh.org>
Re-implement sfq_change() and enable Qdisc_opts.change so "tc qdisc
change" will work.
Signed-off-by: Corey Hickey <bugfood-ml@fatooh.org>
---
net/sched/sch_sfq.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 50 insertions(+), 1 deletions(-)
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index e6a6a21..e042cd0 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -485,6 +485,55 @@ static int sfq_init(struct Qdisc *sch, struct rtattr *opt)
return 0;
}
+static int sfq_change(struct Qdisc *sch, struct rtattr *opt)
+{
+ struct sfq_sched_data *q = qdisc_priv(sch);
+ struct sfq_sched_data tmp;
+ struct sk_buff *skb;
+ int err;
+
+ /* set up tmp queue */
+ memset(&tmp, 0, sizeof(struct sfq_sched_data));
+ tmp.quantum = psched_mtu(sch->dev); /* default */
+ if ((err = sfq_q_init(&tmp, opt)))
+ return err;
+
+ /* copy packets from the old queue to the tmp queue */
+ sch_tree_lock(sch);
+ while (sch->q.qlen >= tmp.limit - 1)
+ sfq_drop(sch);
+ while ((skb = sfq_q_dequeue(q)) != NULL)
+ sfq_q_enqueue(skb, &tmp, SFQ_TAIL);
+
+ /* clean up the old queue */
+ sfq_q_destroy(q);
+
+ /* copy elements of the tmp queue into the old queue */
+ q->perturb_period = tmp.perturb_period;
+ q->quantum = tmp.quantum;
+ q->limit = tmp.limit;
+ q->depth = tmp.depth;
+ q->hash_divisor = tmp.hash_divisor;
+ q->tail = tmp.tail;
+ q->max_depth = tmp.max_depth;
+ q->ht = tmp.ht;
+ q->dep = tmp.dep;
+ q->next = tmp.next;
+ q->allot = tmp.allot;
+ q->hash = tmp.hash;
+ q->qs = tmp.qs;
+
+ /* finish up */
+ if (q->perturb_period) {
+ q->perturb_timer.expires = jiffies + q->perturb_period;
+ add_timer(&q->perturb_timer);
+ } else {
+ q->perturbation = 0;
+ }
+ sch_tree_unlock(sch);
+ return 0;
+}
+
static int sfq_dump(struct Qdisc *sch, struct sk_buff *skb)
{
struct sfq_sched_data *q = qdisc_priv(sch);
@@ -519,7 +568,7 @@ static struct Qdisc_ops sfq_qdisc_ops = {
.init = sfq_init,
.reset = sfq_reset,
.destroy = sfq_destroy,
- .change = NULL,
+ .change = sfq_change,
.dump = sfq_dump,
.owner = THIS_MODULE,
};
--
1.5.2.4
^ permalink raw reply related
* [PATCH 4/7] Add "depth".
From: Corey Hickey @ 2007-07-30 0:21 UTC (permalink / raw)
To: netdev; +Cc: Corey Hickey
In-Reply-To: <11857548771998-git-send-email-bugfood-ml@fatooh.org>
Make "depth" (number of queues) user-configurable:
* replace #define with a parameter
* use old hardcoded value as a default
* kcalloc() arrays in sfq_q_init()
* free() arrays in sfq_q_destroy()
Signed-off-by: Corey Hickey <bugfood-ml@fatooh.org>
---
net/sched/sch_sfq.c | 81 +++++++++++++++++++++++++++++++++++----------------
1 files changed, 56 insertions(+), 25 deletions(-)
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index 583f925..70124ac 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -74,14 +74,16 @@
It is easy to increase these values, but not in flight. */
-#define SFQ_DEPTH 128
+#define SFQ_DEPTH_DEFAULT 128
#define SFQ_HASH_DIVISOR 1024
#define SFQ_HEAD 0
#define SFQ_TAIL 1
-/* This type should contain at least SFQ_DEPTH*2 values */
-typedef unsigned char sfq_index;
+/* This type must contain greater than depth*2 values, so depth is constrained
+ * accordingly. */
+typedef unsigned int sfq_index;
+#define SFQ_MAX_DEPTH (UINT_MAX / 2 - 1)
struct sfq_head
{
@@ -95,6 +97,7 @@ struct sfq_sched_data
int perturb_period;
unsigned quantum; /* Allotment per round: MUST BE >= MTU */
int limit;
+ unsigned depth;
/* Variables */
struct timer_list perturb_timer;
@@ -103,11 +106,11 @@ struct sfq_sched_data
sfq_index max_depth; /* Maximal depth */
sfq_index ht[SFQ_HASH_DIVISOR]; /* Hash table */
- sfq_index next[SFQ_DEPTH]; /* Active slots link */
- short allot[SFQ_DEPTH]; /* Current allotment per slot */
- unsigned short hash[SFQ_DEPTH]; /* Hash value indexed by slots */
- struct sk_buff_head qs[SFQ_DEPTH]; /* Slot queue */
- struct sfq_head dep[SFQ_DEPTH*2]; /* Linked list of slots, indexed by depth */
+ sfq_index *next; /* Active slots link */
+ short *allot; /* Current allotment per slot */
+ unsigned short *hash; /* Hash value indexed by slots */
+ struct sk_buff_head *qs; /* Slot queue */
+ struct sfq_head *dep; /* Linked list of slots, indexed by depth */
};
static __inline__ unsigned sfq_fold_hash(struct sfq_sched_data *q, u32 h, u32 h1)
@@ -164,7 +167,7 @@ static unsigned sfq_hash(struct sfq_sched_data *q, struct sk_buff *skb)
static inline void sfq_link(struct sfq_sched_data *q, sfq_index x)
{
sfq_index p, n;
- int d = q->qs[x].qlen + SFQ_DEPTH;
+ int d = q->qs[x].qlen + q->depth;
p = d;
n = q->dep[d].next;
@@ -215,7 +218,7 @@ static unsigned int sfq_drop(struct Qdisc *sch)
drop a packet from it */
if (d > 1) {
- sfq_index x = q->dep[d+SFQ_DEPTH].next;
+ sfq_index x = q->dep[d + q->depth].next;
skb = q->qs[x].prev;
len = skb->len;
__skb_unlink(skb, &q->qs[x]);
@@ -238,7 +241,7 @@ static unsigned int sfq_drop(struct Qdisc *sch)
kfree_skb(skb);
sfq_dec(q, d);
sch->q.qlen--;
- q->ht[q->hash[d]] = SFQ_DEPTH;
+ q->ht[q->hash[d]] = q->depth;
sch->qstats.drops++;
sch->qstats.backlog -= len;
return len;
@@ -253,8 +256,8 @@ static void sfq_q_enqueue(struct sk_buff *skb, struct sfq_sched_data *q, unsigne
sfq_index x;
x = q->ht[hash];
- if (x == SFQ_DEPTH) {
- q->ht[hash] = x = q->dep[SFQ_DEPTH].next;
+ if (x == q->depth) {
+ q->ht[hash] = x = q->dep[q->depth].next;
q->hash[x] = hash;
}
@@ -265,7 +268,7 @@ static void sfq_q_enqueue(struct sk_buff *skb, struct sfq_sched_data *q, unsigne
sfq_inc(q, x);
if (q->qs[x].qlen == 1) { /* The flow is new */
- if (q->tail == SFQ_DEPTH) { /* It is the first flow */
+ if (q->tail == q->depth) { /* It is the first flow */
q->tail = x;
q->next[x] = x;
q->allot[x] = q->quantum;
@@ -316,7 +319,7 @@ static struct sk_buff *sfq_q_dequeue(struct sfq_sched_data *q)
sfq_index a, old_a;
/* No active slots */
- if (q->tail == SFQ_DEPTH)
+ if (q->tail == q->depth)
return NULL;
a = old_a = q->next[q->tail];
@@ -327,10 +330,10 @@ static struct sk_buff *sfq_q_dequeue(struct sfq_sched_data *q)
/* Is the slot empty? */
if (q->qs[a].qlen == 0) {
- q->ht[q->hash[a]] = SFQ_DEPTH;
+ q->ht[q->hash[a]] = q->depth;
a = q->next[a];
if (a == old_a) {
- q->tail = SFQ_DEPTH;
+ q->tail = q->depth;
return skb;
}
q->next[q->tail] = a;
@@ -383,6 +386,11 @@ static void sfq_perturbation(unsigned long arg)
static void sfq_q_destroy(struct sfq_sched_data *q)
{
del_timer(&q->perturb_timer);
+ kfree(q->dep);
+ kfree(q->next);
+ kfree(q->allot);
+ kfree(q->hash);
+ kfree(q->qs);
}
static void sfq_destroy(struct Qdisc *sch)
@@ -401,30 +409,53 @@ static int sfq_q_init(struct sfq_sched_data *q, struct rtattr *opt)
q->perturbation = 0;
q->max_depth = 0;
- q->tail = q->limit = SFQ_DEPTH;
if (opt == NULL) {
q->perturb_period = 0;
+ q->tail = q->limit = q->depth = SFQ_DEPTH_DEFAULT;
} else {
struct tc_sfq_qopt *ctl = RTA_DATA(opt);
if (ctl->quantum)
q->quantum = ctl->quantum;
q->perturb_period = ctl->perturb_period*HZ;
+ q->tail = q->limit = q->depth = ctl->flows ? : SFQ_DEPTH_DEFAULT;
+
+ if (q->depth > SFQ_MAX_DEPTH)
+ return -EINVAL;
if (ctl->limit)
- q->limit = min_t(u32, ctl->limit, SFQ_DEPTH);
+ q->limit = min_t(u32, ctl->limit, q->depth);
}
+ q->dep = kcalloc(1 + q->depth*2, sizeof(struct sfq_head), GFP_KERNEL);
+ if (!q->dep)
+ goto err_case;
+ q->next = kcalloc(q->depth, sizeof(sfq_index), GFP_KERNEL);
+ if (!q->next)
+ goto err_case;
+ q->allot = kcalloc(q->depth, sizeof(short), GFP_KERNEL);
+ if (!q->allot)
+ goto err_case;
+ q->hash = kcalloc(q->depth, sizeof(unsigned short), GFP_KERNEL);
+ if (!q->hash)
+ goto err_case;
+ q->qs = kcalloc(q->depth, sizeof(struct sk_buff_head), GFP_KERNEL);
+ if (!q->qs)
+ goto err_case;
+
for (i=0; i<SFQ_HASH_DIVISOR; i++)
- q->ht[i] = SFQ_DEPTH;
- for (i=0; i<SFQ_DEPTH; i++) {
+ q->ht[i] = q->depth;
+ for (i=0; i < q->depth; i++) {
skb_queue_head_init(&q->qs[i]);
- q->dep[i+SFQ_DEPTH].next = i+SFQ_DEPTH;
- q->dep[i+SFQ_DEPTH].prev = i+SFQ_DEPTH;
+ q->dep[i + q->depth].next = i + q->depth;
+ q->dep[i + q->depth].prev = i + q->depth;
}
- for (i=0; i<SFQ_DEPTH; i++)
+ for (i=0; i < q->depth; i++)
sfq_link(q, i);
return 0;
+err_case:
+ sfq_q_destroy(q);
+ return -ENOBUFS;
}
static int sfq_init(struct Qdisc *sch, struct rtattr *opt)
@@ -458,7 +489,7 @@ static int sfq_dump(struct Qdisc *sch, struct sk_buff *skb)
opt.limit = q->limit;
opt.divisor = SFQ_HASH_DIVISOR;
- opt.flows = q->limit;
+ opt.flows = q->depth;
RTA_PUT(skb, TCA_OPTIONS, sizeof(opt), &opt);
--
1.5.2.4
^ permalink raw reply related
* [PATCH 7/7] Remove comments about hardcoded values.
From: Corey Hickey @ 2007-07-30 0:21 UTC (permalink / raw)
To: netdev; +Cc: Corey Hickey
In-Reply-To: <11857548771998-git-send-email-bugfood-ml@fatooh.org>
None of these are true anymore (hooray!).
Signed-off-by: Corey Hickey <bugfood-ml@fatooh.org>
---
include/linux/pkt_sched.h | 8 --------
net/sched/sch_sfq.c | 17 +++--------------
2 files changed, 3 insertions(+), 22 deletions(-)
diff --git a/include/linux/pkt_sched.h b/include/linux/pkt_sched.h
index 268c515..58a0ea6 100644
--- a/include/linux/pkt_sched.h
+++ b/include/linux/pkt_sched.h
@@ -148,14 +148,6 @@ struct tc_sfq_qopt
unsigned flows; /* Maximal number of flows */
};
-/*
- * NOTE: limit, divisor and flows are hardwired to code at the moment.
- *
- * limit=flows=128, divisor=1024;
- *
- * The only reason for this is efficiency, it is possible
- * to change these parameters in compile time.
- */
/* RED section */
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index e042cd0..3890452 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -61,18 +61,7 @@
We still need true WFQ for top level CSZ, but using WFQ
for the best effort traffic is absolutely pointless:
- SFQ is superior for this purpose.
-
- IMPLEMENTATION:
- This implementation limits maximal queue length to 128;
- maximal mtu to 2^15-1; number of hash buckets to 1024.
- The only goal of this restrictions was that all data
- fit into one 4K page :-). Struct sfq_sched_data is
- organized in anti-cache manner: all the data for a bucket
- are scattered over different locations. This is not good,
- but it allowed me to put it into 4K.
-
- It is easy to increase these values, but not in flight. */
+ SFQ is superior for this purpose. */
#define SFQ_DEPTH_DEFAULT 128
#define SFQ_DIVISOR_DEFAULT 1024
@@ -491,7 +480,7 @@ static int sfq_change(struct Qdisc *sch, struct rtattr *opt)
struct sfq_sched_data tmp;
struct sk_buff *skb;
int err;
-
+
/* set up tmp queue */
memset(&tmp, 0, sizeof(struct sfq_sched_data));
tmp.quantum = psched_mtu(sch->dev); /* default */
@@ -504,7 +493,7 @@ static int sfq_change(struct Qdisc *sch, struct rtattr *opt)
sfq_drop(sch);
while ((skb = sfq_q_dequeue(q)) != NULL)
sfq_q_enqueue(skb, &tmp, SFQ_TAIL);
-
+
/* clean up the old queue */
sfq_q_destroy(q);
--
1.5.2.4
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox