Skip to content

Commit 2205e8d

Browse files
crosbymichaelcrosbymichael
committed
Improve shim locking
Only lock around shim state and not on actions Signed-off-by: Michael Crosby <[email protected]>
1 parent d50e253 commit 2205e8d

1 file changed

Lines changed: 31 additions & 32 deletions

File tree

‎runtime/v2/runc/service.go‎

Lines changed: 31 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -304,7 +304,7 @@ func (s *service) Create(ctx context.Context, r *taskAPI.CreateTaskRequest) (_ *
304304
if err != nil {
305305
logrus.WithError(err).Errorf("loading cgroup for %d", pid)
306306
}
307-
s.setCgroup(cg)
307+
s.cg = cg
308308
}
309309
s.task = process
310310
return &taskAPI.CreateTaskResponse{
@@ -315,16 +315,15 @@ func (s *service) Create(ctx context.Context, r *taskAPI.CreateTaskRequest) (_ *
315315

316316
// Start a process
317317
func (s *service) Start(ctx context.Context, r *taskAPI.StartRequest) (*taskAPI.StartResponse, error) {
318-
s.mu.Lock()
319-
defer s.mu.Unlock()
320318
p, err := s.getProcess(r.ExecID)
321319
if err != nil {
322320
return nil, err
323321
}
324322
if err := p.Start(ctx); err != nil {
325323
return nil, err
326324
}
327-
if s.cg == nil && p.Pid() > 0 {
325+
// case for restore
326+
if s.getCgroup() == nil && p.Pid() > 0 {
328327
cg, err := cgroups.Load(cgroups.V1, cgroups.PidPath(p.Pid()))
329328
if err != nil {
330329
logrus.WithError(err).Errorf("loading cgroup for %d", p.Pid())
@@ -338,8 +337,6 @@ func (s *service) Start(ctx context.Context, r *taskAPI.StartRequest) (*taskAPI.
338337

339338
// Delete the initial process and container
340339
func (s *service) Delete(ctx context.Context, r *taskAPI.DeleteRequest) (*taskAPI.DeleteResponse, error) {
341-
s.mu.Lock()
342-
defer s.mu.Unlock()
343340
p, err := s.getProcess(r.ExecID)
344341
if err != nil {
345342
return nil, err
@@ -352,7 +349,9 @@ func (s *service) Delete(ctx context.Context, r *taskAPI.DeleteRequest) (*taskAP
352349
}
353350
isTask := r.ExecID == ""
354351
if !isTask {
352+
s.mu.Lock()
355353
delete(s.processes, r.ExecID)
354+
s.mu.Unlock()
356355
}
357356
if isTask && s.platform != nil {
358357
s.platform.Close()
@@ -367,11 +366,12 @@ func (s *service) Delete(ctx context.Context, r *taskAPI.DeleteRequest) (*taskAP
367366
// Exec an additional process inside the container
368367
func (s *service) Exec(ctx context.Context, r *taskAPI.ExecProcessRequest) (*ptypes.Empty, error) {
369368
s.mu.Lock()
370-
defer s.mu.Unlock()
371-
if p := s.processes[r.ExecID]; p != nil {
369+
p := s.processes[r.ExecID]
370+
s.mu.Unlock()
371+
if p != nil {
372372
return nil, errdefs.ToGRPCf(errdefs.ErrAlreadyExists, "id %s", r.ExecID)
373373
}
374-
p := s.task
374+
p = s.task
375375
if p == nil {
376376
return nil, errdefs.ToGRPCf(errdefs.ErrFailedPrecondition, "container must be created")
377377
}
@@ -386,14 +386,14 @@ func (s *service) Exec(ctx context.Context, r *taskAPI.ExecProcessRequest) (*pty
386386
if err != nil {
387387
return nil, errdefs.ToGRPC(err)
388388
}
389+
s.mu.Lock()
389390
s.processes[r.ExecID] = process
391+
s.mu.Unlock()
390392
return empty, nil
391393
}
392394

393395
// ResizePty of a process
394396
func (s *service) ResizePty(ctx context.Context, r *taskAPI.ResizePtyRequest) (*ptypes.Empty, error) {
395-
s.mu.Lock()
396-
defer s.mu.Unlock()
397397
p, err := s.getProcess(r.ExecID)
398398
if err != nil {
399399
return nil, err
@@ -410,8 +410,6 @@ func (s *service) ResizePty(ctx context.Context, r *taskAPI.ResizePtyRequest) (*
410410

411411
// State returns runtime state information for a process
412412
func (s *service) State(ctx context.Context, r *taskAPI.StateRequest) (*taskAPI.StateResponse, error) {
413-
s.mu.Lock()
414-
defer s.mu.Unlock()
415413
p, err := s.getProcess(r.ExecID)
416414
if err != nil {
417415
return nil, err
@@ -451,8 +449,8 @@ func (s *service) State(ctx context.Context, r *taskAPI.StateRequest) (*taskAPI.
451449
// Pause the container
452450
func (s *service) Pause(ctx context.Context, r *taskAPI.PauseRequest) (*ptypes.Empty, error) {
453451
s.mu.Lock()
454-
defer s.mu.Unlock()
455452
p := s.task
453+
s.mu.Unlock()
456454
if p == nil {
457455
return nil, errdefs.ToGRPCf(errdefs.ErrFailedPrecondition, "container must be created")
458456
}
@@ -465,8 +463,8 @@ func (s *service) Pause(ctx context.Context, r *taskAPI.PauseRequest) (*ptypes.E
465463
// Resume the container
466464
func (s *service) Resume(ctx context.Context, r *taskAPI.ResumeRequest) (*ptypes.Empty, error) {
467465
s.mu.Lock()
468-
defer s.mu.Unlock()
469466
p := s.task
467+
s.mu.Unlock()
470468
if p == nil {
471469
return nil, errdefs.ToGRPCf(errdefs.ErrFailedPrecondition, "container must be created")
472470
}
@@ -478,8 +476,6 @@ func (s *service) Resume(ctx context.Context, r *taskAPI.ResumeRequest) (*ptypes
478476

479477
// Kill a process with the provided signal
480478
func (s *service) Kill(ctx context.Context, r *taskAPI.KillRequest) (*ptypes.Empty, error) {
481-
s.mu.Lock()
482-
defer s.mu.Unlock()
483479
p, err := s.getProcess(r.ExecID)
484480
if err != nil {
485481
return nil, err
@@ -526,8 +522,6 @@ func (s *service) Pids(ctx context.Context, r *taskAPI.PidsRequest) (*taskAPI.Pi
526522

527523
// CloseIO of a process
528524
func (s *service) CloseIO(ctx context.Context, r *taskAPI.CloseIORequest) (*ptypes.Empty, error) {
529-
s.mu.Lock()
530-
defer s.mu.Unlock()
531525
p, err := s.getProcess(r.ExecID)
532526
if err != nil {
533527
return nil, err
@@ -543,8 +537,8 @@ func (s *service) CloseIO(ctx context.Context, r *taskAPI.CloseIORequest) (*ptyp
543537
// Checkpoint the container
544538
func (s *service) Checkpoint(ctx context.Context, r *taskAPI.CheckpointTaskRequest) (*ptypes.Empty, error) {
545539
s.mu.Lock()
546-
defer s.mu.Unlock()
547540
p := s.task
541+
s.mu.Unlock()
548542
if p == nil {
549543
return nil, errdefs.ToGRPCf(errdefs.ErrFailedPrecondition, "container must be created")
550544
}
@@ -589,13 +583,11 @@ func (s *service) Shutdown(ctx context.Context, r *taskAPI.ShutdownRequest) (*pt
589583
}
590584

591585
func (s *service) Stats(ctx context.Context, r *taskAPI.StatsRequest) (*taskAPI.StatsResponse, error) {
592-
s.mu.Lock()
593-
defer s.mu.Unlock()
594-
595-
if s.cg == nil {
586+
cg := s.getCgroup()
587+
if cg == nil {
596588
return nil, errdefs.ToGRPCf(errdefs.ErrNotFound, "cgroup does not exist")
597589
}
598-
stats, err := s.cg.Stat(cgroups.IgnoreNotExist)
590+
stats, err := cg.Stat(cgroups.IgnoreNotExist)
599591
if err != nil {
600592
return nil, err
601593
}
@@ -611,8 +603,8 @@ func (s *service) Stats(ctx context.Context, r *taskAPI.StatsRequest) (*taskAPI.
611603
// Update a running container
612604
func (s *service) Update(ctx context.Context, r *taskAPI.UpdateTaskRequest) (*ptypes.Empty, error) {
613605
s.mu.Lock()
614-
defer s.mu.Unlock()
615606
p := s.task
607+
s.mu.Unlock()
616608
if p == nil {
617609
return nil, errdefs.ToGRPCf(errdefs.ErrFailedPrecondition, "container must be created")
618610
}
@@ -624,9 +616,7 @@ func (s *service) Update(ctx context.Context, r *taskAPI.UpdateTaskRequest) (*pt
624616

625617
// Wait for a process to exit
626618
func (s *service) Wait(ctx context.Context, r *taskAPI.WaitRequest) (*taskAPI.WaitResponse, error) {
627-
s.mu.Lock()
628619
p, err := s.getProcess(r.ExecID)
629-
s.mu.Unlock()
630620
if err != nil {
631621
return nil, err
632622
}
@@ -648,9 +638,6 @@ func (s *service) processExits() {
648638
}
649639

650640
func (s *service) checkProcesses(e runcC.Exit) {
651-
s.mu.Lock()
652-
defer s.mu.Unlock()
653-
654641
for _, p := range s.allProcesses() {
655642
if p.Pid() == e.Pid {
656643
if ip, ok := p.(*proc.Init); ok {
@@ -674,6 +661,8 @@ func (s *service) checkProcesses(e runcC.Exit) {
674661
}
675662

676663
func (s *service) allProcesses() (o []rproc.Process) {
664+
s.mu.Lock()
665+
defer s.mu.Unlock()
677666
for _, p := range s.processes {
678667
o = append(o, p)
679668
}
@@ -685,8 +674,8 @@ func (s *service) allProcesses() (o []rproc.Process) {
685674

686675
func (s *service) getContainerPids(ctx context.Context, id string) ([]uint32, error) {
687676
s.mu.Lock()
688-
defer s.mu.Unlock()
689677
p := s.task
678+
s.mu.Unlock()
690679
if p == nil {
691680
return nil, errors.Wrapf(errdefs.ErrFailedPrecondition, "container must be created")
692681
}
@@ -713,6 +702,8 @@ func (s *service) forward(publisher events.Publisher) {
713702
}
714703

715704
func (s *service) getProcess(execID string) (rproc.Process, error) {
705+
s.mu.Lock()
706+
defer s.mu.Unlock()
716707
if execID == "" {
717708
return s.task, nil
718709
}
@@ -723,8 +714,16 @@ func (s *service) getProcess(execID string) (rproc.Process, error) {
723714
return p, nil
724715
}
725716

717+
func (s *service) getCgroup() cgroups.Cgroup {
718+
s.mu.Lock()
719+
defer s.mu.Unlock()
720+
return s.cg
721+
}
722+
726723
func (s *service) setCgroup(cg cgroups.Cgroup) {
724+
s.mu.Lock()
727725
s.cg = cg
726+
s.mu.Unlock()
728727
if err := s.ep.add(s.id, cg); err != nil {
729728
logrus.WithError(err).Error("add cg to OOM monitor")
730729
}

0 commit comments

Comments
 (0)