u99127 commented on a change in pull request #6703: URL: https://github.com/apache/incubator-tvm/pull/6703#discussion_r509704644
########## File path: apps/microtvm-vm/base-box/Vagrantfile.packer-template ########## @@ -0,0 +1,45 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +Vagrant.configure("2") do |config| + # From hashicorp default template: + # https://github.com/hashicorp/packer/blob/master/builder/vagrant/step_create_vagrantfile.go#L23-L37 + + config.vm.define "source" do |source| + source.vm.box = "{{.SourceBox}}" + config.ssh.insert_key = {{.InsertKey}} Review comment: Where do users insert their ssh key and have you thought about security with this ? Is there a requirement to bake that into some image some where ? ########## File path: apps/microtvm-vm/base-box/packer.json.template ########## @@ -0,0 +1,43 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +{ + "variables": { + "provider": null, + "version": null, + "access_token": null, + "skip_add": "false" + }, + "builders": [ + { + "type": "vagrant", + "output_dir": "output-vagrant-{{user `provider`}}", + "communicator": "ssh", + "source_path": "generic/ubuntu2004", + "provider": "{{user `provider`}}", + "template": "Vagrantfile.packer-template", + "skip_add": "{{user `skip_add`}}" + } + ], + "post-processors": [ + { + "type": "vagrant-cloud", + "box_tag": "areusch/microtvm-staging", Review comment: What is areusch/microtvm-staging ? Should we be merging this in ? If we are putting this into main - I don't think this should have any information of this sort here. ########## File path: python/tvm/target/target.py ########## @@ -220,19 +220,24 @@ def intel_graphics(model="unknown", options=None): return Target(" ".join(["opencl"] + opts)) -def micro(hardware="unknown", options=None): +def micro(model="unknown", options=None): Review comment: Changes for target API ? Should this really be a separate PR ? ########## File path: python/tvm/micro/debugger.py ########## @@ -45,34 +56,102 @@ def stop(self): """Terminate the debugger.""" raise NotImplementedError() + def _run_on_terminate_callbacks(self): + for callback in self.on_terminate_callbacks: + try: + callback() + except Exception: # pylint: disable=broad-except + _LOG.warning("on_terminate_callback raised exception", exc_info=True) + class GdbDebugger(Debugger): """Handles launching, suspending signals, and potentially dealing with terminal issues.""" + # Number of seconds to wait in stop() for a graceful shutdown. After this time has elapsed, + # the debugger is kill()'d. + _GRACEFUL_SHUTDOWN_TIMEOUT_SEC = 5.0 + + # The instance of GdbDebugger that's currently started. + _STARTED_INSTANCE = None + + @classmethod + def _stop_all(cls): + if cls._STARTED_INSTANCE: + cls._STARTED_INSTANCE.stop() + + def __init__(self): + super(GdbDebugger, self).__init__() + self._is_running = False + self._child_alive_lock = threading.RLock() + self._is_child_alive = False + @abc.abstractmethod def popen_kwargs(self): raise NotImplementedError() - def _wait_restore_signal(self): + def _wait_for_child(self): self.popen.wait() - if not self.did_terminate.is_set(): - for callback in self.on_terminate_callbacks: + with self._child_alive_lock: + self._is_child_alive = True + + @classmethod + def _sigint_handler(cls, signum, stack_frame): # pylint: disable=unused-argument + if cls._STARTED_INSTANCE is not None: + with cls._STARTED_INSTANCE._child_alive_lock: + exists = cls._STARTED_INSTANCE._is_child_alive + if exists: try: - callback() - except Exception: # pylint: disable=broad-except - logging.warn("on_terminate_callback raised exception", exc_info=True) + os.killpg(cls._STARTED_INSTANCE.child_pgid, signal.SIGINT) + return + except ProcessLookupError: + pass + + raise Exception() def start(self): + assert not self._is_running + assert not self._STARTED_INSTANCE + kwargs = self.popen_kwargs() - self.did_terminate = threading.Event() - self.old_signal = signal.signal(signal.SIGINT, signal.SIG_IGN) + self.did_start_new_session = kwargs.setdefault("start_new_session", True) + + self.old_termios = termios.tcgetattr(sys.stdin.fileno()) self.popen = subprocess.Popen(**kwargs) - threading.Thread(target=self._wait_restore_signal).start() + self._is_running = True + self.__class__._STARTED_INSTANCE = self + try: + self.child_pgid = os.getpgid(self.popen.pid) + except Exception: + self.stop() + raise + with self._child_alive_lock: + self._is_child_alive = True + self.old_sigint_handler = signal.signal(signal.SIGINT, self._sigint_handler) + t = threading.Thread(target=self._wait_for_child) + t.daemon = True + t.start() def stop(self): - self.did_terminate.set() - self.popen.terminate() - signal.signal(signal.SIGINT, self.old_signal) + if not self._is_running: + return + + signal.signal(signal.SIGINT, self.old_sigint_handler) + termios.tcsetattr(sys.stdin.fileno(), termios.TCSAFLUSH, self.old_termios) + + try: + children = psutil.Process(self.popen.pid).children(recursive=True) + for c in children: + c.terminate() + _, alive = psutil.wait_procs(children, timeout=self._GRACEFUL_SHUTDOWN_TIMEOUT_SEC) + for a in alive: + a.kill() + finally: + self.__class__._STARTED_INSTANCE = None + self._is_running = False + self._run_on_terminate_callbacks() + + +atexit.register(GdbDebugger._stop_all) Review comment: More comments in this file would be useful. ########## File path: apps/microtvm-vm/base-box/Vagrantfile ########## @@ -0,0 +1,23 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +Vagrant.configure("2") do |config| + config.vm.box = "generic/ubuntu2004" Review comment: The rest of our CI requires 18.04 : why 20.04 here ? ########## File path: apps/microtvm-vm/base-box/release-version.sh ########## @@ -0,0 +1,53 @@ +#!/bin/bash -e +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + Review comment: Need a top level comment as to why this file exists. What's it's claim to fame, what's it supposed to be used for ? ########## File path: src/runtime/rpc/rpc_endpoint.cc ########## @@ -700,6 +700,14 @@ void RPCEndpoint::Init() { }); } +/*! + * \brief Create a new RPCEndpoint instance. + * \param channel RPCChannel used to communicate + * \param name + * \param The remote key reported during protocol initialization, or "%toinit" if the RPCEndpoint + * should handle this phase of the protocol for you. Some servers may prefer to access parts of + * the key to modify their behavior. + */ Review comment: Related to the RPC mechanism, why isn't this an independent PR ? ########## File path: apps/microtvm-vm/base-box/setup-tvm-user.sh ########## @@ -0,0 +1,39 @@ +#!/bin/bash -e +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# Zephyr +pip3 install --user -U west +echo 'export PATH=$HOME/.local/bin:"$PATH"' >> ~/.profile +source ~/.profile +echo PATH=$PATH +west init --mr v2.4.0 ~/zephyr +cd ~/zephyr +west update +west zephyr-export + Review comment: This appears to be zephyr specific and not uTVM specific. Is there really an expectation that the vagrant box will have a user named west ? What happens when we need other RTOS's ? ########## File path: python/tvm/micro/contrib/zephyr.py ########## @@ -40,6 +38,7 @@ from ..transport import debug from ..transport import file_descriptor +from ..transport import serial Review comment: why are changes in this file necessary with this PR , can this be split out into it's own Zephyr specific PR ? ########## File path: apps/microtvm-vm/vm/rebuild-tvm.sh ########## @@ -0,0 +1,33 @@ +#!/bin/bash -e +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +set -e + +cd "$(dirname $0)" +cd "$(git rev-parse --show-toplevel)" +BUILD_DIR=build-microtvm + +if [ ! -e "${BUILD_DIR}" ]; then + mkdir "${BUILD_DIR}" +fi +cp cmake/config.cmake "${BUILD_DIR}" +cd "${BUILD_DIR}" +sed -i 's/USE_MICRO OFF/USE_MICRO ON/' config.cmake +sed -i 's/USE_LLVM OFF/USE_LLVM ON/' config.cmake Review comment: This forces the use of uTVM with llvm only. how does the user choose between using llvm or the C backend or turning this off entirely ? ########## File path: apps/microtvm-vm/base-box/packer.json.template ########## @@ -0,0 +1,43 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +{ Review comment: Please link to a schema at the top level or something that is a reference to this json schema . What do these keys mean ? How much of an expert should a uTVM developer be to configure this or use it as is ? ########## File path: src/target/llvm/llvm_common.cc ########## @@ -60,10 +61,11 @@ void InitializeLLVM() { } void ParseLLVMTargetOptions(const Target& target, std::string* triple, std::string* mcpu, - std::string* mattr, llvm::TargetOptions* options) { Review comment: Please pull out the llvm target changes to a separate PR . ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
