I think it's worth warning in all cases. You are going to get pretty messed up behavior if you don't have a default queue.
Ethan On Wed, Jul 20, 2011 at 12:30, Ben Pfaff <[email protected]> wrote: > Thanks. I pushed this. > > Thanks for investigating the differences between HTB and HFSC. I > didn't realize that they behaved differently. > > We can refine it to only warn for qdiscs that drop packets later, if > it seems warranted. > > On Wed, Jul 20, 2011 at 11:51:39AM -0700, Ethan Jackson wrote: >> Ok so I just tested it. HFSC drops the packets, HTB lets them >> through. Since we are encouraging use of HFSC I think it's best to >> assume that people will be using it. Patch looks good, thanks. >> >> Ethan >> >> On Wed, Jul 20, 2011 at 11:43, Ethan Jackson <[email protected]> wrote: >> > I haven't looked at the code, maybe the behavior has changed. ?I >> > definitely remember running into this with HTB at least. ?It certainly >> > didn't drop packets in that case. ?I'll try it out experimentally and >> > get back to you. >> > >> > Ethan >> > >> > On Wed, Jul 20, 2011 at 11:42, Ben Pfaff <[email protected]> wrote: >> >> I read hfsc_classify() in net/sched/sch_hfsc.c as first trying to find >> >> the correct class for a packet, then looking for a default class if >> >> there is none, then returning NULL if there is no default class. ?That >> >> function's caller in hfsc_enqueue() does a kfree_skb() and returns an >> >> error if hfsc_classify() returns NULL. ?So it looks to me like having >> >> no default queue (class) causes packet drops, am I missing anything? >> >> >> >> On Wed, Jul 20, 2011 at 11:36:57AM -0700, Ethan Jackson wrote: >> >>> I think the idea of warning is a good one. ?Last time I tried (months >> >>> ago), if you don't configure a default queue on the QoS types >> >>> supported by OVS currently (hfsc/htb), the behavior is not to drop >> >>> packets. ?Instead it simply egresses the link shrinking the available >> >>> bandwidth for the properly queued traffic. ?This can cause extremely >> >>> strange QoS behavior, but not packet drops. >> >>> >> >>> Ethan >> >>> >> >>> On Wed, Jul 20, 2011 at 10:40, Ben Pfaff <[email protected]> wrote: >> >>> > Queue 0 is documented as the "default queue" used when a packet is not >> >>> > directed to any specific queue. ?Many qdiscs drop packets not directed >> >>> > to a >> >>> > queue if the default queue is not configured. ?This is therefore >> >>> > likely to >> >>> > be a misconfiguration, so warn about it. >> >>> > --- >> >>> > ?vswitchd/bridge.c | ? 13 +++++++++++++ >> >>> > ?1 files changed, 13 insertions(+), 0 deletions(-) >> >>> > >> >>> > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c >> >>> > index 449957a..fcd7a78 100644 >> >>> > --- a/vswitchd/bridge.c >> >>> > +++ b/vswitchd/bridge.c >> >>> > @@ -2474,6 +2474,7 @@ iface_configure_qos(struct iface *iface, const >> >>> > struct ovsrec_qos *qos) >> >>> > ? ? } else { >> >>> > ? ? ? ? struct iface_delete_queues_cbdata cbdata; >> >>> > ? ? ? ? struct shash details; >> >>> > + ? ? ? ?bool queue_zero; >> >>> > ? ? ? ? size_t i; >> >>> > >> >>> > ? ? ? ? /* Configure top-level Qos for 'iface'. */ >> >>> > @@ -2489,16 +2490,28 @@ iface_configure_qos(struct iface *iface, const >> >>> > struct ovsrec_qos *qos) >> >>> > ? ? ? ? netdev_dump_queues(iface->netdev, iface_delete_queues, >> >>> > &cbdata); >> >>> > >> >>> > ? ? ? ? /* Configure queues for 'iface'. */ >> >>> > + ? ? ? ?queue_zero = false; >> >>> > ? ? ? ? for (i = 0; i < qos->n_queues; i++) { >> >>> > ? ? ? ? ? ? const struct ovsrec_queue *queue = qos->value_queues[i]; >> >>> > ? ? ? ? ? ? unsigned int queue_id = qos->key_queues[i]; >> >>> > >> >>> > + ? ? ? ? ? ?if (queue_id == 0) { >> >>> > + ? ? ? ? ? ? ? ?queue_zero = true; >> >>> > + ? ? ? ? ? ?} >> >>> > + >> >>> > ? ? ? ? ? ? shash_from_ovs_idl_map(queue->key_other_config, >> >>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?queue->value_other_config, >> >>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?queue->n_other_config, &details); >> >>> > ? ? ? ? ? ? netdev_set_queue(iface->netdev, queue_id, &details); >> >>> > ? ? ? ? ? ? shash_destroy(&details); >> >>> > ? ? ? ? } >> >>> > + ? ? ? ?if (!queue_zero) { >> >>> > + ? ? ? ? ? ?static struct vlog_rate_limit rl = >> >>> > VLOG_RATE_LIMIT_INIT(1, 1); >> >>> > + ? ? ? ? ? ?VLOG_WARN_RL(&rl, "interface %s: QoS configured without a >> >>> > default " >> >>> > + ? ? ? ? ? ? ? ? ? ? ? ? "queue (queue 0). ?Packets not directed to a >> >>> > " >> >>> > + ? ? ? ? ? ? ? ? ? ? ? ? "correctly configured queue may be dropped.", >> >>> > + ? ? ? ? ? ? ? ? ? ? ? ? iface->name); >> >>> > + ? ? ? ?} >> >>> > ? ? } >> >>> > >> >>> > ? ? netdev_set_policing(iface->netdev, >> >>> > -- >> >>> > 1.7.4.4 >> >>> > >> >>> > _______________________________________________ >> >>> > dev mailing list >> >>> > [email protected] >> >>> > http://openvswitch.org/mailman/listinfo/dev >> >>> > >> >> >> > > _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
