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
