Page MenuHomeFreeBSD

Fix HFSC configuration bug introduced in r343287
ClosedPublic

Authored by pkelsey on Fri, Feb 8, 7:03 PM.

Details

Summary

The short story is that as part of r343287, code was added to get rid of the false-positive warnings from pfctl that the sum of the child queue bandwidths exceeded the parent that could occur in certain HFSC configurations, and that code has a bug if a queue has an upperlimit service curve that is above the linkshare service curve of its parent - in that case, the upperlimit service curve winds up being erroneously pulled down to the parent's linkshare service curve. In practical terms, the result of this bug would be that any affected HFSC queue would wind up functioning subject to a tighter constraint than expected when it wasn't competing for the link.

It turns out that in the HFSC case, this sum-of-child-bandwidths check logic should not be used at all - in HFSC, the conceptually equivalent form of this check is to make sure that the sum of the child linkshare service curves is at or under the parent's linkshare service curves, as those are the constraints that apply when the queues are competing for the link. This check is already implemented in eval_pfqueue_hfsc().

This patch removes the new parameter conditioning logic introduced in r343287, bypasses the sum-of-child-bandwidths check for HFSC, and moves the call to eval_queue_opts() back to where it was prior to r343287.

Here is a more exhaustive discussion:

The previous fix attempted to make the sum-of-child-bandwidths check
logic 'work for HFSC' by overriding the queue bandwidth parameter with
the link share m2 parameter if it was set.

For HFSC, the queue bandwidth parameter is only used in pfctl (not
used by the kernel logic at all) to set an upper limit for the m2
parameters for all three service curves (realtime, linkshare, and
upperlimit) - specifically, a given queue's m2 parameter on any curve
is limited to the parent's bandwidth parameter. As a valid config
can't have a queue whose linkshare m2 parameter exceeds its parent's
linkshare m2 parameter, and likewise for the realtime m2 parameter (as
the realtime m2 parameter logically can't be greater than the
linkshare m2, it thus transitively can't be greater than the parent's
linkshare m2 parameter), limiting a queue's bandwidth parameter to the
supplied linkshare m2 parameter appeared to be a valid way to pass the
sum-of-child-bandwidths check.

The flaw in the reasoning appears when properly considering the
upperlimit service curve. For example, a configuration may have a queue
whose upperlimit m2 parameter is the same as the interface bandwidth,
and thus greater than any parent linkshare m2 parameter that is less
than the interface bandwidth (such a config would express the idea
that when the queue isn't competing for the link, it can use the whole
thing). Such a config would be broken by the above queue bandwidth
parameter = linkshare m2 parameter approach, as that approach will
pull down any upperlimit m2 parameter that is greater than the parent
queue's linkshare m2 parameter to that parent's linkshare m2 value. The
practical result of this pulling down would be that a queue that
otherwise could have the entire link to itself under the configuration
intent would still be limited to the linkshare limitation of its
parent.

Taking a step back, the next key observation is that the
sum-of-child-bandwidths check should not be appeased for HFSC, it
should be skipped. For HFSC, a check of this type only make sense
when the child queues are competing for the link. When that is
happening, resource allocation occurs according to the child linkshare
service curves, so what should be checked is that the sum of the child
linkshare service curves is under the parent linkshare service curve.
That check is already done by eval_pfqueue_hfsc().

So, in eval_pfqueue(), the sum-of-child-bandwidths check is now
skipped for HFSC, and the call to eval_queue_opts() is moved back to
where it used to be (it was moved up in order to evlauate the limit
logic on the linkshare m2 option prior to making a decision based on
it - a decision which has now been removed).

Test Plan

The bug was detected and verified to be fixed by this patch using a test case having queues with an upperlimit service curve that was above the parent's linkshare service curve.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

pkelsey created this revision.Fri, Feb 8, 7:03 PM

I have no objections to the patch, but I don't know enough about HFSC to meaningfully review this, I'm afraid.

pkelsey edited the test plan for this revision. (Show Details)Sat, Feb 9, 5:22 PM

I have no objections to the patch, but I don't know enough about HFSC to meaningfully review this, I'm afraid.

I don't have any ideas as to who else to add to the review.

One way to look at this is that this patch returns things to the way they were, except it now avoids the hierarchical check based on the queue bandwidth parameter for HFSC. I think without knowing much about the finer points of HFSC, it's possible to check the following:

  • The in-kernel HFSC code does not use the queue bandwidth parameter for any purpose, so it is a question of what the bandwidth parameter gets used for in pfctl for HFSC queues
  • Inspection of pfctl_altq.c should show that outside of the check in eval_pfqueue() that is now being skipped for HFSC, pa->bandwidth is only used in the course of processing HFSC queues in eval_pfqueue_hfsc() and
    • eval_pfqueue_hfsc() only uses pa->bandwidth to initialize linkshare service curve parameters (lssc) when they are not set
    • eval_pfqueue_hfsc() checks the sum of the child linkshare service curves against the parent's linkshare service curve, therefore, eval_pfqueue_hfsc() is already performing this hierarchical-sum checking for the only part of the operationally used HFSC config that pa->bandwidth can influence and thus the hierarchical-sum check on pa->bandwidth in eval_pfqueue() does not apply
This revision was not accepted when it landed; it landed in state Needs Review.Mon, Feb 11, 10:58 PM
This revision was automatically updated to reflect the committed changes.