| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <467E6090.6070703@trash.net>
Date: Sun, 24 Jun 2007 14:16:16 +0200
From: Patrick McHardy <kaber@...sh.net>
To: PJ Waskiewicz <peter.p.waskiewicz.jr@...el.com>
CC: davem@...emloft.net, netdev@...r.kernel.org, jeff@...zik.org,
auke-jan.h.kok@...el.com, hadi@...erus.ca
Subject: Re: [PATCH 3/3] NET: [SCHED] Qdisc changes and sch_rr added for multiqueue
PJ Waskiewicz wrote:
> diff --git a/include/linux/pkt_sched.h b/include/linux/pkt_sched.h
> index 09808b7..ec3a9a5 100644
> --- a/include/linux/pkt_sched.h
> +++ b/include/linux/pkt_sched.h
> @@ -103,8 +103,8 @@ struct tc_prio_qopt
>
> enum
> {
> - TCA_PRIO_UNPSEC,
> - TCA_PRIO_TEST,
You misunderstood me. You can work on top of my compat attribute
patches, but the example code should not have to go in to apply
your patch.
> diff --git a/net/sched/Kconfig b/net/sched/Kconfig
> index 475df84..7f14fa6 100644
> --- a/net/sched/Kconfig
> +++ b/net/sched/Kconfig
> @@ -102,8 +102,16 @@ config NET_SCH_ATM
> To compile this code as a module, choose M here: the
> module will be called sch_atm.
>
> +config NET_SCH_BANDS
> + bool "Multi Band Queueing (PRIO and RR)"
This options seems useless. Its not used *anywhere* except for
dependencies.
> + ---help---
> + Say Y here if you want to use n-band multiqueue packet
> + schedulers. These include a priority-based scheduler and
> + a round-robin scheduler.
> +
> config NET_SCH_PRIO
> tristate "Multi Band Priority Queueing (PRIO)"
> + depends on NET_SCH_BANDS
And this dependency as well.
> ---help---
> Say Y here if you want to use an n-band priority queue packet
> scheduler.
> @@ -111,6 +119,28 @@ config NET_SCH_PRIO
> To compile this code as a module, choose M here: the
> module will be called sch_prio.
>
> +config NET_SCH_RR
> + tristate "Multi Band Round Robin Queuing (RR)"
> + depends on NET_SCH_BANDS
Same here. RR
> + select NET_SCH_PRIO
> + ---help---
> + Say Y here if you want to use an n-band round robin packet
> + scheduler.
> +
> + The module uses sch_prio for its framework and is aliased as
> + sch_rr, so it will load sch_prio, although it is referred
> + to using sch_rr.
> +
> +config NET_SCH_BANDS_MQ
> + bool "Multiple hardware queue support"
> + depends on NET_SCH_BANDS
OK, again:
Introduce NET_SCH_RR. NET_SCH_RR selects NET_SCH_PRIO. Nothing at
all changes for NET_SCH_PRIO itself. Additionally introduce a
boolean NET_SCH_MULTIQUEUE. No dependencies at all. Use
NET_SCH_MULTIQUEUE to guard the multiqueue code in sch_prio.c.
Your current code doesn't even have any ifdefs anymore though,
so this might not be needed at all.
Additionally you could later introduce E1000_MULTIQUEUE and
have that select NET_SCH_MULTIQUEUE.
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index 9461e8a..203d5c4 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -168,7 +168,8 @@ static inline int qdisc_restart(struct net_device *dev)
> spin_unlock(&dev->queue_lock);
>
> ret = NETDEV_TX_BUSY;
> - if (!netif_queue_stopped(dev))
> + if (!netif_queue_stopped(dev) &&
> + !netif_subqueue_stopped(dev, skb->queue_mapping))
> /* churn baby churn .. */
> ret = dev_hard_start_xmit(skb, dev);
I'll try again - please move this to patch 2/3.
> diff --git a/net/sched/sch_prio.c b/net/sched/sch_prio.c
> index 40a13e8..8a716f0 100644
> --- a/net/sched/sch_prio.c
> +++ b/net/sched/sch_prio.c
> @@ -40,9 +40,11 @@
> struct prio_sched_data
> {
> int bands;
> + int curband; /* for round-robin */
> struct tcf_proto *filter_list;
> u8 prio2band[TC_PRIO_MAX+1];
> struct Qdisc *queues[TCQ_PRIO_BANDS];
> + unsigned char mq;
> };
>
>
> @@ -70,14 +72,28 @@ prio_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr)
> #endif
> if (TC_H_MAJ(band))
> band = 0;
> + if (q->mq)
> + skb->queue_mapping =
> + q->prio2band[band&TC_PRIO_MAX];
> + else
> + skb->queue_mapping = 0;
Might look cleaner if you have one central point where queue_mapping is
set and the band is returned.
> + /* If we're multiqueue, make sure the number of incoming bands
> + * matches the number of queues on the device we're associating with.
> + */
> + if (tb[TCA_PRIO_MQ - 1])
> + q->mq = *(unsigned char *)RTA_DATA(tb[TCA_PRIO_MQ - 1]);
If you're using it as a flag, please use RTA_GET_FLAG(),
otherwise RTA_GET_U8.
> + if (q->mq && (qopt->bands != sch->dev->egress_subqueue_count))
> + return -EINVAL;
>
> sch_tree_lock(sch);
> q->bands = qopt->bands;
> @@ -280,7 +342,7 @@ static int prio_dump(struct Qdisc *sch, struct sk_buff *skb)
> memcpy(&opt.priomap, q->prio2band, TC_PRIO_MAX+1);
>
> nest = RTA_NEST_COMPAT(skb, TCA_OPTIONS, sizeof(opt), &opt);
> - RTA_PUT_U32(skb, TCA_PRIO_TEST, 321);
> + RTA_PUT_U8(skb, TCA_PRIO_MQ, q->mq);
And RTA_PUT_FLAG. Now that I think of it, does it even makes sense
to have a prio private flag for this instead of a qdisc global one?
> static int __init prio_module_init(void)
> {
> - return register_qdisc(&prio_qdisc_ops);
> + register_qdisc(&prio_qdisc_ops);
> + register_qdisc(&rr_qdisc_ops);
Proper error handling please.
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists