While I have nothing against the idea, the patch needs a lot
of cleanup. You seem to add almost as much ifdefed code to
dummy as it currently has, which makes me wonder if a completely
new device wouldn't be much cleaner.
jamal wrote:
diff --git a/drivers/net/dummy.c b/drivers/net/dummy.c
index dd8c15a..6c2a0bf 100644
--- a/drivers/net/dummy.c
+++ b/drivers/net/dummy.c
@@ -26,8 +26,15 @@
Nick Holloway, 27th May 1994
[I tweaked this explanation a little but that's all]
Alan Cox, 30th May 1994
+
+*/
+/*
+ * This driver isnt abused enough ;->
+ * Here to add only _just_ a _feeew more_ features,
+ * 10 years after AC added comment above ;-> hehe - JHS
*/
+
Useless newline
#include <linux/config.h>
#include <linux/module.h>
#include <linux/kernel.h>
@@ -35,11 +42,130 @@
#include <linux/etherdevice.h>
#include <linux/init.h>
#include <linux/moduleparam.h>
+#ifdef CONFIG_NET_CLS_ACT
+#include <net/pkt_sched.h>
+#endif
Just include in unconditionally instead of adding ugly ifdefs.
+
+#define TX_TIMEOUT (2*HZ)
+
+#define TX_Q_LIMIT 32
+struct dummy_private {
+ struct net_device_stats stats;
+#ifdef CONFIG_NET_CLS_ACT
+ struct tasklet_struct dummy_tasklet;
+ int tasklet_pending;
+ /* mostly debug stats leave in for now */
+ unsigned long stat_te; /* tasklet entered */
+ unsigned long stat_tqr; /* transmit queue refill attempt */
+ unsigned long stat_rqe; /* receive queue entered */
+ unsigned long stat_r2t; /* receive to trasmit transfers */
+ unsigned long stat_rqne; /*receiveQ not entered, tasklet resched */
+ unsigned long stat_rfe; /* received from egress path */
+ unsigned long stat_rfi; /* received from ingress path */
+ unsigned long stat_crq;
+ unsigned long stat_rqw;
I would prefer more descriptive names, like tasklet_entered,
txqueue_refill_attempt, ...
I also don't see a way to actually read these stats, except by
enabling debugging.
+ struct sk_buff_head rq;
+ struct sk_buff_head tq;
+#endif
+};
+
+#ifdef CONFIG_NET_CLS_ACT
+static void ri_tasklet(unsigned long dev);
+#endif
Please avoid the excessive use of ifdefs. Unused forward-declarations
don't hurt. It seems to be unnecessary anyway.
+
static int numdummies = 1;
static int dummy_xmit(struct sk_buff *skb, struct net_device *dev);
static struct net_device_stats *dummy_get_stats(struct net_device *dev);
+static void dummy_timeout(struct net_device *dev);
+static int dummy_open(struct net_device *dev);
+static int dummy_close(struct net_device *dev);
+
+static void dummy_timeout(struct net_device *dev) {
opening parenthesis of a function body go on a new line.
+
+ int cpu = smp_processor_id();
+
+ dev->trans_start = jiffies;
+ printk("%s: BUG tx timeout on CPU %d\n",dev->name,cpu);
+ if (spin_is_locked((&dev->xmit_lock)))
+ printk("xmit lock grabbed already\n");
+ if (spin_is_locked((&dev->queue_lock)))
+ printk("queue lock grabbed already\n");
+}
The watchdog always takes xmit_lock, and these "... lock grabbed
already" printks look totally useless.
+
+#ifdef CONFIG_NET_CLS_ACT
+static void ri_tasklet(unsigned long dev) {
+
+ struct net_device *dv = (struct net_device *)dev;
dev is a much nicer name than dv, please use __dev or something
for the unsigned long and dev for the struct netdevice *.
+ struct dummy_private *dp = ((struct net_device *)dev)->priv;
dummy currently uses netdev_priv(), please don't change that without
a reason.
+ struct net_device_stats *stats = &dp->stats;
+ struct sk_buff *skb = NULL;
No need to initialize to NULL, it is assigned a value right below.
+
+ dp->stat_te +=1;
+=<space>1, below you use ++ half of the time.
+ if (NULL == (skb = skb_peek(&dp->tq))) {
Please avoid these backward comparisions, especially if the
remaining file already does it the other way around.
+ dp->stat_tqr +=1;
+ if (spin_trylock(&dv->xmit_lock)) {
+ dp->stat_rqe +=1;
+ while (NULL != (skb = skb_dequeue(&dp->rq))) {
+ skb_queue_tail(&dp->tq, skb);
Looks like you want skb_move or something like that. Why do
you need this shoving skbs between queues anyway? You pull
them out of the queue again right below.
+ dp->stat_r2t +=1;
+ }
+ spin_unlock(&dv->xmit_lock);
+ } else {
+ /* reschedule */
+ dp->stat_rqne +=1;
+ goto resched;
+ }
+ }
+
+ while (NULL != (skb = skb_dequeue(&dp->tq))) {
+ __u32 from = G_TC_FROM(skb->tc_verd);
u32
+
+ skb->tc_verd = 0;
+ skb->tc_verd = SET_TC_NCLS(skb->tc_verd);
+ stats->tx_packets++;
+ stats->tx_bytes +=skb->len;
+ if (from & AT_EGRESS) {
+ dp->stat_rfe +=1;
+ dev_queue_xmit(skb);
+ } else if (from & AT_INGRESS) {
+
+ dp->stat_rfi +=1;
+ netif_rx(skb);
+ } else {
+ /* if netfilt is compiled in and packet is
+ tagged, we could reinject the packet back
+ this would make it do remaining 10%
+ of what current IMQ does
+ if someone really really insists then
+ this is the spot .. jhs */
+ dev_kfree_skb(skb);
+ stats->tx_dropped++;
Packets that are queued from netfilter must not be freed other
than by passing them to nf_reinject with a verdict of drop.
It doesn't look as if you were using netfilter at all, but
at least that comment is wrong.
+ }
+ }
+
+ if (spin_trylock(&dv->xmit_lock)) {
+ dp->stat_crq +=1;
+ if (NULL == (skb = skb_peek(&dp->rq))) {
+ dp->tasklet_pending = 0;
+ if (netif_queue_stopped(dv))
+ netif_wake_queue(dv);
+ //netif_start_queue(dv);
??
+ } else {
+ dp->stat_rqw +=1;
+ spin_unlock(&dv->xmit_lock);
+ goto resched;
+ }
+ spin_unlock(&dv->xmit_lock);
+ } else {
+resched:
+ dp->tasklet_pending = 1;
+ tasklet_schedule(&dp->dummy_tasklet);
+ }
+
+}
+#endif
static int dummy_set_address(struct net_device *dev, void *p)
{
@@ -62,12 +188,17 @@ static void __init dummy_setup(struct ne
/* Initialize the device structure. */
dev->get_stats = dummy_get_stats;
dev->hard_start_xmit = dummy_xmit;
+ dev->tx_timeout = &dummy_timeout;
+ dev->watchdog_timeo = TX_TIMEOUT;
+ dev->open = &dummy_open;
+ dev->stop = &dummy_close;
+
dev->set_multicast_list = set_multicast_list;
dev->set_mac_address = dummy_set_address;
/* Fill in device structure with ethernet-generic values. */
ether_setup(dev);
- dev->tx_queue_len = 0;
+ dev->tx_queue_len = TX_Q_LIMIT;
dev->change_mtu = NULL;
dev->flags |= IFF_NOARP;
dev->flags &= ~IFF_MULTICAST;
@@ -77,18 +208,64 @@ static void __init dummy_setup(struct ne
static int dummy_xmit(struct sk_buff *skb, struct net_device *dev)
{
- struct net_device_stats *stats = netdev_priv(dev);
+ struct dummy_private *dp = ((struct net_device *)dev)->priv;
+ struct net_device_stats *stats = &dp->stats;
+ int ret = 0;
+ {
stats->tx_packets++;
stats->tx_bytes+=skb->len;
+ }
+#ifdef CONFIG_NET_CLS_ACT
+ __u32 from = G_TC_FROM(skb->tc_verd);
u32
+ if (!from || !skb->input_dev ) {
unnecessary space before closing bracket
+dropped:
+ dev_kfree_skb(skb);
+ stats->rx_dropped++;
+ return ret;
+ } else {
+ if (skb->input_dev)
+ skb->dev = skb->input_dev;
+ else
+ printk("warning!!! no idev %s\n",skb->dev->name);
+
+ skb->input_dev = dev;
Is that still necessary and/or correct? We moved the input_dev
assignment to netif_receive_skb().
+ if (from & AT_INGRESS) {
+ skb_pull(skb, skb->dev->hard_header_len);
+ } else {
+ if (!(from & AT_EGRESS)) {
+ goto dropped;
+ }
+ }
+ }
+ if (skb_queue_len(&dp->rq) >= dev->tx_queue_len) {
+ netif_stop_queue(dev);
+ }
+ dev->trans_start = jiffies;
+ skb_queue_tail(&dp->rq, skb);
+ if (!dp->tasklet_pending) {
+ dp->tasklet_pending = 1;
+ tasklet_schedule(&dp->dummy_tasklet);
+ }
+#else
+ stats->rx_dropped++;
dev_kfree_skb(skb);
- return 0;
+#endif
+ return ret;
}
static struct net_device_stats *dummy_get_stats(struct net_device *dev)
{
- return netdev_priv(dev);
+ struct dummy_private *dp = ((struct net_device *)dev)->priv;
+ struct net_device_stats *stats = &dp->stats;
+
+ pr_debug("tasklets stats %ld:%ld:%ld:%ld:%ld:%ld:%ld:%ld:%ld \n",
+ dp->stat_rqne, dp->stat_rqw, dp->stat_crq,
+ dp->stat_te, dp->stat_tqr, dp->stat_rfe,
+ dp->stat_rfi,dp->stat_rqe, dp->stat_r2t);
+
+ return stats;
}
static struct net_device **dummies;
@@ -97,12 +274,42 @@ static struct net_device **dummies;
module_param(numdummies, int, 0);
MODULE_PARM_DESC(numdummies, "Number of dummy pseudo devices");
+static int dummy_close(struct net_device *dev)
+{
+
Please kill the above newline
+#ifdef CONFIG_NET_CLS_ACT
+ struct dummy_private *dp = ((struct net_device *)dev)->priv;
+
+ tasklet_kill(&dp->dummy_tasklet);
+ skb_queue_purge(&dp->rq);
+ skb_queue_purge(&dp->tq);
+#endif
+ netif_stop_queue(dev);
+ return 0;
+}
+
+static int dummy_open(struct net_device *dev)
+{
+
this one too
+#ifdef CONFIG_NET_CLS_ACT
+ struct dummy_private *dp = ((struct net_device *)dev)->priv;
+
+ tasklet_init(&dp->dummy_tasklet, ri_tasklet, (unsigned long)dev);
+ skb_queue_head_init(&dp->rq);
+ skb_queue_head_init(&dp->tq);
+#endif
+ netif_start_queue(dev);
+
+ return 0;
+}
+
+
this one too
static int __init dummy_init_one(int index)
{
struct net_device *dev_dummy;
int err;
- dev_dummy = alloc_netdev(sizeof(struct net_device_stats),
+ dev_dummy = alloc_netdev(sizeof(struct dummy_private),
"dummy%d", dummy_setup);
if (!dev_dummy)
@@ -128,6 +335,7 @@ static int __init dummy_init_module(void
{
int i, err = 0;
dummies = kmalloc(numdummies * sizeof(void *), GFP_KERNEL);
+
unrelated newline-change, more below.
if (!dummies)
return -ENOMEM;
for (i = 0; i < numdummies && !err; i++)
@@ -136,12 +344,14 @@ static int __init dummy_init_module(void
while (--i >= 0)
dummy_free_one(i);
}
+
return err;
}
static void __exit dummy_cleanup_module(void)
{
int i;
+
for (i = 0; i < numdummies; i++)
dummy_free_one(i);
kfree(dummies);
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
