Introduce device ctrl for device_led, ne_clock, etc., #46

sukyoungjeong-furiosa · 2022-07-04T06:28:10Z

libc-furiosa

좋은 것 같습니다.
몇몇 부분에 의견을 드렸습니다.
검토 부탁드립니다. 감사합니다.

libc-furiosa · 2022-07-04T08:45:15Z

src/device.rs

+    pub fn ctrl_device_led(&mut self, led: (bool, bool, bool)) -> DeviceResult<()> {
+        self.device_info.ctrl(
+            sysfs::npu_mgmt::DEVICE_LED,
+            &(led.0 as i32 + 0b10 * led.1 as i32 + 0b100 * led.2 as i32).to_string(),


tuple의 요소가 각각 (LED7, LED8, LED9)를 나타낸 것이 맞을까요?
Host Tools 가이드 상으로는 램프의 물리적인 순서가 [9 8 7] 인 것 같은데,
실제 카드 상에서 어떻게 표시되는지 확인하고 마저 정리하면 될 것 같습니다.

안타깝게도 케이스가 씌워진 버전에서는 어느 램프에 불이 들어왔는지 확인이 어렵네요. 어차피 보이지 않아서 순서는 의미가 없을 것 같습니다.

이 API는 장치 제어 인터페이스와 직접적으로 연결되니 현재의 구현을 유지하되,
실제로 이 API를 사용하게 될 furiosactl 등에서 전체를 키고 끄는 기능을 제공한다든가 blink를 하되 반복 주기를 다양하게 하는 등의 옵션 기능을 제공하면 목적은 달성할 수 있을 것 같습니다.

어쨌든 현재 작성하신 코드는 그대로 유지하면 될 것 같습니다.

네, 일단 유지하겠습니다. 물리적인 위치와 tuple 내 position 관계는 자료구조로 표현할 수도 있지만 제 생각에는 주석으로 충분할것 같습니다. 카드 상에서 확인이 되면 향후에 추가하겠습니다.

libc-furiosa · 2022-07-04T08:56:29Z

src/device.rs

+    }
+
+    /// Enable NE clocks.
+    pub fn enable_ne_clock(&mut self) -> DeviceResult<()> {


아마 고민을 먼저 해 보셨을 것 같은데요,
각 요소에 대한 기능을 함수 이름으로 구분하는 것과 인자로 구분하는 방법이 있을텐데요.

ex)
enable_ne_clock() vs set_ne_clock(ENABLE)
ne_dtm_policy_conservative() vs set_ne_dtm_policy(CONSERVATIVE)

어느 쪽이 더 좋을지 의견 부탁드립니다.

저는 크게 차이가 없다고 생각하는데, 두가지 사소한 이유에서 지금 방향을 선택했습니다. 첫째는 enum variant가 계속 만들어지는 것이 성가시기도 하고, boolean으로 쓰자니 false를 넣었을 때 동작이 잘 예상되지 않는다고 봤습니다. 두번째는 set_ne_clock(DISABLE)로 쓰게 되면 set 에 enable의 어감이 들어가 있어서 표현이 직관성이 떨어진다고 생각했습니다. 이건 표현을 잘 고르면 해소될것 같긴 합니다. 다른 의견 있으시면 부탁드립니다!

set 대신 ctrl 을 쓰는건 어떨까요? ctrl_performance_level 등과 일관성도 맞출 수 있을 것 같습니다.

비슷한 enum을 API마다 만들어줘야하는게 다소 번거로울 수 있을 것 같습니다.
다만 우리가 가진 writable API가 그리 많지 않고, 앞으로도 크게 늘어날 가능성은 낮을 것 같은 측면은 있습니다.

API 차원에서 생각해보면.. 함수가 많아지는 것 보다는 인자 표현이 충분해지는게 좀 더 낫지 않나 하는 개인적인 생각도 들어있습니다.

ctrl_*() 으로 쓰고 enum을 ENABLE/DISABLE로 쓰는 방법이 직관성이 괜찮아 보이네요. 반영하겠습니다.

src/device.rs

libc-furiosa · 2022-07-04T10:06:23Z

src/sysfs.rs

+        contents: C,
+    ) -> io::Result<()> {
+        let path = path(sysfs, ctrl_file, idx);
+        std::fs::write(&path, contents)


write에서 가장 흔하게 발생할 에러는 permission denided 인데요.
root 권한을 필요로 하기에 일반 사용자 계정으로 시도 시 에러가 자주 발생할 것 같습니다.

그래서 저는 가능하면 이 부분에서 std::io의 에러 중 권한과 관련된 내용에 대해서는
DeviceError 레벨에서 다룰 수 있으면 어떨까하고 생각되는데요.
어떻게 생각하시는지 의견 부탁드립니다.

네, DeviceError에 다른 variant를 두어서 Permission 문제를 별도로 받는 것을 제안주시는 걸까요?
현재는 std::io로부터 DeviceError로의 변환이 지원되어서 (From<io::Error>) DeviceError::IoError(io::Error) 으로 처리되고 있습니다. Permission denied를 별도로 처리하는 것은 간단한 일이지만 이미 thiserror 통해서 io::Error까지 cause를 보여주고 있어서 사용자에 큰 차이가 있을지 잘 모르겠습니다. 권한 문제를 코드 레벨에서 처리할 일이 있을까요?

io::Error의 cause가 출력된다면 아마도 IoError(Os { code: 13, kind: PermissionDenied, message: "Permission denied" }) 이런 식으로 나올 것으로 예상되는데요.
응용 레이어에서는 root 권한으로 실행해주세요 등의 사용자 친화적인 표현이 출력되면 좋을 것 같습니다.

이런 목적이 있을 경우에 각각이 DeviceError::IoError(io::Error::PermissionDenided) 와 같이 개별적으로 패턴매칭을 하는 것 보다 DeviceError::NeedPrivileged같은 DeviceError 레벨에서 정의된 variant가 있으면 Error Handling을 보다 엄밀하게 할 수 있을 것 같다는 이유로 말씀을 드렸습니다.

과하다고 생각되시면 넘어가셔도 괜찮을 것 같습니다.

네, intention이 이해가 되었습니다. 1768a75 에서 반영했습니다. (다른 코멘트 주신 부분들도 반영 완료했습니다.) 감사합니다!

libc-furiosa · 2022-07-04T10:07:13Z

src/device.rs

-        )
+        sysfs::npu_mgmt::write_ctrl_file(&self.sys_root, key, self.device_index, contents)?;
+
+        if let Some((key, _)) = sysfs::npu_mgmt::MGMT_FILES


sukyoungjeong-furiosa · 2022-07-05T03:05:41Z

@libc-furiosa 님, 리뷰 주신 부분 반영하거나 제 생각을 답변 달았습니다. 일부 코멘트는 편의상 제가 임의로 resolve를 눌러두었습니다만 한번 확인 부탁드리겠습니다!

libc-furiosa

피드백 반영 감사드립니다.
관련된 부분에 추가로 커멘트 남겼습니다. 검토 부탁드립니다.

libc-furiosa · 2022-07-05T07:08:13Z

src/device.rs

+        &mut self,
+        level: sysfs::npu_mgmt::PerfLevel,
+    ) -> DeviceResult<()> {
+        if !(0..=15).contains(&(level as u8)) {


의견 반영해주셔서 감사합니다.
enum을 통해 variant로 값을 받으면 이와 같은 예외처리는 생략 가능할 것 같습니다.
오히려 variant가 변경되면(늘어나거나 줄어들 때) 방해요소가 될 것 같습니다.
의견 부탁드립니다.

그렇네요! 신경쓰지 못했는데 감사합니다~

libc-furiosa · 2022-07-05T07:27:57Z

src/sysfs.rs

+        contents: C,
+    ) -> io::Result<()> {
+        let path = path(sysfs, ctrl_file, idx);
+        std::fs::write(&path, contents)


io::Error의 cause가 출력된다면 아마도 IoError(Os { code: 13, kind: PermissionDenied, message: "Permission denied" }) 이런 식으로 나올 것으로 예상되는데요.
응용 레이어에서는 root 권한으로 실행해주세요 등의 사용자 친화적인 표현이 출력되면 좋을 것 같습니다.

이런 목적이 있을 경우에 각각이 DeviceError::IoError(io::Error::PermissionDenided) 와 같이 개별적으로 패턴매칭을 하는 것 보다 DeviceError::NeedPrivileged같은 DeviceError 레벨에서 정의된 variant가 있으면 Error Handling을 보다 엄밀하게 할 수 있을 것 같다는 이유로 말씀을 드렸습니다.

과하다고 생각되시면 넘어가셔도 괜찮을 것 같습니다.

libc-furiosa · 2022-07-05T07:34:58Z

src/device.rs

+    }
+
+    /// Enable NE clocks.
+    pub fn enable_ne_clock(&mut self) -> DeviceResult<()> {


set 대신 ctrl 을 쓰는건 어떨까요? ctrl_performance_level 등과 일관성도 맞출 수 있을 것 같습니다.

비슷한 enum을 API마다 만들어줘야하는게 다소 번거로울 수 있을 것 같습니다.
다만 우리가 가진 writable API가 그리 많지 않고, 앞으로도 크게 늘어날 가능성은 낮을 것 같은 측면은 있습니다.

API 차원에서 생각해보면.. 함수가 많아지는 것 보다는 인자 표현이 충분해지는게 좀 더 낫지 않나 하는 개인적인 생각도 들어있습니다.

libc-furiosa

수고하셨습니다!

libc-furiosa · 2022-07-05T08:22:58Z

머지하겠습니다.

sukyoungjeong-furiosa added 6 commits July 4, 2022 05:09

Move read_mgmt_files to npu_mgmt module

8ccf180

Remove unnecessary async keyword

211ba8c

Add primitives for writing ctrl files

3e1972b

Utilize DeviceResult enum for interface

000c95b

Resolve clippy warning

d29e385

device_led, ne_clock, ne_dtm_policy, performances

a6358a2

sukyoungjeong-furiosa marked this pull request as ready for review July 4, 2022 07:44

sukyoungjeong-furiosa self-assigned this Jul 4, 2022

sukyoungjeong-furiosa requested a review from libc-furiosa July 4, 2022 07:44

sukyoungjeong-furiosa added enhancement New feature or request api API labels Jul 4, 2022

libc-furiosa reviewed Jul 4, 2022

View reviewed changes

Address review comments

14a0e46

sukyoungjeong-furiosa requested a review from hyunsik July 5, 2022 03:04

libc-furiosa reviewed Jul 5, 2022

View reviewed changes

sukyoungjeong-furiosa added 3 commits July 5, 2022 07:41

Remove unused unsupported_input error

81451b1

Address review comments

31289cc

Add some traits to pub enum

06e35d1

libc-furiosa approved these changes Jul 5, 2022

View reviewed changes

Add a PermissionDenied variant to DeviceError

1768a75

libc-furiosa merged commit 689eabc into furiosa-ai:main Jul 5, 2022

libc-furiosa mentioned this pull request Jul 6, 2022

Implement metadata access: alive, atr_error, heartbeat #45

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce device ctrl for device_led, ne_clock, etc., #46

Introduce device ctrl for device_led, ne_clock, etc., #46

sukyoungjeong-furiosa commented Jul 4, 2022 •

edited

Loading

libc-furiosa left a comment

libc-furiosa Jul 4, 2022

libc-furiosa Jul 4, 2022

sukyoungjeong-furiosa Jul 5, 2022 •

edited

Loading

libc-furiosa Jul 4, 2022

sukyoungjeong-furiosa Jul 5, 2022

libc-furiosa Jul 5, 2022

sukyoungjeong-furiosa Jul 5, 2022

libc-furiosa Jul 4, 2022

sukyoungjeong-furiosa Jul 5, 2022 •

edited

Loading

libc-furiosa Jul 5, 2022

sukyoungjeong-furiosa Jul 5, 2022

libc-furiosa Jul 4, 2022

sukyoungjeong-furiosa commented Jul 5, 2022

libc-furiosa left a comment

libc-furiosa Jul 5, 2022

sukyoungjeong-furiosa Jul 5, 2022

libc-furiosa Jul 5, 2022

libc-furiosa Jul 5, 2022

libc-furiosa left a comment

libc-furiosa commented Jul 5, 2022

Introduce device ctrl for device_led, ne_clock, etc., #46

Introduce device ctrl for device_led, ne_clock, etc., #46

Conversation

sukyoungjeong-furiosa commented Jul 4, 2022 • edited Loading

libc-furiosa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sukyoungjeong-furiosa Jul 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sukyoungjeong-furiosa Jul 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sukyoungjeong-furiosa commented Jul 5, 2022

libc-furiosa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

libc-furiosa left a comment

Choose a reason for hiding this comment

libc-furiosa commented Jul 5, 2022

sukyoungjeong-furiosa commented Jul 4, 2022 •

edited

Loading

sukyoungjeong-furiosa Jul 5, 2022 •

edited

Loading

sukyoungjeong-furiosa Jul 5, 2022 •

edited

Loading