reduce heap allocations #11 (#364)

Summary:
Pull Request resolved: #364

By not openning/closing device every time we read/adjust we will not just save tons of allocations, we will actually prevent "hangups" on the kernel IO open/close syscalls.

Reviewed By: abulimov, vvfedorenko

Differential Revision: D58465569

fbshipit-source-id: 272ee024fd33da89354863409c45bb846d3aa3c5

cmd/ptpcheck/cmd/adjfreq.go

@@ -19,6 +19,7 @@ package cmd
 import (
 	"fmt"
 	"math"
+	"os"
 
 	log "github.com/sirupsen/logrus"
 	"github.com/spf13/cobra"
@@ -37,13 +38,20 @@ func init() {
 }
 
 func doAdjFreq(device string, freq float64) error {
-	curFreq, err := phc.FrequencyPPBFromDevice(device)
+	// we need RW permissions to issue CLOCK_ADJTIME on the device, even with empty struct
+	f, err := os.OpenFile(device, os.O_RDWR, 0)
+	if err != nil {
+		return fmt.Errorf("opening device %q to read frequency: %w", device, err)
+	}
+	defer f.Close()
+
+	curFreq, err := phc.FrequencyPPBFromDevice(f)
 	if err != nil {
 		return err
 	}
 	log.Infof("Current device frequency: %f", curFreq)
 
-	maxFreq, err := phc.MaxFreqAdjPPBFromDevice(device)
+	maxFreq, err := phc.MaxFreqAdjPPBFromDevice(f)
 	if err != nil {
 		return err
 	}
@@ -58,7 +66,7 @@ func doAdjFreq(device string, freq float64) error {
 	}
 
 	log.Infof("Setting new frequency value %f", freq)
-	err = phc.ClockAdjFreq(device, freq)
+	err = phc.ClockAdjFreq(f, freq)
 
 	return err
 }

cmd/ptpcheck/cmd/phc2phc.go

@@ -17,6 +17,8 @@ limitations under the License.
 package cmd
 
 import (
+	"fmt"
+	"os"
 	"time"
 
 	"github.com/facebook/time/clock"
@@ -43,7 +45,19 @@ func init() {
 }
 
 func phc2phcRun(srcDevice string, dstDevice string, interval time.Duration, stepth time.Duration) error {
-	freq, err := phc.FrequencyPPBFromDevice(dstDevice)
+	src, err := os.Open(srcDevice)
+	if err != nil {
+		return fmt.Errorf("opening device %q to read frequency: %w", srcDevice, err)
+	}
+	defer src.Close()
+	// we need RW permissions to issue CLOCK_ADJTIME on the device, even with empty struct
+	dst, err := os.OpenFile(dstDevice, os.O_RDWR, 0)
+	if err != nil {
+		return fmt.Errorf("opening device %q to set frequency: %w", dstDevice, err)
+	}
+	defer dst.Close()
+
+	freq, err := phc.FrequencyPPBFromDevice(dst)
 	if err != nil {
 		return err
 	}
@@ -58,7 +72,7 @@ func phc2phcRun(srcDevice string, dstDevice string, interval time.Duration, step
 	pi := servo.NewPiServo(servoCfg, servo.DefaultPiServoCfg(), -freq)
 	pi.SyncInterval(interval.Seconds())
 
-	maxFreq, err := phc.MaxFreqAdjPPBFromDevice(dstDevice)
+	maxFreq, err := phc.MaxFreqAdjPPBFromDevice(dst)
 	if err != nil {
 		log.Warningf("max PHC frequency error: %v", err)
 		maxFreq = phc.DefaultMaxClockFreqPPB
@@ -68,12 +82,12 @@ func phc2phcRun(srcDevice string, dstDevice string, interval time.Duration, step
 	log.Debugf("max PHC frequency: %v", maxFreq)
 
 	for ; ; time.Sleep(interval) {
-		extendedSrc, err := phc.ReadPTPSysOffsetExtended(srcDevice, phc.ExtendedNumProbes)
+		extendedSrc, err := phc.ReadPTPSysOffsetExtended(src, phc.ExtendedNumProbes)
 		if err != nil {
 			log.Errorf("failed to read data from %v: %v", srcDevice, err)
 			continue
 		}
-		extendedDst, err := phc.ReadPTPSysOffsetExtended(dstDevice, phc.ExtendedNumProbes)
+		extendedDst, err := phc.ReadPTPSysOffsetExtended(dst, phc.ExtendedNumProbes)
 		if err != nil {
 			log.Errorf("failed to read data from %v: %v", dstDevice, err)
 			continue
@@ -85,12 +99,12 @@ func phc2phcRun(srcDevice string, dstDevice string, interval time.Duration, step
 		freqAdj, state := pi.Sample(int64(phcOffset), uint64(timeAndOffsetDst.SysTime.UnixNano()))
 		log.Infof("offset %12d freq %+9.0f path delay %5d", phcOffset, freqAdj, timeAndOffsetSrc.Delay.Nanoseconds()+timeAndOffsetDst.Delay.Nanoseconds())
 		if state == servo.StateJump {
-			if err := phc.ClockStep(dstDevice, -phcOffset); err != nil {
+			if err := phc.ClockStep(dst, -phcOffset); err != nil {
 				log.Errorf("failed to step clock by %v: %v", -phcOffset, err)
 				continue
 			}
 		} else {
-			if err := phc.ClockAdjFreq(dstDevice, -freqAdj); err != nil {
+			if err := phc.ClockAdjFreq(dst, -freqAdj); err != nil {
 				log.Errorf("failed to adjust freq to %v: %v", -freqAdj, err)
 				continue
 			}

cmd/ptpcheck/cmd/phcdiff.go

@@ -19,6 +19,7 @@ package cmd
 import (
 	"encoding/json"
 	"fmt"
+	"os"
 	"time"
 
 	"github.com/facebook/time/phc"
@@ -46,11 +47,22 @@ func init() {
 }
 
 func phcdiffRun(deviceA, deviceB string, isJSON bool) error {
-	extendedA, err := phc.ReadPTPSysOffsetExtended(deviceA, phc.ExtendedNumProbes)
+	a, err := os.Open(deviceA)
+	if err != nil {
+		return fmt.Errorf("opening device %q to set frequency: %w", deviceA, err)
+	}
+	defer a.Close()
+	b, err := os.Open(deviceB)
+	if err != nil {
+		return fmt.Errorf("opening device %q to set frequency: %w", deviceB, err)
+	}
+	defer b.Close()
+
+	extendedA, err := phc.ReadPTPSysOffsetExtended(a, phc.ExtendedNumProbes)
 	if err != nil {
 		return err
 	}
-	extendedB, err := phc.ReadPTPSysOffsetExtended(deviceB, phc.ExtendedNumProbes)
+	extendedB, err := phc.ReadPTPSysOffsetExtended(b, phc.ExtendedNumProbes)
 	if err != nil {
 		return err
 	}

fbclock/daemon/daemon.go

@@ -21,6 +21,7 @@ import (
 	"errors"
 	"fmt"
 	"math"
+	"os"
 	"sync"
 	"time"
 
@@ -198,9 +199,16 @@ func New(cfg *Config, stats stats.Server, l Logger) (*Daemon, error) {
 	if err != nil {
 		return nil, fmt.Errorf("finding PHC device for %q: %w", cfg.Iface, err)
 	}
+
+	// Keep file open for the lifetime of the fbclock
+	f, err := os.OpenFile(phcDevice, os.O_RDWR, 0)
+	if err != nil {
+		return nil, err
+	}
+
 	// function to get time from phc
-	s.getPHCTime = func() (time.Time, error) { return phc.TimeFromDevice(phcDevice) }
-	s.getPHCFreqPPB = func() (float64, error) { return phc.FrequencyPPBFromDevice(phcDevice) }
+	s.getPHCTime = func() (time.Time, error) { return phc.TimeFromDevice(f) }
+	s.getPHCFreqPPB = func() (float64, error) { return phc.FrequencyPPBFromDevice(f) }
 	// calculated values
 	s.stats.SetCounter("m_ns", 0)
 	s.stats.SetCounter("w_ns", 0)

phc/adjtime.go

@@ -26,56 +26,29 @@ import (
 )
 
 // FrequencyPPBFromDevice reads PHC device frequency in PPB
-func FrequencyPPBFromDevice(phcDevice string) (freqPPB float64, err error) {
-	// we need RW permissions to issue CLOCK_ADJTIME on the device, even with empty struct
-	f, err := os.OpenFile(phcDevice, os.O_RDWR, 0)
-	if err != nil {
-		return freqPPB, fmt.Errorf("opening device %q to read frequency: %w", phcDevice, err)
-	}
-	defer f.Close()
+func FrequencyPPBFromDevice(phcDevice *os.File) (freqPPB float64, err error) {
 	var state int
-	freqPPB, state, err = clock.FrequencyPPB(FDToClockID(f.Fd()))
+	freqPPB, state, err = clock.FrequencyPPB(FDToClockID(phcDevice.Fd()))
 	if err == nil && state != unix.TIME_OK {
-		return freqPPB, fmt.Errorf("clock %q state %d is not TIME_OK", phcDevice, state)
+		return freqPPB, fmt.Errorf("clock %q state %d is not TIME_OK", phcDevice.Name(), state)
 	}
 	return freqPPB, err
 }
 
-// FrequencyPPB reads network card PHC device frequency in PPB
-func FrequencyPPB(iface string) (float64, error) {
-	device, err := IfaceToPHCDevice(iface)
-	if err != nil {
-		return 0.0, err
-	}
-	return FrequencyPPBFromDevice(device)
-}
-
 // ClockAdjFreq adjusts PHC clock frequency in PPB
-func ClockAdjFreq(phcDevice string, freqPPB float64) error {
-	// we need RW permissions to issue CLOCK_ADJTIME on the device, even with empty struct
-	f, err := os.OpenFile(phcDevice, os.O_RDWR, 0)
-	if err != nil {
-		return fmt.Errorf("opening device %q to set frequency: %w", phcDevice, err)
-	}
-	defer f.Close()
-	state, err := clock.AdjFreqPPB(FDToClockID(f.Fd()), freqPPB)
+func ClockAdjFreq(phcDevice *os.File, freqPPB float64) error {
+	state, err := clock.AdjFreqPPB(FDToClockID(phcDevice.Fd()), freqPPB)
 	if err == nil && state != unix.TIME_OK {
-		return fmt.Errorf("clock %q state %d is not TIME_OK", phcDevice, state)
+		return fmt.Errorf("clock %q state %d is not TIME_OK", phcDevice.Name(), state)
 	}
 	return err
 }
 
 // ClockStep steps PHC clock by given step
-func ClockStep(phcDevice string, step time.Duration) error {
-	// we need RW permissions to issue CLOCK_ADJTIME on the device, even with empty struct
-	f, err := os.OpenFile(phcDevice, os.O_RDWR, 0)
-	if err != nil {
-		return fmt.Errorf("opening device %q to set frequency: %w", phcDevice, err)
-	}
-	defer f.Close()
-	state, err := clock.Step(FDToClockID(f.Fd()), step)
+func ClockStep(phcDevice *os.File, step time.Duration) error {
+	state, err := clock.Step(FDToClockID(phcDevice.Fd()), step)
 	if err == nil && state != unix.TIME_OK {
-		return fmt.Errorf("clock %q state %d is not TIME_OK", phcDevice, state)
+		return fmt.Errorf("clock %q state %d is not TIME_OK", phcDevice.Name(), state)
 	}
 	return err
 }

phc/offset.go

@@ -90,13 +90,14 @@ func TimeAndOffset(iface string, method TimeMethod) (SysoffResult, error) {
 
 // TimeAndOffsetFromDevice returns time we got from phc device + offset
 func TimeAndOffsetFromDevice(device string, method TimeMethod) (SysoffResult, error) {
+	f, err := os.Open(device)
+	if err != nil {
+		return SysoffResult{}, err
+	}
+	defer f.Close()
+
 	switch method {
 	case MethodSyscallClockGettime:
-		f, err := os.Open(device)
-		if err != nil {
-			return SysoffResult{}, err
-		}
-		defer f.Close()
 		var ts unix.Timespec
 		ts1 := time.Now()
 		err = unix.ClockGettime(FDToClockID(f.Fd()), &ts)
@@ -107,7 +108,7 @@ func TimeAndOffsetFromDevice(device string, method TimeMethod) (SysoffResult, er
 
 		return SysoffEstimateBasic(ts1, time.Unix(ts.Unix()), ts2), nil
 	case MethodIoctlSysOffsetExtended:
-		extended, err := ReadPTPSysOffsetExtended(device, ExtendedNumProbes)
+		extended, err := ReadPTPSysOffsetExtended(f, ExtendedNumProbes)
 		if err != nil {
 			return SysoffResult{}, err
 		}
@@ -154,7 +155,7 @@ func OffsetBetweenExtendedReadings(extendedA, extendedB *PTPSysOffsetExtended) t
 }
 
 // OffsetBetweenDevices returns estimated difference between two PHC devices
-func OffsetBetweenDevices(deviceA, deviceB string) (time.Duration, error) {
+func OffsetBetweenDevices(deviceA, deviceB *os.File) (time.Duration, error) {
 	extendedA, err := ReadPTPSysOffsetExtended(deviceA, ExtendedNumProbes)
 	if err != nil {
 		return 0, err

phc/phc.go

@@ -80,11 +80,18 @@ func Time(iface string, method TimeMethod) (time.Time, error) {
 	if err != nil {
 		return time.Time{}, err
 	}
+
+	f, err := os.Open(device)
+	if err != nil {
+		return time.Time{}, err
+	}
+	defer f.Close()
+
 	switch method {
 	case MethodSyscallClockGettime:
-		return TimeFromDevice(device)
+		return TimeFromDevice(f)
 	case MethodIoctlSysOffsetExtended:
-		extended, err := ReadPTPSysOffsetExtended(device, 1)
+		extended, err := ReadPTPSysOffsetExtended(f, 1)
 		if err != nil {
 			return time.Time{}, err
 		}
@@ -95,31 +102,21 @@ func Time(iface string, method TimeMethod) (time.Time, error) {
 }
 
 // TimeFromDevice returns time we got from PTP device
-func TimeFromDevice(device string) (time.Time, error) {
-	f, err := os.Open(device)
-	if err != nil {
-		return time.Time{}, err
-	}
-	defer f.Close()
+func TimeFromDevice(phcDevice *os.File) (time.Time, error) {
 	var ts unix.Timespec
-	if err := unix.ClockGettime(FDToClockID(f.Fd()), &ts); err != nil {
+	if err := unix.ClockGettime(FDToClockID(phcDevice.Fd()), &ts); err != nil {
 		return time.Time{}, fmt.Errorf("failed clock_gettime: %w", err)
 	}
 	return time.Unix(ts.Unix()), nil
 }
 
 // ReadPTPSysOffsetExtended gets precise time from PHC along with SYS time to measure the call delay.
-func ReadPTPSysOffsetExtended(device string, nsamples int) (*PTPSysOffsetExtended, error) {
-	f, err := os.Open(device)
-	if err != nil {
-		return nil, err
-	}
-	defer f.Close()
+func ReadPTPSysOffsetExtended(phcDevice *os.File, nsamples int) (*PTPSysOffsetExtended, error) {
 	res := &PTPSysOffsetExtended{
 		NSamples: uint32(nsamples),
 	}
 	_, _, errno := unix.Syscall(
-		unix.SYS_IOCTL, f.Fd(),
+		unix.SYS_IOCTL, phcDevice.Fd(),
 		ioctlPTPSysOffsetExtended,
 		uintptr(unsafe.Pointer(res)),
 	)
@@ -130,17 +127,11 @@ func ReadPTPSysOffsetExtended(device string, nsamples int) (*PTPSysOffsetExtende
 }
 
 // ReadPTPClockCapsFromDevice reads ptp capabilities using ioctl
-func ReadPTPClockCapsFromDevice(phcDevice string) (*PTPClockCaps, error) {
-	f, err := os.OpenFile(phcDevice, os.O_RDWR, 0)
-	if err != nil {
-		return nil, fmt.Errorf("opening device %q to get max frequency: %w", phcDevice, err)
-	}
-	defer f.Close()
-
+func ReadPTPClockCapsFromDevice(phcDevice *os.File) (*PTPClockCaps, error) {
 	caps := &PTPClockCaps{}
 
 	_, _, errno := unix.Syscall(
-		unix.SYS_IOCTL, f.Fd(),
+		unix.SYS_IOCTL, phcDevice.Fd(),
 		ioctlPTPClockGetcaps,
 		uintptr(unsafe.Pointer(caps)),
 	)
@@ -152,7 +143,7 @@ func ReadPTPClockCapsFromDevice(phcDevice string) (*PTPClockCaps, error) {
 }
 
 // MaxFreqAdjPPBFromDevice reads max value for frequency adjustments (in PPB) from ptp device
-func MaxFreqAdjPPBFromDevice(phcDevice string) (maxFreq float64, err error) {
+func MaxFreqAdjPPBFromDevice(phcDevice *os.File) (maxFreq float64, err error) {
 	caps, err := ReadPTPClockCapsFromDevice(phcDevice)
 
 	if err != nil {

ptp/c4u/clock/ts2phc.go

@@ -17,8 +17,10 @@ limitations under the License.
 package clock
 
 import (
-	"github.com/facebook/time/phc"
+	"os"
 	"time"
+
+	"github.com/facebook/time/phc"
 )
 
 const (
@@ -27,5 +29,17 @@ const (
 )
 
 func ts2phc() (time.Duration, error) {
-	return phc.OffsetBetweenDevices(phcTimeCardPath, phcNICPath)
+	a, err := os.Open(phcTimeCardPath)
+	if err != nil {
+		return 0, err
+	}
+	defer a.Close()
+
+	b, err := os.Open(phcNICPath)
+	if err != nil {
+		return 0, err
+	}
+	defer b.Close()
+
+	return phc.OffsetBetweenDevices(a, b)
 }