Hi,
On 09/27/2017 02:14 PM, Vinicius Costa Gomes wrote: > Hi, > > Cong Wang <xiyou.wangc...@gmail.com> writes: > >> On Tue, Sep 26, 2017 at 4:39 PM, Vinicius Costa Gomes >> <vinicius.go...@intel.com> wrote: >>> +static int cbs_init(struct Qdisc *sch, struct nlattr *opt) >>> +{ >>> + struct cbs_sched_data *q = qdisc_priv(sch); >>> + struct net_device *dev = qdisc_dev(sch); >>> + >>> + if (!opt) >>> + return -EINVAL; >>> + >>> + /* FIXME: this means that we can only install this qdisc >>> + * "under" mqprio. Do we need a more generic way to retrieve >>> + * the queue, or do we pass the netdev_queue to the driver? >>> + */ >>> + q->queue = TC_H_MIN(sch->parent) - 1 - netdev_get_num_tc(dev); >>> + >>> + return cbs_change(sch, opt); >>> +} >> >> Yeah it is ugly to assume its parent is mqprio, at least you should >> error out if it is not the case. > > Will add an error for this, for now. > >> >> I am not sure how we can solve this elegantly, perhaps you should >> extend mqprio rather than add a new one? > > Is the alternative hinted in the FIXME worse? Instead of passing the > index of the hardware queue to the driver we pass the pointer to a > netdev_queue to the driver and it "discovers" the HW queue from that. What if we keep passing the index, but calculate it from the netdev_queue pointer instead? i.e.: q->queue = sch->dev_queue - netdev_get_tx_queue(dev, 0); At least it wouldn't rely on the root qdisc being of any specific type. Regards, Jesus