[libvirt PATCH 2/2] nwfilter: Use immediate paket delivery mode rather than buffering

Daniel P. Berrangé berrange at redhat.com
Thu Jan 30 14:53:11 UTC 2020


On Thu, Jan 30, 2020 at 03:43:06PM +0100, Erik Skultety wrote:
> Our nwfilter code doesn't set any timeout on the pcap paket buffer which
> means that when DHCP snooping is enabled on a guest interface and
> libvirt is trying to learn the IP address from guest's DHCP traffic, it
> takes up to 4x longer to ping a guest successfully compared to a case
> where nwfilter isn't enabled at all or libvirt uses the cached nwfilter
> leases to populate the corresponding rules to ebtables.
> With the pcap filter and rate limiting already in place, we should be
> able to afford enabling the immediate paket delivery, FWIW immediate
> mode was actually the default prior libpcap-1.5.0 (CentOS 6) regardless
> of whether a buffer was requested.
> 
> The lack of any kind of timeout on the pcap buffer messed with the
> libvirt TCK test suite which, even with a generous timeout in place,
> timeouts every single time simply because it takes a while until
> guest actually starts producing any kind of traffic to fill up
> the buffer in place (appart from the DHCP traffic which happens fairly
> early on).
> 
> Signed-off-by: Erik Skultety <eskultet at redhat.com>
> ---
> 
> An alternative I've been also looking into is to use pcap_set_timeout before
> activating the pcap handle. The question is what should an appropriate timeout
> look like in that case (e.g. I tried with 500ms), but since prior
> libpcap < 1.5.0 the capture devices were always in the immediate mode on Linux,
> I'd go down the same road again, quoting the man page:
> 
> "in 1.5.0 and later, they are, by default, not in immediate mode, so if
> pcap_set_immediate_mode() is available, it should be used"

Yeah, I don't see a serious downside to immediate mode and it
was historical default, so this looks fine, especially because
it solves the annoying buffsize problem.

> 
>  src/nwfilter/nwfilter_dhcpsnoop.c | 24 +-----------------------
>  1 file changed, 1 insertion(+), 23 deletions(-)
> 
> diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c
> index 10567e9cd3..a1c0c0189e 100644
> --- a/src/nwfilter/nwfilter_dhcpsnoop.c
> +++ b/src/nwfilter/nwfilter_dhcpsnoop.c
> @@ -242,23 +242,6 @@ struct _virNWFilterDHCPDecodeJob {
>  # define DHCP_PKT_BURST         50 /* pkts/sec */
>  # define DHCP_BURST_INTERVAL_S  10 /* sec */
> 
> -/*
> - * NB: Any libpcap built with HAVE_TPACKET3 will require
> - * PCAP_BUFFERSIZE to be at least 262144 (although
> - * pcap_set_buffer_size() with a lower value will succeed, and the
> - * error will only show up later when pcap_setfilter() is called).
> - *
> - * It is possible that in the future libpcap could increase the
> - * minimum size even further, but due to the fact that each guest
> - * using dhcp snooping keeps 2 pcap sockets open (and thus 2 buffers
> - * allocated) for the life of the guest, we want to minimize the
> - * length of the buffer, so instead of leaving it at the default size
> - * (2MB), we are setting it to the minimum viable size and including
> - * this clue in the source to help quickly resolve the problem when/if
> - * it reoccurs.
> - */
> -# define PCAP_BUFFERSIZE        (256 * 1024)
> -
>  # define MAX_QUEUED_JOBS        (DHCP_PKT_BURST + 2 * DHCP_PKT_RATE)
> 
>  typedef struct _virNWFilterSnoopRateLimitConf virNWFilterSnoopRateLimitConf;
> @@ -1098,13 +1081,8 @@ virNWFilterSnoopDHCPOpen(const char *ifname, virMacAddr *mac,
>          goto cleanup_nohandle;
>      }
> 
> -    /* IMPORTANT: If there is any failure of *any* pcap_* function
> -     * during setup of the socket, look to the comment where
> -     * PCAP_BUFFERSIZE is defined. It may be too small, even if the
> -     * generated error doesn't imply that.
> -     */
>      if (pcap_set_snaplen(handle, PCAP_PBUFSIZE) < 0 ||
> -        pcap_set_buffer_size(handle, PCAP_BUFFERSIZE) < 0 ||
> +        pcap_set_immediate_mode(handle, 1) < 0 ||
>          pcap_activate(handle) < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("setup of pcap handle failed: %s"),

Reviewed-by: Daniel P. Berrangé <berrange at redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|




More information about the libvir-list mailing list