Page MenuHomeFreeBSD

Improve pf.conf parsing speed for large numbers of queues
ClosedPublic

Authored by pkelsey on Sun, Jan 6, 12:23 AM.

Details

Summary

This patch reduces pf.conf parsing cost for configs that define N queues from O(N^2) to O(N). It also reduces the number of syscalls made during parsing of any config that defines tables. On the systems and large-queue-count configurations tested, parse time was reduced by 30-50%. There is corresponding kernel side work that will arrive in the near future.

Details:

  • Added a wrapper structure in pfctl for struct pf_altq that stores metadata that pfctl now uses to avoid unnecessary scanning of the list of all queues and to avoid computing aggregate bandwidths from scratch for error checking each time a new queue is added.
  • Queues are now tracked by hash tables instead of by a single list, with one table for queues that represent interfaces, and another hash table for all other queues.
  • Kernel-assigned queue IDs are now tracked in a hash table to avoid scanning all queues when enforcing the same-queue-name-means-same-queue-id-across-interfaces policy.
  • Summation-type metrics and error checks that were previously recomputed from scratch across all queues for each new queue are now computed incrementally.
  • Some error checks have been converted from pre-commit to on-the-fly-during-queue-add in order to avoid re-scanning the set of all queues.
  • The logic that ensures unique queue priorities for each PRIQ queue on a given interface now uses a bitmap instead of list-scanning.
  • The socket used to query the kernel about interfaces is now created once and cached instead of being created and destroyed for each request. This greatly reduces the number of syscalls made during parse of configs with a large number of queues.
  • Responses to the SIOCGIFGMEMB ioctl are now cached, which greatly reduces the number of this ioctl that occur during parse.
  • A table of interface groups is now built when the interface table is built at startup and that table is used instead of querying the kernel forever after. This is important because pfctl will try to determine if each entry in a table definition is an interface (or interface group name), before, say, discovering that the entry is an IP address or CIDR notation. Prior to adding the interface groups table, each table entry in the config would result in unnecessary syscalls.
  • Also fixed the bandwidth check for HFSC queues, which previously would emit spurious warnings about total child bandwidth exceeding the parent queue's bandwidth when that was not in fact the case. This required evaluating the queue opts a bit sooner and also implementing the override of queue bandwidth by the m2 parameter (if set) that occurs in the kernel.

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.Sun, Jan 6, 12:23 AM

I’ll try to review asap, but I’m on holiday wirh limited internet access right now.

kristof accepted this revision.Sat, Jan 12, 5:55 AM
kristof added inline comments.
sbin/pfctl/pfctl_altq.c
1301 ↗(On Diff #52599)

How hard would it be to pass around a pfctl_t or something to track the socket? That'd make it easier to factor out some bits into a library.

sbin/pfctl/pfctl_parser.h
335 ↗(On Diff #52599)

That's because hsearch_r() doesn't take a const, right?

This revision is now accepted and ready to land.Sat, Jan 12, 5:55 AM

Also, I’d really, really like some basic tests for altq. It should be reasonably straightforward to do something based on the existing pf tests. At least, if altq can work on top of epairs.

That’s not a requirement to commit this patch, of course, but it’d be nice to be confident we don’t accidentally break altq.

pkelsey added inline comments.Mon, Jan 21, 11:48 PM
sbin/pfctl/pfctl_altq.c
1301 ↗(On Diff #52599)

I had that in mind, but realized that there are a number of globals in pfctl that would need to be addressed as part of introducing something like a pfctl_t and chose to make one small addition to the 'problem' in lieu of biting off solving the whole thing in this patch.

sbin/pfctl/pfctl_parser.h
335 ↗(On Diff #52599)

More or less - it's specifically because the key member of the ENTRY struct is a char * and not a const char *.

Also, I’d really, really like some basic tests for altq. It should be reasonably straightforward to do something based on the existing pf tests. At least, if altq can work on top of epairs.

That’s not a requirement to commit this patch, of course, but it’d be nice to be confident we don’t accidentally break altq.

Agree, altq automated tests would be nice. A quick scan of if_epair.c looks like some effort was made to support ALTQ, but I also don't see a call to if_setsendqready() in the initialization path, so there may be dragons.

This revision was automatically updated to reflect the committed changes.