Split vcpu lock #81
Split vcpu lock #81
Conversation
efenniht
force-pushed
the
upstream-locksplit
branch
from
1203c38
to
0677f59
Compare
| break; | ||
| } | ||
|
|
||
| if (!vcpu_try_lock(target_vcpu, &target_vcpu_locked)) { |
There was a problem hiding this comment.
이 지점에서 vCPU가 켜져 있는지를 확인하기 위해서 락을 겁니다. 따라서 실행 중이 아니더라도 execution lock 을 걸어야 하는 경우가 발생하는데, 이것을 어떻게 하는 게 맞을까요?
- execution lock을 status lock 과 register lock 으로 분리. 이 경우 register lock이 걸린 경우 status 가 무조건 실행 중이고, 따라서 status를 무조건 잠궈야 합니다. 이런 invariant를 어떻게 표현해야 할까요?
- 그냥 무시. execution lock에 달린 주석과 행동이 달라지고 개발자들 입장에서 더 부자연스러울 수 있습니다.
There was a problem hiding this comment.
PSCI_AFFINITY_INFO의 semantics가 궁금합니다.- 저는 lock을 분리하는건 너무 복잡도가 올라갈 것 같습니다. 하나로 남기는데 이렇게 vCPU의 상태를 확인하기 위한 경우가 생긴다면 "execution lock"의 이름이 잘못된 걸수도 있겠습니다. 혹시 적절한 이름을 suggest해주시길 부탁드려요.
- 어떻게 결정을 내린다고 하더라도 try_lock을 잡을 때 왜 잡는지 주석이 필요할 것 같습니다.
There was a problem hiding this comment.
PSCI_AFFINITY_INFO 는 ARM의 PSCI(Power State Coordination Interface) 가 지원하는 명령 중 하나입니다. 원래는 physical CPU의 꺼지고 켜짐을 조회하는 데 쓰이는데, 여기서는 hypervisor가 중간에 개입하여, secondary VM이 자신의 vCPU에 대한 정보를 얻을 수 있도록 하고 있습니다.
이 lock은 state_lock 같은 이름이 맞을 것 같은데, 그대로 둔다면 secondary VM A의
- vCPU 0가 vCPU 1이 켜져 있는 지 확인하는 중에
- primary VM이 vCPU 1을 실행하려 하면
Primary VM은 vCPU 1의 실행을 실패할 수 있습니다. 이것이 옳은 semantics인가요?
There was a problem hiding this comment.
Semantics가 틀려보이긴 하네요... 원래 구현은 어땠는지 궁금합니다.
Change-Id: I24e714a3db3c873860f0dab5a424e5c3e585eb0c
efenniht
force-pushed
the
upstream-locksplit
branch
from
0677f59
to
cf3c878
Compare
| sl_lock(&vcpu->lock); | ||
| vcpu->regs_available = true; | ||
| sl_unlock(&vcpu->lock); | ||
| if (vcpu->vm->id != HF_PRIMARY_VM_ID) { |
There was a problem hiding this comment.
- 이 if는 왜 필요한지 궁금합니다.
- 이건 이유를 한눈에 알아보기 쉽지 않은데 주석으로 남겨주세요.
| break; | ||
| } | ||
|
|
||
| if (!vcpu_try_lock(target_vcpu, &target_vcpu_locked)) { |
There was a problem hiding this comment.
PSCI_AFFINITY_INFO의 semantics가 궁금합니다.- 저는 lock을 분리하는건 너무 복잡도가 올라갈 것 같습니다. 하나로 남기는데 이렇게 vCPU의 상태를 확인하기 위한 경우가 생긴다면 "execution lock"의 이름이 잘못된 걸수도 있겠습니다. 혹시 적절한 이름을 suggest해주시길 부탁드려요.
- 어떻게 결정을 내린다고 하더라도 try_lock을 잡을 때 왜 잡는지 주석이 필요할 것 같습니다.
| @@ -61,6 +62,12 @@ static inline void sl_lock(struct spinlock *l) | |||
| : "cc"); | |||
| } | |||
|
|
|||
| static inline bool sl_try_lock(struct spinlock *l) | |||
| { | |||
| return !atomic_flag_test_and_set_explicit((volatile atomic_flag *)&l->v, | |||
There was a problem hiding this comment.
Linter를 통과했는지 궁금합니다. indent가 이상해 보이는데 손으로 한 것 같진 않아서..
There was a problem hiding this comment.
make format 을 실행하니까 이렇게 만들어 주었습니다.
Split vCPU lock into an execution lock and interrupts lock, and remove
regs_availableandVCPU_STATE_RUNNING.Successor of #78.
TODO:
sl_try_lock을src/arch/aarch64/inc/hf/arch/spinlock.h파일의 다른 함수들처럼 만들어야 합니다. 해당하는 어셈블리를 몰라서 당장은 반영하기 어려운 패치일 듯 합니다.