* [PATCH 1/9] habanalabs: add handle field to context structure
2019-07-28 11:28 [PATCH 0/9] habanalabs: support open device by multiple processes Oded Gabbay
@ 2019-07-28 11:28 ` Oded Gabbay
2019-07-28 11:28 ` [PATCH 2/9] habanalabs: verify context is valid in IOCTLs Oded Gabbay
` (7 subsequent siblings)
8 siblings, 0 replies; 16+ messages in thread
From: Oded Gabbay @ 2019-07-28 11:28 UTC (permalink / raw)
To: linux-kernel, oshpigelman, ttayar, gregkh
This patch adds a field to the context's structure that will hold a unique
handle for the context.
This will be needed when the user will create the context.
Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com>
---
drivers/misc/habanalabs/context.c | 27 ++++++++++++++++-----------
drivers/misc/habanalabs/habanalabs.h | 2 ++
2 files changed, 18 insertions(+), 11 deletions(-)
diff --git a/drivers/misc/habanalabs/context.c b/drivers/misc/habanalabs/context.c
index 8682590e3f6e..1d8390418234 100644
--- a/drivers/misc/habanalabs/context.c
+++ b/drivers/misc/habanalabs/context.c
@@ -67,9 +67,20 @@ int hl_ctx_create(struct hl_device *hdev, struct hl_fpriv *hpriv)
goto out_err;
}
+ mutex_lock(&mgr->ctx_lock);
+ rc = idr_alloc(&mgr->ctx_handles, ctx, 1, 0, GFP_KERNEL);
+ mutex_unlock(&mgr->ctx_lock);
+
+ if (rc < 0) {
+ dev_err(hdev->dev, "Failed to allocate IDR for a new CTX\n");
+ goto free_ctx;
+ }
+
+ ctx->handle = rc;
+
rc = hl_ctx_init(hdev, ctx, false);
if (rc)
- goto free_ctx;
+ goto remove_from_idr;
hl_hpriv_get(hpriv);
ctx->hpriv = hpriv;
@@ -78,18 +89,12 @@ int hl_ctx_create(struct hl_device *hdev, struct hl_fpriv *hpriv)
hpriv->ctx = ctx;
hdev->user_ctx = ctx;
- mutex_lock(&mgr->ctx_lock);
- rc = idr_alloc(&mgr->ctx_handles, ctx, 1, 0, GFP_KERNEL);
- mutex_unlock(&mgr->ctx_lock);
-
- if (rc < 0) {
- dev_err(hdev->dev, "Failed to allocate IDR for a new CTX\n");
- hl_ctx_free(hdev, ctx);
- goto out_err;
- }
-
return 0;
+remove_from_idr:
+ mutex_lock(&mgr->ctx_lock);
+ idr_remove(&mgr->ctx_handles, ctx->handle);
+ mutex_unlock(&mgr->ctx_lock);
free_ctx:
kfree(ctx);
out_err:
diff --git a/drivers/misc/habanalabs/habanalabs.h b/drivers/misc/habanalabs/habanalabs.h
index 57183ae9b95d..e41800e68578 100644
--- a/drivers/misc/habanalabs/habanalabs.h
+++ b/drivers/misc/habanalabs/habanalabs.h
@@ -631,6 +631,7 @@ struct hl_va_range {
* execution phase before the context switch phase
* has finished.
* @asid: context's unique address space ID in the device's MMU.
+ * @handle: context's opaque handle for user
*/
struct hl_ctx {
DECLARE_HASHTABLE(mem_hash, MEM_HASH_TABLE_BITS);
@@ -652,6 +653,7 @@ struct hl_ctx {
atomic_t thread_ctx_switch_token;
u32 thread_ctx_switch_wait_token;
u32 asid;
+ u32 handle;
};
/**
--
2.17.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 2/9] habanalabs: verify context is valid in IOCTLs
2019-07-28 11:28 [PATCH 0/9] habanalabs: support open device by multiple processes Oded Gabbay
2019-07-28 11:28 ` [PATCH 1/9] habanalabs: add handle field to context structure Oded Gabbay
@ 2019-07-28 11:28 ` Oded Gabbay
2019-07-28 11:28 ` [PATCH 3/9] habanalabs: create context in lazy mode Oded Gabbay
` (6 subsequent siblings)
8 siblings, 0 replies; 16+ messages in thread
From: Oded Gabbay @ 2019-07-28 11:28 UTC (permalink / raw)
To: linux-kernel, oshpigelman, ttayar, gregkh
This patch adds to most IOCTL operations a check of whether the calling
process has a valid context.
This is in preparation for when contexts will be created by the user and
not the driver.
Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com>
---
drivers/misc/habanalabs/command_buffer.c | 6 ++++++
drivers/misc/habanalabs/command_submission.c | 12 ++++++++++++
drivers/misc/habanalabs/context.c | 7 ++++++-
drivers/misc/habanalabs/habanalabs.h | 5 +++--
drivers/misc/habanalabs/habanalabs_ioctl.c | 12 ++++++++++++
drivers/misc/habanalabs/memory.c | 6 ++++++
6 files changed, 45 insertions(+), 3 deletions(-)
diff --git a/drivers/misc/habanalabs/command_buffer.c b/drivers/misc/habanalabs/command_buffer.c
index e495f44064fa..981caa8ec068 100644
--- a/drivers/misc/habanalabs/command_buffer.c
+++ b/drivers/misc/habanalabs/command_buffer.c
@@ -221,6 +221,12 @@ int hl_cb_ioctl(struct hl_fpriv *hpriv, void *data)
return -EBUSY;
}
+ if (!hl_ctx_is_valid(hpriv)) {
+ dev_err_ratelimited(hdev->dev,
+ "Can't execute CB IOCTL, missing a valid context\n");
+ return -EACCES;
+ }
+
switch (args->in.op) {
case HL_CB_OP_CREATE:
rc = hl_cb_create(hdev, &hpriv->cb_mgr, args->in.cb_size,
diff --git a/drivers/misc/habanalabs/command_submission.c b/drivers/misc/habanalabs/command_submission.c
index f4e2c2ad0057..56910dee6026 100644
--- a/drivers/misc/habanalabs/command_submission.c
+++ b/drivers/misc/habanalabs/command_submission.c
@@ -613,6 +613,12 @@ int hl_cs_ioctl(struct hl_fpriv *hpriv, void *data)
goto out;
}
+ if (!hl_ctx_is_valid(hpriv)) {
+ dev_err_ratelimited(hdev->dev,
+ "Can't execute CS IOCTL, missing a valid context\n");
+ return -EACCES;
+ }
+
do_ctx_switch = atomic_cmpxchg(&ctx->thread_ctx_switch_token, 1, 0);
if (do_ctx_switch || (args->in.cs_flags & HL_CS_FLAGS_FORCE_RESTORE)) {
@@ -759,6 +765,12 @@ int hl_cs_wait_ioctl(struct hl_fpriv *hpriv, void *data)
u64 seq = args->in.seq;
long rc;
+ if (!hl_ctx_is_valid(hpriv)) {
+ dev_err_ratelimited(hdev->dev,
+ "Can't execute WAIT_CS IOCTL, missing a valid context\n");
+ return -EACCES;
+ }
+
rc = _hl_cs_wait_ioctl(hdev, hpriv->ctx, args->in.timeout_us, seq);
memset(args, 0, sizeof(*args));
diff --git a/drivers/misc/habanalabs/context.c b/drivers/misc/habanalabs/context.c
index 1d8390418234..771a1055e67b 100644
--- a/drivers/misc/habanalabs/context.c
+++ b/drivers/misc/habanalabs/context.c
@@ -85,7 +85,7 @@ int hl_ctx_create(struct hl_device *hdev, struct hl_fpriv *hpriv)
hl_hpriv_get(hpriv);
ctx->hpriv = hpriv;
- /* TODO: remove for multiple contexts */
+ /* TODO: remove for multiple contexts per process */
hpriv->ctx = ctx;
hdev->user_ctx = ctx;
@@ -196,6 +196,11 @@ struct dma_fence *hl_ctx_get_fence(struct hl_ctx *ctx, u64 seq)
return fence;
}
+bool hl_ctx_is_valid(struct hl_fpriv *hpriv)
+{
+ return hpriv->ctx ? true : false;
+}
+
/*
* hl_ctx_mgr_init - initialize the context manager
*
diff --git a/drivers/misc/habanalabs/habanalabs.h b/drivers/misc/habanalabs/habanalabs.h
index e41800e68578..475cdaa0005e 100644
--- a/drivers/misc/habanalabs/habanalabs.h
+++ b/drivers/misc/habanalabs/habanalabs.h
@@ -905,7 +905,7 @@ struct hl_debug_params {
* @hdev: habanalabs device structure.
* @filp: pointer to the given file structure.
* @taskpid: current process ID.
- * @ctx: current executing context.
+ * @ctx: current executing context. TODO: remove for multiple ctx per process
* @ctx_mgr: context manager to handle multiple context for this FD.
* @cb_mgr: command buffer manager to handle multiple buffers for this FD.
* @debugfs_list: list of relevant ASIC debugfs.
@@ -916,7 +916,7 @@ struct hl_fpriv {
struct hl_device *hdev;
struct file *filp;
struct pid *taskpid;
- struct hl_ctx *ctx; /* TODO: remove for multiple ctx */
+ struct hl_ctx *ctx;
struct hl_ctx_mgr ctx_mgr;
struct hl_cb_mgr cb_mgr;
struct list_head debugfs_list;
@@ -1422,6 +1422,7 @@ int hl_ctx_put(struct hl_ctx *ctx);
struct dma_fence *hl_ctx_get_fence(struct hl_ctx *ctx, u64 seq);
void hl_ctx_mgr_init(struct hl_ctx_mgr *mgr);
void hl_ctx_mgr_fini(struct hl_device *hdev, struct hl_ctx_mgr *mgr);
+bool hl_ctx_is_valid(struct hl_fpriv *hpriv);
int hl_device_init(struct hl_device *hdev, struct class *hclass);
void hl_device_fini(struct hl_device *hdev);
diff --git a/drivers/misc/habanalabs/habanalabs_ioctl.c b/drivers/misc/habanalabs/habanalabs_ioctl.c
index 07127576b3e8..60b18af66283 100644
--- a/drivers/misc/habanalabs/habanalabs_ioctl.c
+++ b/drivers/misc/habanalabs/habanalabs_ioctl.c
@@ -208,6 +208,12 @@ static int hl_info_ioctl(struct hl_fpriv *hpriv, void *data)
return -EBUSY;
}
+ if (!hl_ctx_is_valid(hpriv)) {
+ dev_err_ratelimited(hdev->dev,
+ "Can't execute INFO IOCTL, missing a valid context\n");
+ return -EACCES;
+ }
+
switch (args->op) {
case HL_INFO_HW_IP_INFO:
rc = hw_ip_info(hdev, args);
@@ -247,6 +253,12 @@ static int hl_debug_ioctl(struct hl_fpriv *hpriv, void *data)
return -EBUSY;
}
+ if (!hl_ctx_is_valid(hpriv)) {
+ dev_err_ratelimited(hdev->dev,
+ "Can't execute DEBUG IOCTL, missing a valid context\n");
+ return -EACCES;
+ }
+
switch (args->op) {
case HL_DEBUG_OP_ETR:
case HL_DEBUG_OP_ETF:
diff --git a/drivers/misc/habanalabs/memory.c b/drivers/misc/habanalabs/memory.c
index 42d237cae1dc..fddbca623bd2 100644
--- a/drivers/misc/habanalabs/memory.c
+++ b/drivers/misc/habanalabs/memory.c
@@ -1154,6 +1154,12 @@ int hl_mem_ioctl(struct hl_fpriv *hpriv, void *data)
return -EBUSY;
}
+ if (!hl_ctx_is_valid(hpriv)) {
+ dev_err_ratelimited(hdev->dev,
+ "Can't execute MEMORY IOCTL, missing a valid context\n");
+ return -EACCES;
+ }
+
if (!hdev->mmu_enable)
return mem_ioctl_no_mmu(hpriv, args);
--
2.17.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 3/9] habanalabs: create context in lazy mode
2019-07-28 11:28 [PATCH 0/9] habanalabs: support open device by multiple processes Oded Gabbay
2019-07-28 11:28 ` [PATCH 1/9] habanalabs: add handle field to context structure Oded Gabbay
2019-07-28 11:28 ` [PATCH 2/9] habanalabs: verify context is valid in IOCTLs Oded Gabbay
@ 2019-07-28 11:28 ` Oded Gabbay
2019-07-28 11:28 ` [PATCH 4/9] habanalabs: don't change frequency if user context is valid Oded Gabbay
` (5 subsequent siblings)
8 siblings, 0 replies; 16+ messages in thread
From: Oded Gabbay @ 2019-07-28 11:28 UTC (permalink / raw)
To: linux-kernel, oshpigelman, ttayar, gregkh
Instead of creating the context when the user opens the device, create the
context when the first IOCTL that needs a context is called.
This change is required to provide backward compatibility for older
user-space when the driver will require the user to create a context.
Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com>
---
drivers/misc/habanalabs/context.c | 29 +++++++++++++++++++++++-
drivers/misc/habanalabs/device.c | 5 +++-
drivers/misc/habanalabs/habanalabs.h | 2 ++
drivers/misc/habanalabs/habanalabs_drv.c | 13 -----------
4 files changed, 34 insertions(+), 15 deletions(-)
diff --git a/drivers/misc/habanalabs/context.c b/drivers/misc/habanalabs/context.c
index 771a1055e67b..3c1f7c29e908 100644
--- a/drivers/misc/habanalabs/context.c
+++ b/drivers/misc/habanalabs/context.c
@@ -198,7 +198,34 @@ struct dma_fence *hl_ctx_get_fence(struct hl_ctx *ctx, u64 seq)
bool hl_ctx_is_valid(struct hl_fpriv *hpriv)
{
- return hpriv->ctx ? true : false;
+ struct hl_device *hdev = hpriv->hdev;
+ bool valid = true;
+ int rc;
+
+ /* First thing, to minimize latency impact, check if context exists.
+ * If so, exit immediately
+ */
+ if (hpriv->ctx)
+ return true;
+
+ mutex_lock(&hdev->lazy_ctx_creation_lock);
+
+ /* We must check again the existence of the context. This is because
+ * there could be competing threads in the same process that call
+ * IOCTLs at the same time. In this case, we must protect against double
+ * creation of a context
+ */
+ if (!hpriv->ctx) {
+ rc = hl_ctx_create(hdev, hpriv);
+ if (rc) {
+ dev_err(hdev->dev, "Failed to create context %d\n", rc);
+ valid = false;
+ }
+ }
+
+ mutex_unlock(&hdev->lazy_ctx_creation_lock);
+
+ return valid;
}
/*
diff --git a/drivers/misc/habanalabs/device.c b/drivers/misc/habanalabs/device.c
index 0c4894dd9c02..b63beb73ec76 100644
--- a/drivers/misc/habanalabs/device.c
+++ b/drivers/misc/habanalabs/device.c
@@ -58,8 +58,9 @@ static void hpriv_release(struct kref *ref)
/* Now the FD is really closed */
atomic_dec(&hdev->fd_open_cnt);
- /* This allows a new user context to open the device */
+ mutex_lock(&hdev->lazy_ctx_creation_lock);
hdev->user_ctx = NULL;
+ mutex_unlock(&hdev->lazy_ctx_creation_lock);
}
void hl_hpriv_get(struct hl_fpriv *hpriv)
@@ -233,6 +234,7 @@ static int device_early_init(struct hl_device *hdev)
mutex_init(&hdev->send_cpu_message_lock);
mutex_init(&hdev->debug_lock);
mutex_init(&hdev->mmu_cache_lock);
+ mutex_init(&hdev->lazy_ctx_creation_lock);
INIT_LIST_HEAD(&hdev->hw_queues_mirror_list);
spin_lock_init(&hdev->hw_queues_mirror_lock);
atomic_set(&hdev->in_reset, 0);
@@ -262,6 +264,7 @@ static int device_early_init(struct hl_device *hdev)
*/
static void device_early_fini(struct hl_device *hdev)
{
+ mutex_destroy(&hdev->lazy_ctx_creation_lock);
mutex_destroy(&hdev->mmu_cache_lock);
mutex_destroy(&hdev->debug_lock);
mutex_destroy(&hdev->send_cpu_message_lock);
diff --git a/drivers/misc/habanalabs/habanalabs.h b/drivers/misc/habanalabs/habanalabs.h
index 475cdaa0005e..d0e55839d673 100644
--- a/drivers/misc/habanalabs/habanalabs.h
+++ b/drivers/misc/habanalabs/habanalabs.h
@@ -1183,6 +1183,7 @@ struct hl_device_reset_work {
* @cb_pool: list of preallocated CBs.
* @cb_pool_lock: protects the CB pool.
* @user_ctx: current user context executing.
+ * @lazy_ctx_creation_lock: lock to protect lazy creation of context
* @dram_used_mem: current DRAM memory consumption.
* @timeout_jiffies: device CS timeout value.
* @max_power: the max power of the device, as configured by the sysadmin. This
@@ -1261,6 +1262,7 @@ struct hl_device {
/* TODO: remove user_ctx for multiple process support */
struct hl_ctx *user_ctx;
+ struct mutex lazy_ctx_creation_lock;
atomic64_t dram_used_mem;
u64 timeout_jiffies;
diff --git a/drivers/misc/habanalabs/habanalabs_drv.c b/drivers/misc/habanalabs/habanalabs_drv.c
index 678f61646ca9..b2c52e46e130 100644
--- a/drivers/misc/habanalabs/habanalabs_drv.c
+++ b/drivers/misc/habanalabs/habanalabs_drv.c
@@ -141,12 +141,6 @@ int hl_device_open(struct inode *inode, struct file *filp)
hl_cb_mgr_init(&hpriv->cb_mgr);
hl_ctx_mgr_init(&hpriv->ctx_mgr);
- rc = hl_ctx_create(hdev, hpriv);
- if (rc) {
- dev_err(hdev->dev, "Failed to open FD (CTX fail)\n");
- goto out_err;
- }
-
hpriv->taskpid = find_get_pid(current->pid);
/*
@@ -160,13 +154,6 @@ int hl_device_open(struct inode *inode, struct file *filp)
return 0;
-out_err:
- filp->private_data = NULL;
- hl_ctx_mgr_fini(hpriv->hdev, &hpriv->ctx_mgr);
- hl_cb_mgr_fini(hpriv->hdev, &hpriv->cb_mgr);
- mutex_destroy(&hpriv->restore_phase_mutex);
- kfree(hpriv);
-
close_device:
atomic_dec(&hdev->fd_open_cnt);
return rc;
--
2.17.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 4/9] habanalabs: don't change frequency if user context is valid
2019-07-28 11:28 [PATCH 0/9] habanalabs: support open device by multiple processes Oded Gabbay
` (2 preceding siblings ...)
2019-07-28 11:28 ` [PATCH 3/9] habanalabs: create context in lazy mode Oded Gabbay
@ 2019-07-28 11:28 ` Oded Gabbay
2019-07-28 11:28 ` [PATCH 5/9] habanalabs: maintain a list of file private data objects Oded Gabbay
` (4 subsequent siblings)
8 siblings, 0 replies; 16+ messages in thread
From: Oded Gabbay @ 2019-07-28 11:28 UTC (permalink / raw)
To: linux-kernel, oshpigelman, ttayar, gregkh
Instead of using the FD open counter to check if there is a user opened on
the device, check if there is a valid user context.
Use the new lazy context creation mutex to protect against multiple calls
to change the frequency.
This is in preparation for removing the FD open counter.
Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com>
---
drivers/misc/habanalabs/context.c | 9 +++++
drivers/misc/habanalabs/device.c | 40 +++++++++--------------
drivers/misc/habanalabs/goya/goya_hwmgr.c | 11 +++----
drivers/misc/habanalabs/habanalabs.h | 4 +--
drivers/misc/habanalabs/habanalabs_drv.c | 34 ++++++++-----------
5 files changed, 45 insertions(+), 53 deletions(-)
diff --git a/drivers/misc/habanalabs/context.c b/drivers/misc/habanalabs/context.c
index 3c1f7c29e908..a4cd47ced94d 100644
--- a/drivers/misc/habanalabs/context.c
+++ b/drivers/misc/habanalabs/context.c
@@ -220,9 +220,18 @@ bool hl_ctx_is_valid(struct hl_fpriv *hpriv)
if (rc) {
dev_err(hdev->dev, "Failed to create context %d\n", rc);
valid = false;
+ goto unlock_mutex;
}
+
+ /* Device is IDLE at this point so it is legal to change PLLs.
+ * There is no need to check anything because if the PLL is
+ * already HIGH, the set function will return without doing
+ * anything
+ */
+ hl_device_set_frequency(hdev, PLL_HIGH);
}
+unlock_mutex:
mutex_unlock(&hdev->lazy_ctx_creation_lock);
return valid;
diff --git a/drivers/misc/habanalabs/device.c b/drivers/misc/habanalabs/device.c
index b63beb73ec76..a791a1b58215 100644
--- a/drivers/misc/habanalabs/device.c
+++ b/drivers/misc/habanalabs/device.c
@@ -289,9 +289,13 @@ static void set_freq_to_low_job(struct work_struct *work)
struct hl_device *hdev = container_of(work, struct hl_device,
work_freq.work);
- if (atomic_read(&hdev->fd_open_cnt) == 0)
+ mutex_lock(&hdev->lazy_ctx_creation_lock);
+
+ if (!hdev->user_ctx)
hl_device_set_frequency(hdev, PLL_LOW);
+ mutex_unlock(&hdev->lazy_ctx_creation_lock);
+
schedule_delayed_work(&hdev->work_freq,
usecs_to_jiffies(HL_PLL_LOW_JOB_FREQ_USEC));
}
@@ -341,7 +345,7 @@ static int device_late_init(struct hl_device *hdev)
hdev->high_pll = hdev->asic_prop.high_pll;
/* force setting to low frequency */
- atomic_set(&hdev->curr_pll_profile, PLL_LOW);
+ hdev->curr_pll_profile = PLL_LOW;
if (hdev->pm_mng_profile == PM_AUTO)
hdev->asic_funcs->set_pll_profile(hdev, PLL_LOW);
@@ -390,38 +394,26 @@ static void device_late_fini(struct hl_device *hdev)
* @hdev: pointer to habanalabs device structure
* @freq: the new frequency value
*
- * Change the frequency if needed.
- * We allose to set PLL to low only if there is no user process
- * Returns 0 if no change was done, otherwise returns 1;
+ * Change the frequency if needed. This function has no protection against
+ * concurrency, therefore it is assumed that the calling function has protected
+ * itself against the case of calling this function from multiple threads with
+ * different values
+ *
+ * Returns 0 if no change was done, otherwise returns 1
*/
int hl_device_set_frequency(struct hl_device *hdev, enum hl_pll_frequency freq)
{
- enum hl_pll_frequency old_freq =
- (freq == PLL_HIGH) ? PLL_LOW : PLL_HIGH;
- int ret;
-
- if (hdev->pm_mng_profile == PM_MANUAL)
- return 0;
-
- ret = atomic_cmpxchg(&hdev->curr_pll_profile, old_freq, freq);
- if (ret == freq)
+ if ((hdev->pm_mng_profile == PM_MANUAL) ||
+ (hdev->curr_pll_profile == freq))
return 0;
- /*
- * in case we want to lower frequency, check if device is not
- * opened. We must have a check here to workaround race condition with
- * hl_device_open
- */
- if ((freq == PLL_LOW) && (atomic_read(&hdev->fd_open_cnt) > 0)) {
- atomic_set(&hdev->curr_pll_profile, PLL_HIGH);
- return 0;
- }
-
dev_dbg(hdev->dev, "Changing device frequency to %s\n",
freq == PLL_HIGH ? "high" : "low");
hdev->asic_funcs->set_pll_profile(hdev, freq);
+ hdev->curr_pll_profile = freq;
+
return 1;
}
diff --git a/drivers/misc/habanalabs/goya/goya_hwmgr.c b/drivers/misc/habanalabs/goya/goya_hwmgr.c
index a51d836542a1..8522c6e0a25e 100644
--- a/drivers/misc/habanalabs/goya/goya_hwmgr.c
+++ b/drivers/misc/habanalabs/goya/goya_hwmgr.c
@@ -254,11 +254,11 @@ static ssize_t pm_mng_profile_store(struct device *dev,
goto out;
}
- mutex_lock(&hdev->fd_open_cnt_lock);
+ mutex_lock(&hdev->lazy_ctx_creation_lock);
- if (atomic_read(&hdev->fd_open_cnt) > 0) {
+ if (hdev->user_ctx) {
dev_err(hdev->dev,
- "Can't change PM profile while user process is opened on the device\n");
+ "Can't change PM profile while user context is opened on the device\n");
count = -EPERM;
goto unlock_mutex;
}
@@ -266,7 +266,7 @@ static ssize_t pm_mng_profile_store(struct device *dev,
if (strncmp("auto", buf, strlen("auto")) == 0) {
/* Make sure we are in LOW PLL when changing modes */
if (hdev->pm_mng_profile == PM_MANUAL) {
- atomic_set(&hdev->curr_pll_profile, PLL_HIGH);
+ hdev->curr_pll_profile = PLL_HIGH;
hl_device_set_frequency(hdev, PLL_LOW);
hdev->pm_mng_profile = PM_AUTO;
}
@@ -279,11 +279,10 @@ static ssize_t pm_mng_profile_store(struct device *dev,
} else {
dev_err(hdev->dev, "value should be auto or manual\n");
count = -EINVAL;
- goto unlock_mutex;
}
unlock_mutex:
- mutex_unlock(&hdev->fd_open_cnt_lock);
+ mutex_unlock(&hdev->lazy_ctx_creation_lock);
out:
return count;
}
diff --git a/drivers/misc/habanalabs/habanalabs.h b/drivers/misc/habanalabs/habanalabs.h
index d0e55839d673..6354fc88ef8a 100644
--- a/drivers/misc/habanalabs/habanalabs.h
+++ b/drivers/misc/habanalabs/habanalabs.h
@@ -1190,10 +1190,10 @@ struct hl_device_reset_work {
* value is saved so in case of hard-reset, KMD will restore this
* value and update the F/W after the re-initialization
* @in_reset: is device in reset flow.
- * @curr_pll_profile: current PLL profile.
* @fd_open_cnt: number of open user processes.
* @cs_active_cnt: number of active command submissions on this device (active
* means already in H/W queues)
+ * @curr_pll_profile: current PLL profile.
* @major: habanalabs KMD major.
* @high_pll: high PLL profile frequency.
* @soft_reset_cnt: number of soft reset since KMD loading.
@@ -1268,9 +1268,9 @@ struct hl_device {
u64 timeout_jiffies;
u64 max_power;
atomic_t in_reset;
- atomic_t curr_pll_profile;
atomic_t fd_open_cnt;
atomic_t cs_active_cnt;
+ enum hl_pll_frequency curr_pll_profile;
u32 major;
u32 high_pll;
u32 soft_reset_cnt;
diff --git a/drivers/misc/habanalabs/habanalabs_drv.c b/drivers/misc/habanalabs/habanalabs_drv.c
index b2c52e46e130..a781aa936160 100644
--- a/drivers/misc/habanalabs/habanalabs_drv.c
+++ b/drivers/misc/habanalabs/habanalabs_drv.c
@@ -95,42 +95,40 @@ int hl_device_open(struct inode *inode, struct file *filp)
return -ENXIO;
}
+ hpriv = kzalloc(sizeof(*hpriv), GFP_KERNEL);
+ if (!hpriv)
+ return -ENOMEM;
+
mutex_lock(&hdev->fd_open_cnt_lock);
if (hl_device_disabled_or_in_reset(hdev)) {
dev_err_ratelimited(hdev->dev,
"Can't open %s because it is disabled or in reset\n",
dev_name(hdev->dev));
- mutex_unlock(&hdev->fd_open_cnt_lock);
- return -EPERM;
+ rc = -EPERM;
+ goto out_err;
}
if (hdev->in_debug) {
dev_err_ratelimited(hdev->dev,
"Can't open %s because it is being debugged by another user\n",
dev_name(hdev->dev));
- mutex_unlock(&hdev->fd_open_cnt_lock);
- return -EPERM;
+ rc = -EPERM;
+ goto out_err;
}
if (atomic_read(&hdev->fd_open_cnt)) {
dev_info_ratelimited(hdev->dev,
"Can't open %s because another user is working on it\n",
dev_name(hdev->dev));
- mutex_unlock(&hdev->fd_open_cnt_lock);
- return -EBUSY;
+ rc = -EBUSY;
+ goto out_err;
}
atomic_inc(&hdev->fd_open_cnt);
mutex_unlock(&hdev->fd_open_cnt_lock);
- hpriv = kzalloc(sizeof(*hpriv), GFP_KERNEL);
- if (!hpriv) {
- rc = -ENOMEM;
- goto close_device;
- }
-
hpriv->hdev = hdev;
filp->private_data = hpriv;
hpriv->filp = filp;
@@ -143,19 +141,13 @@ int hl_device_open(struct inode *inode, struct file *filp)
hpriv->taskpid = find_get_pid(current->pid);
- /*
- * Device is IDLE at this point so it is legal to change PLLs. There
- * is no need to check anything because if the PLL is already HIGH, the
- * set function will return without doing anything
- */
- hl_device_set_frequency(hdev, PLL_HIGH);
-
hl_debugfs_add_file(hpriv);
return 0;
-close_device:
- atomic_dec(&hdev->fd_open_cnt);
+out_err:
+ mutex_unlock(&hdev->fd_open_cnt_lock);
+ kfree(hpriv);
return rc;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 5/9] habanalabs: maintain a list of file private data objects
2019-07-28 11:28 [PATCH 0/9] habanalabs: support open device by multiple processes Oded Gabbay
` (3 preceding siblings ...)
2019-07-28 11:28 ` [PATCH 4/9] habanalabs: don't change frequency if user context is valid Oded Gabbay
@ 2019-07-28 11:28 ` Oded Gabbay
2019-07-28 11:28 ` [PATCH 6/9] habanalabs: define user context as compute context Oded Gabbay
` (3 subsequent siblings)
8 siblings, 0 replies; 16+ messages in thread
From: Oded Gabbay @ 2019-07-28 11:28 UTC (permalink / raw)
To: linux-kernel, oshpigelman, ttayar, gregkh
This patch adds a new list to the driver's device structure. The list will
keep the file private data structures that the driver creates when a user
process opens the device.
This change is needed because it is useless to try to count how many FD
are open. Instead, save the private data structure and once it is
released, remove it from the list. As long as the list is not empty, it
means we have a user that can do something with our device.
This is also needed in preparation for when the user will open a context.
When that happens, we can remove the restriction of denying a user to
open the device if the list is not empty.
Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com>
---
drivers/misc/habanalabs/device.c | 40 +++++++++++++-----------
drivers/misc/habanalabs/habanalabs.h | 20 ++++++------
drivers/misc/habanalabs/habanalabs_drv.c | 11 +++----
3 files changed, 36 insertions(+), 35 deletions(-)
diff --git a/drivers/misc/habanalabs/device.c b/drivers/misc/habanalabs/device.c
index a791a1b58215..efeeaa2dc915 100644
--- a/drivers/misc/habanalabs/device.c
+++ b/drivers/misc/habanalabs/device.c
@@ -53,10 +53,14 @@ static void hpriv_release(struct kref *ref)
mutex_destroy(&hpriv->restore_phase_mutex);
- kfree(hpriv);
+ /* Remove private data node from the device list. This enables
+ * another process to open the device
+ */
+ mutex_lock(&hdev->fpriv_list_lock);
+ list_del(&hpriv->dev_node);
+ mutex_unlock(&hdev->fpriv_list_lock);
- /* Now the FD is really closed */
- atomic_dec(&hdev->fd_open_cnt);
+ kfree(hpriv);
mutex_lock(&hdev->lazy_ctx_creation_lock);
hdev->user_ctx = NULL;
@@ -230,15 +234,15 @@ static int device_early_init(struct hl_device *hdev)
hl_cb_mgr_init(&hdev->kernel_cb_mgr);
- mutex_init(&hdev->fd_open_cnt_lock);
mutex_init(&hdev->send_cpu_message_lock);
mutex_init(&hdev->debug_lock);
mutex_init(&hdev->mmu_cache_lock);
mutex_init(&hdev->lazy_ctx_creation_lock);
INIT_LIST_HEAD(&hdev->hw_queues_mirror_list);
spin_lock_init(&hdev->hw_queues_mirror_lock);
+ INIT_LIST_HEAD(&hdev->fpriv_list);
+ mutex_init(&hdev->fpriv_list_lock);
atomic_set(&hdev->in_reset, 0);
- atomic_set(&hdev->fd_open_cnt, 0);
atomic_set(&hdev->cs_active_cnt, 0);
return 0;
@@ -269,6 +273,8 @@ static void device_early_fini(struct hl_device *hdev)
mutex_destroy(&hdev->debug_lock);
mutex_destroy(&hdev->send_cpu_message_lock);
+ mutex_destroy(&hdev->fpriv_list_lock);
+
hl_cb_mgr_fini(hdev, &hdev->kernel_cb_mgr);
kfree(hdev->hl_chip_info);
@@ -280,8 +286,6 @@ static void device_early_fini(struct hl_device *hdev)
if (hdev->asic_funcs->early_fini)
hdev->asic_funcs->early_fini(hdev);
-
- mutex_destroy(&hdev->fd_open_cnt_lock);
}
static void set_freq_to_low_job(struct work_struct *work)
@@ -444,19 +448,19 @@ int hl_device_set_debug_mode(struct hl_device *hdev, bool enable)
goto out;
}
- mutex_lock(&hdev->fd_open_cnt_lock);
+ mutex_lock(&hdev->fpriv_list_lock);
- if (atomic_read(&hdev->fd_open_cnt) > 1) {
+ if (!list_is_singular(&hdev->fpriv_list)) {
dev_err(hdev->dev,
"Failed to enable debug mode. More then a single user is using the device\n");
rc = -EPERM;
- goto unlock_fd_open_lock;
+ goto unlock_fpriv_list_lock;
}
hdev->in_debug = 1;
-unlock_fd_open_lock:
- mutex_unlock(&hdev->fd_open_cnt_lock);
+unlock_fpriv_list_lock:
+ mutex_unlock(&hdev->fpriv_list_lock);
out:
mutex_unlock(&hdev->debug_lock);
@@ -573,9 +577,9 @@ static void device_kill_open_processes(struct hl_device *hdev)
pending_cnt = pending_total;
/* Flush all processes that are inside hl_open */
- mutex_lock(&hdev->fd_open_cnt_lock);
+ mutex_lock(&hdev->fpriv_list_lock);
- while ((atomic_read(&hdev->fd_open_cnt)) && (pending_cnt)) {
+ while ((!list_empty(&hdev->fpriv_list)) && (pending_cnt)) {
pending_cnt--;
@@ -584,7 +588,7 @@ static void device_kill_open_processes(struct hl_device *hdev)
ssleep(1);
}
- if (atomic_read(&hdev->fd_open_cnt)) {
+ if (!list_empty(&hdev->fpriv_list)) {
task = get_pid_task(hdev->user_ctx->hpriv->taskpid,
PIDTYPE_PID);
if (task) {
@@ -604,18 +608,18 @@ static void device_kill_open_processes(struct hl_device *hdev)
pending_cnt = pending_total;
- while ((atomic_read(&hdev->fd_open_cnt)) && (pending_cnt)) {
+ while ((!list_empty(&hdev->fpriv_list)) && (pending_cnt)) {
pending_cnt--;
ssleep(1);
}
- if (atomic_read(&hdev->fd_open_cnt))
+ if (!list_empty(&hdev->fpriv_list))
dev_crit(hdev->dev,
"Going to hard reset with open user contexts\n");
- mutex_unlock(&hdev->fd_open_cnt_lock);
+ mutex_unlock(&hdev->fpriv_list_lock);
}
diff --git a/drivers/misc/habanalabs/habanalabs.h b/drivers/misc/habanalabs/habanalabs.h
index 6354fc88ef8a..589eb40bb95d 100644
--- a/drivers/misc/habanalabs/habanalabs.h
+++ b/drivers/misc/habanalabs/habanalabs.h
@@ -909,6 +909,7 @@ struct hl_debug_params {
* @ctx_mgr: context manager to handle multiple context for this FD.
* @cb_mgr: command buffer manager to handle multiple buffers for this FD.
* @debugfs_list: list of relevant ASIC debugfs.
+ * @dev_node: node in the device list of file private data
* @refcount: number of related contexts.
* @restore_phase_mutex: lock for context switch and restore phase.
*/
@@ -920,6 +921,7 @@ struct hl_fpriv {
struct hl_ctx_mgr ctx_mgr;
struct hl_cb_mgr cb_mgr;
struct list_head debugfs_list;
+ struct list_head dev_node;
struct kref refcount;
struct mutex restore_phase_mutex;
};
@@ -1161,12 +1163,6 @@ struct hl_device_reset_work {
* @cpu_accessible_dma_pool: KMD <-> ArmCP shared memory pool.
* @asid_bitmap: holds used/available ASIDs.
* @asid_mutex: protects asid_bitmap.
- * @fd_open_cnt_lock: lock for updating fd_open_cnt in hl_device_open. Although
- * fd_open_cnt is atomic, we need this lock to serialize
- * the open function because the driver currently supports
- * only a single process at a time. In addition, we need a
- * lock here so we can flush user processes which are opening
- * the device while we are trying to hard reset it
* @send_cpu_message_lock: enforces only one message in KMD <-> ArmCP queue.
* @debug_lock: protects critical section of setting debug mode for device
* @asic_prop: ASIC specific immutable properties.
@@ -1182,6 +1178,9 @@ struct hl_device_reset_work {
* @hl_debugfs: device's debugfs manager.
* @cb_pool: list of preallocated CBs.
* @cb_pool_lock: protects the CB pool.
+ * @fpriv_list: list of file private data structures. Each structure is created
+ * when a user opens the device
+ * @fpriv_list_lock: protects the fpriv_list
* @user_ctx: current user context executing.
* @lazy_ctx_creation_lock: lock to protect lazy creation of context
* @dram_used_mem: current DRAM memory consumption.
@@ -1190,7 +1189,6 @@ struct hl_device_reset_work {
* value is saved so in case of hard-reset, KMD will restore this
* value and update the F/W after the re-initialization
* @in_reset: is device in reset flow.
- * @fd_open_cnt: number of open user processes.
* @cs_active_cnt: number of active command submissions on this device (active
* means already in H/W queues)
* @curr_pll_profile: current PLL profile.
@@ -1212,7 +1210,7 @@ struct hl_device_reset_work {
* @mmu_enable: is MMU enabled.
* @device_cpu_disabled: is the device CPU disabled (due to timeouts)
* @dma_mask: the dma mask that was set for this device
- * @in_debug: is device under debug. This, together with fd_open_cnt, enforces
+ * @in_debug: is device under debug. This, together with fpriv_list, enforces
* that only a single user is configuring the debug infrastructure.
*/
struct hl_device {
@@ -1240,8 +1238,6 @@ struct hl_device {
struct gen_pool *cpu_accessible_dma_pool;
unsigned long *asid_bitmap;
struct mutex asid_mutex;
- /* TODO: remove fd_open_cnt_lock for multiple process support */
- struct mutex fd_open_cnt_lock;
struct mutex send_cpu_message_lock;
struct mutex debug_lock;
struct asic_fixed_properties asic_prop;
@@ -1260,6 +1256,9 @@ struct hl_device {
struct list_head cb_pool;
spinlock_t cb_pool_lock;
+ struct list_head fpriv_list;
+ struct mutex fpriv_list_lock;
+
/* TODO: remove user_ctx for multiple process support */
struct hl_ctx *user_ctx;
struct mutex lazy_ctx_creation_lock;
@@ -1268,7 +1267,6 @@ struct hl_device {
u64 timeout_jiffies;
u64 max_power;
atomic_t in_reset;
- atomic_t fd_open_cnt;
atomic_t cs_active_cnt;
enum hl_pll_frequency curr_pll_profile;
u32 major;
diff --git a/drivers/misc/habanalabs/habanalabs_drv.c b/drivers/misc/habanalabs/habanalabs_drv.c
index a781aa936160..d990b30c0701 100644
--- a/drivers/misc/habanalabs/habanalabs_drv.c
+++ b/drivers/misc/habanalabs/habanalabs_drv.c
@@ -99,7 +99,7 @@ int hl_device_open(struct inode *inode, struct file *filp)
if (!hpriv)
return -ENOMEM;
- mutex_lock(&hdev->fd_open_cnt_lock);
+ mutex_lock(&hdev->fpriv_list_lock);
if (hl_device_disabled_or_in_reset(hdev)) {
dev_err_ratelimited(hdev->dev,
@@ -117,7 +117,7 @@ int hl_device_open(struct inode *inode, struct file *filp)
goto out_err;
}
- if (atomic_read(&hdev->fd_open_cnt)) {
+ if (!list_empty(&hdev->fpriv_list)) {
dev_info_ratelimited(hdev->dev,
"Can't open %s because another user is working on it\n",
dev_name(hdev->dev));
@@ -125,9 +125,8 @@ int hl_device_open(struct inode *inode, struct file *filp)
goto out_err;
}
- atomic_inc(&hdev->fd_open_cnt);
-
- mutex_unlock(&hdev->fd_open_cnt_lock);
+ list_add(&hpriv->dev_node, &hdev->fpriv_list);
+ mutex_unlock(&hdev->fpriv_list_lock);
hpriv->hdev = hdev;
filp->private_data = hpriv;
@@ -146,7 +145,7 @@ int hl_device_open(struct inode *inode, struct file *filp)
return 0;
out_err:
- mutex_unlock(&hdev->fd_open_cnt_lock);
+ mutex_unlock(&hdev->fpriv_list_lock);
kfree(hpriv);
return rc;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 6/9] habanalabs: define user context as compute context
2019-07-28 11:28 [PATCH 0/9] habanalabs: support open device by multiple processes Oded Gabbay
` (4 preceding siblings ...)
2019-07-28 11:28 ` [PATCH 5/9] habanalabs: maintain a list of file private data objects Oded Gabbay
@ 2019-07-28 11:28 ` Oded Gabbay
2019-07-28 11:28 ` [PATCH 7/9] habanalabs: protect only pointer dereference in hard-reset Oded Gabbay
` (2 subsequent siblings)
8 siblings, 0 replies; 16+ messages in thread
From: Oded Gabbay @ 2019-07-28 11:28 UTC (permalink / raw)
To: linux-kernel, oshpigelman, ttayar, gregkh
This patch changes the definition of the user_ctx field in the device
structure. It renames the field to "compute_context" and adds a couple of
checks:
1. When checking if a context is valid, the calling function must say it
requires a compute context. So valid now means exists + capability.
2. In context lazy creation, if the calling function requires compute
context, and one already exists, we can't create an additional compute
context.
3. When a context finishes, we remove the debug mode, if the device was
set to debug mode. The patch changes this code so the remove will happen
only if the compute context finished.
This patch is in preparation to supporting multiple processes.
Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com>
---
drivers/misc/habanalabs/command_buffer.c | 2 +-
drivers/misc/habanalabs/command_submission.c | 4 +-
drivers/misc/habanalabs/context.c | 59 +++++++++++++-------
drivers/misc/habanalabs/debugfs.c | 4 +-
drivers/misc/habanalabs/device.c | 16 +++---
drivers/misc/habanalabs/goya/goya_hwmgr.c | 4 +-
drivers/misc/habanalabs/habanalabs.h | 7 +--
drivers/misc/habanalabs/habanalabs_ioctl.c | 7 ++-
drivers/misc/habanalabs/memory.c | 2 +-
9 files changed, 63 insertions(+), 42 deletions(-)
diff --git a/drivers/misc/habanalabs/command_buffer.c b/drivers/misc/habanalabs/command_buffer.c
index 981caa8ec068..a0409ff38d18 100644
--- a/drivers/misc/habanalabs/command_buffer.c
+++ b/drivers/misc/habanalabs/command_buffer.c
@@ -221,7 +221,7 @@ int hl_cb_ioctl(struct hl_fpriv *hpriv, void *data)
return -EBUSY;
}
- if (!hl_ctx_is_valid(hpriv)) {
+ if (!hl_ctx_is_valid(hpriv, true)) {
dev_err_ratelimited(hdev->dev,
"Can't execute CB IOCTL, missing a valid context\n");
return -EACCES;
diff --git a/drivers/misc/habanalabs/command_submission.c b/drivers/misc/habanalabs/command_submission.c
index 56910dee6026..c8de16a3e3ed 100644
--- a/drivers/misc/habanalabs/command_submission.c
+++ b/drivers/misc/habanalabs/command_submission.c
@@ -613,7 +613,7 @@ int hl_cs_ioctl(struct hl_fpriv *hpriv, void *data)
goto out;
}
- if (!hl_ctx_is_valid(hpriv)) {
+ if (!hl_ctx_is_valid(hpriv, true)) {
dev_err_ratelimited(hdev->dev,
"Can't execute CS IOCTL, missing a valid context\n");
return -EACCES;
@@ -765,7 +765,7 @@ int hl_cs_wait_ioctl(struct hl_fpriv *hpriv, void *data)
u64 seq = args->in.seq;
long rc;
- if (!hl_ctx_is_valid(hpriv)) {
+ if (!hl_ctx_is_valid(hpriv, true)) {
dev_err_ratelimited(hdev->dev,
"Can't execute WAIT_CS IOCTL, missing a valid context\n");
return -EACCES;
diff --git a/drivers/misc/habanalabs/context.c b/drivers/misc/habanalabs/context.c
index a4cd47ced94d..57bbe59da9b6 100644
--- a/drivers/misc/habanalabs/context.c
+++ b/drivers/misc/habanalabs/context.c
@@ -26,12 +26,13 @@ static void hl_ctx_fini(struct hl_ctx *ctx)
dma_fence_put(ctx->cs_pending[i]);
if (ctx->asid != HL_KERNEL_ASID_ID) {
- /*
- * The engines are stopped as there is no executing CS, but the
+ /* The engines are stopped as there is no executing CS, but the
* Coresight might be still working by accessing addresses
* related to the stopped engines. Hence stop it explicitly.
+ * Stop only if this is the compute context, as there can be
+ * only one compute context
*/
- if (hdev->in_debug)
+ if ((hdev->in_debug) && (hdev->compute_ctx == ctx))
hl_device_set_debug_mode(hdev, false);
hl_vm_ctx_fini(ctx);
@@ -87,7 +88,9 @@ int hl_ctx_create(struct hl_device *hdev, struct hl_fpriv *hpriv)
/* TODO: remove for multiple contexts per process */
hpriv->ctx = ctx;
- hdev->user_ctx = ctx;
+
+ /* TODO: remove the following line for multiple process support */
+ hdev->compute_ctx = ctx;
return 0;
@@ -196,17 +199,20 @@ struct dma_fence *hl_ctx_get_fence(struct hl_ctx *ctx, u64 seq)
return fence;
}
-bool hl_ctx_is_valid(struct hl_fpriv *hpriv)
+bool hl_ctx_is_valid(struct hl_fpriv *hpriv, bool requires_compute_ctx)
{
struct hl_device *hdev = hpriv->hdev;
bool valid = true;
int rc;
/* First thing, to minimize latency impact, check if context exists.
- * If so, exit immediately
+ * Also check if it matches the requirements. If so, exit immediately
*/
- if (hpriv->ctx)
+ if (hpriv->ctx) {
+ if ((requires_compute_ctx) && (hdev->compute_ctx != hpriv->ctx))
+ return false;
return true;
+ }
mutex_lock(&hdev->lazy_ctx_creation_lock);
@@ -215,22 +221,37 @@ bool hl_ctx_is_valid(struct hl_fpriv *hpriv)
* IOCTLs at the same time. In this case, we must protect against double
* creation of a context
*/
- if (!hpriv->ctx) {
- rc = hl_ctx_create(hdev, hpriv);
- if (rc) {
- dev_err(hdev->dev, "Failed to create context %d\n", rc);
+ if (hpriv->ctx) {
+ if ((requires_compute_ctx) && (hdev->compute_ctx != hpriv->ctx))
valid = false;
- goto unlock_mutex;
- }
+ goto unlock_mutex;
+ }
- /* Device is IDLE at this point so it is legal to change PLLs.
- * There is no need to check anything because if the PLL is
- * already HIGH, the set function will return without doing
- * anything
- */
- hl_device_set_frequency(hdev, PLL_HIGH);
+ /* If we already have a compute context, there is no point
+ * of creating one in case we are called from ioctl that needs
+ * a compute context
+ */
+ if ((hdev->compute_ctx) && (requires_compute_ctx)) {
+ dev_err(hdev->dev,
+ "Can't create new compute context as one already exists\n");
+ valid = false;
+ goto unlock_mutex;
+ }
+
+ rc = hl_ctx_create(hdev, hpriv);
+ if (rc) {
+ dev_err(hdev->dev, "Failed to create context %d\n", rc);
+ valid = false;
+ goto unlock_mutex;
}
+ /* Device is IDLE at this point so it is legal to change PLLs.
+ * There is no need to check anything because if the PLL is
+ * already HIGH, the set function will return without doing
+ * anything
+ */
+ hl_device_set_frequency(hdev, PLL_HIGH);
+
unlock_mutex:
mutex_unlock(&hdev->lazy_ctx_creation_lock);
diff --git a/drivers/misc/habanalabs/debugfs.c b/drivers/misc/habanalabs/debugfs.c
index 18e499c900c7..2b9bc1c41d40 100644
--- a/drivers/misc/habanalabs/debugfs.c
+++ b/drivers/misc/habanalabs/debugfs.c
@@ -370,7 +370,7 @@ static int mmu_show(struct seq_file *s, void *data)
if (dev_entry->mmu_asid == HL_KERNEL_ASID_ID)
ctx = hdev->kernel_ctx;
else
- ctx = hdev->user_ctx;
+ ctx = hdev->compute_ctx;
if (!ctx) {
dev_err(hdev->dev, "no ctx available\n");
@@ -533,7 +533,7 @@ static bool hl_is_device_va(struct hl_device *hdev, u64 addr)
static int device_va_to_pa(struct hl_device *hdev, u64 virt_addr,
u64 *phys_addr)
{
- struct hl_ctx *ctx = hdev->user_ctx;
+ struct hl_ctx *ctx = hdev->compute_ctx;
u64 hop_addr, hop_pte_addr, hop_pte;
u64 offset_mask = HOP4_MASK | OFFSET_MASK;
int rc = 0;
diff --git a/drivers/misc/habanalabs/device.c b/drivers/misc/habanalabs/device.c
index efeeaa2dc915..5400e65ba5fa 100644
--- a/drivers/misc/habanalabs/device.c
+++ b/drivers/misc/habanalabs/device.c
@@ -63,7 +63,7 @@ static void hpriv_release(struct kref *ref)
kfree(hpriv);
mutex_lock(&hdev->lazy_ctx_creation_lock);
- hdev->user_ctx = NULL;
+ hdev->compute_ctx = NULL;
mutex_unlock(&hdev->lazy_ctx_creation_lock);
}
@@ -295,7 +295,7 @@ static void set_freq_to_low_job(struct work_struct *work)
mutex_lock(&hdev->lazy_ctx_creation_lock);
- if (!hdev->user_ctx)
+ if (!hdev->compute_ctx)
hl_device_set_frequency(hdev, PLL_LOW);
mutex_unlock(&hdev->lazy_ctx_creation_lock);
@@ -589,7 +589,7 @@ static void device_kill_open_processes(struct hl_device *hdev)
}
if (!list_empty(&hdev->fpriv_list)) {
- task = get_pid_task(hdev->user_ctx->hpriv->taskpid,
+ task = get_pid_task(hdev->compute_ctx->hpriv->taskpid,
PIDTYPE_PID);
if (task) {
dev_info(hdev->dev, "Killing user processes\n");
@@ -754,9 +754,9 @@ int hl_device_reset(struct hl_device *hdev, bool hard_reset,
hl_cq_reset(hdev, &hdev->completion_queue[i]);
/* Make sure the context switch phase will run again */
- if (hdev->user_ctx) {
- atomic_set(&hdev->user_ctx->thread_ctx_switch_token, 1);
- hdev->user_ctx->thread_ctx_switch_wait_token = 0;
+ if (hdev->compute_ctx) {
+ atomic_set(&hdev->compute_ctx->thread_ctx_switch_token, 1);
+ hdev->compute_ctx->thread_ctx_switch_wait_token = 0;
}
/* Finished tear-down, starting to re-initialize */
@@ -787,7 +787,7 @@ int hl_device_reset(struct hl_device *hdev, bool hard_reset,
goto out_err;
}
- hdev->user_ctx = NULL;
+ hdev->compute_ctx = NULL;
rc = hl_ctx_init(hdev, hdev->kernel_ctx, true);
if (rc) {
@@ -964,7 +964,7 @@ int hl_device_init(struct hl_device *hdev, struct class *hclass)
goto mmu_fini;
}
- hdev->user_ctx = NULL;
+ hdev->compute_ctx = NULL;
rc = hl_ctx_init(hdev, hdev->kernel_ctx, true);
if (rc) {
diff --git a/drivers/misc/habanalabs/goya/goya_hwmgr.c b/drivers/misc/habanalabs/goya/goya_hwmgr.c
index 8522c6e0a25e..0d83660b2860 100644
--- a/drivers/misc/habanalabs/goya/goya_hwmgr.c
+++ b/drivers/misc/habanalabs/goya/goya_hwmgr.c
@@ -256,9 +256,9 @@ static ssize_t pm_mng_profile_store(struct device *dev,
mutex_lock(&hdev->lazy_ctx_creation_lock);
- if (hdev->user_ctx) {
+ if (hdev->compute_ctx) {
dev_err(hdev->dev,
- "Can't change PM profile while user context is opened on the device\n");
+ "Can't change PM profile while compute context is opened on the device\n");
count = -EPERM;
goto unlock_mutex;
}
diff --git a/drivers/misc/habanalabs/habanalabs.h b/drivers/misc/habanalabs/habanalabs.h
index 589eb40bb95d..6a6c71fd5eef 100644
--- a/drivers/misc/habanalabs/habanalabs.h
+++ b/drivers/misc/habanalabs/habanalabs.h
@@ -1181,7 +1181,7 @@ struct hl_device_reset_work {
* @fpriv_list: list of file private data structures. Each structure is created
* when a user opens the device
* @fpriv_list_lock: protects the fpriv_list
- * @user_ctx: current user context executing.
+ * @compute_ctx: current compute context executing.
* @lazy_ctx_creation_lock: lock to protect lazy creation of context
* @dram_used_mem: current DRAM memory consumption.
* @timeout_jiffies: device CS timeout value.
@@ -1259,8 +1259,7 @@ struct hl_device {
struct list_head fpriv_list;
struct mutex fpriv_list_lock;
- /* TODO: remove user_ctx for multiple process support */
- struct hl_ctx *user_ctx;
+ struct hl_ctx *compute_ctx;
struct mutex lazy_ctx_creation_lock;
atomic64_t dram_used_mem;
@@ -1422,7 +1421,7 @@ int hl_ctx_put(struct hl_ctx *ctx);
struct dma_fence *hl_ctx_get_fence(struct hl_ctx *ctx, u64 seq);
void hl_ctx_mgr_init(struct hl_ctx_mgr *mgr);
void hl_ctx_mgr_fini(struct hl_device *hdev, struct hl_ctx_mgr *mgr);
-bool hl_ctx_is_valid(struct hl_fpriv *hpriv);
+bool hl_ctx_is_valid(struct hl_fpriv *hpriv, bool requires_compute_ctx);
int hl_device_init(struct hl_device *hdev, struct class *hclass);
void hl_device_fini(struct hl_device *hdev);
diff --git a/drivers/misc/habanalabs/habanalabs_ioctl.c b/drivers/misc/habanalabs/habanalabs_ioctl.c
index 60b18af66283..8d84f2ac302d 100644
--- a/drivers/misc/habanalabs/habanalabs_ioctl.c
+++ b/drivers/misc/habanalabs/habanalabs_ioctl.c
@@ -104,7 +104,8 @@ static int dram_usage_info(struct hl_device *hdev, struct hl_info_args *args)
prop->dram_base_address);
dram_usage.dram_free_mem = (prop->dram_size - dram_kmd_size) -
atomic64_read(&hdev->dram_used_mem);
- dram_usage.ctx_dram_mem = atomic64_read(&hdev->user_ctx->dram_phys_mem);
+ dram_usage.ctx_dram_mem =
+ atomic64_read(&hdev->compute_ctx->dram_phys_mem);
return copy_to_user(out, &dram_usage,
min((size_t) max_size, sizeof(dram_usage))) ? -EFAULT : 0;
@@ -208,7 +209,7 @@ static int hl_info_ioctl(struct hl_fpriv *hpriv, void *data)
return -EBUSY;
}
- if (!hl_ctx_is_valid(hpriv)) {
+ if (!hl_ctx_is_valid(hpriv, false)) {
dev_err_ratelimited(hdev->dev,
"Can't execute INFO IOCTL, missing a valid context\n");
return -EACCES;
@@ -253,7 +254,7 @@ static int hl_debug_ioctl(struct hl_fpriv *hpriv, void *data)
return -EBUSY;
}
- if (!hl_ctx_is_valid(hpriv)) {
+ if (!hl_ctx_is_valid(hpriv, true)) {
dev_err_ratelimited(hdev->dev,
"Can't execute DEBUG IOCTL, missing a valid context\n");
return -EACCES;
diff --git a/drivers/misc/habanalabs/memory.c b/drivers/misc/habanalabs/memory.c
index fddbca623bd2..c55243b918c5 100644
--- a/drivers/misc/habanalabs/memory.c
+++ b/drivers/misc/habanalabs/memory.c
@@ -1154,7 +1154,7 @@ int hl_mem_ioctl(struct hl_fpriv *hpriv, void *data)
return -EBUSY;
}
- if (!hl_ctx_is_valid(hpriv)) {
+ if (!hl_ctx_is_valid(hpriv, true)) {
dev_err_ratelimited(hdev->dev,
"Can't execute MEMORY IOCTL, missing a valid context\n");
return -EACCES;
--
2.17.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 7/9] habanalabs: protect only pointer dereference in hard-reset
2019-07-28 11:28 [PATCH 0/9] habanalabs: support open device by multiple processes Oded Gabbay
` (5 preceding siblings ...)
2019-07-28 11:28 ` [PATCH 6/9] habanalabs: define user context as compute context Oded Gabbay
@ 2019-07-28 11:28 ` Oded Gabbay
2019-07-28 11:28 ` [PATCH 8/9] habanalabs: kill user process after CS rollback Oded Gabbay
2019-07-28 11:28 ` [PATCH 9/9] habanalabs: allow multiple processes to open FD Oded Gabbay
8 siblings, 0 replies; 16+ messages in thread
From: Oded Gabbay @ 2019-07-28 11:28 UTC (permalink / raw)
To: linux-kernel, oshpigelman, ttayar, gregkh
This patch changes the location of taking a mutex lock and releasing it
during the hard-reset process of the ASIC.
The only place we need to protect is when we dereference pointers that may
go away in case the user process aborts/closes the FD.
That way, we allow the user process to actually close its FD in case we
tell him that an error occurred.
Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com>
---
drivers/misc/habanalabs/device.c | 26 ++++++++++++++------------
1 file changed, 14 insertions(+), 12 deletions(-)
diff --git a/drivers/misc/habanalabs/device.c b/drivers/misc/habanalabs/device.c
index 5400e65ba5fa..471506b54217 100644
--- a/drivers/misc/habanalabs/device.c
+++ b/drivers/misc/habanalabs/device.c
@@ -574,20 +574,21 @@ static void device_kill_open_processes(struct hl_device *hdev)
else
pending_total = HL_PENDING_RESET_PER_SEC;
- pending_cnt = pending_total;
-
/* Flush all processes that are inside hl_open */
mutex_lock(&hdev->fpriv_list_lock);
+ mutex_unlock(&hdev->fpriv_list_lock);
- while ((!list_empty(&hdev->fpriv_list)) && (pending_cnt)) {
-
- pending_cnt--;
-
- dev_info(hdev->dev,
- "Can't HARD reset, waiting for user to close FD\n");
+ /* Giving time for user to close FD, and for processes that are inside
+ * hl_device_open to finish
+ */
+ if (!list_empty(&hdev->fpriv_list))
ssleep(1);
- }
+ mutex_lock(&hdev->fpriv_list_lock);
+
+ /* This section must be protected because we are dereferencing
+ * pointers that are freed if the process exits
+ */
if (!list_empty(&hdev->fpriv_list)) {
task = get_pid_task(hdev->compute_ctx->hpriv->taskpid,
PIDTYPE_PID);
@@ -600,6 +601,8 @@ static void device_kill_open_processes(struct hl_device *hdev)
}
}
+ mutex_unlock(&hdev->fpriv_list_lock);
+
/* We killed the open users, but because the driver cleans up after the
* user contexts are closed (e.g. mmu mappings), we need to wait again
* to make sure the cleaning phase is finished before continuing with
@@ -609,6 +612,8 @@ static void device_kill_open_processes(struct hl_device *hdev)
pending_cnt = pending_total;
while ((!list_empty(&hdev->fpriv_list)) && (pending_cnt)) {
+ dev_info(hdev->dev,
+ "Waiting for all unmap operations to finish before hard reset\n");
pending_cnt--;
@@ -618,9 +623,6 @@ static void device_kill_open_processes(struct hl_device *hdev)
if (!list_empty(&hdev->fpriv_list))
dev_crit(hdev->dev,
"Going to hard reset with open user contexts\n");
-
- mutex_unlock(&hdev->fpriv_list_lock);
-
}
static void device_hard_reset_pending(struct work_struct *work)
--
2.17.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 8/9] habanalabs: kill user process after CS rollback
2019-07-28 11:28 [PATCH 0/9] habanalabs: support open device by multiple processes Oded Gabbay
` (6 preceding siblings ...)
2019-07-28 11:28 ` [PATCH 7/9] habanalabs: protect only pointer dereference in hard-reset Oded Gabbay
@ 2019-07-28 11:28 ` Oded Gabbay
2019-07-28 11:28 ` [PATCH 9/9] habanalabs: allow multiple processes to open FD Oded Gabbay
8 siblings, 0 replies; 16+ messages in thread
From: Oded Gabbay @ 2019-07-28 11:28 UTC (permalink / raw)
To: linux-kernel, oshpigelman, ttayar, gregkh
This patch calls the kill user process function after we rollback the
in-flight CSs. This is because the user process can't be closed while
there are open CSs. Therefore, there is no point of sending it a SIGKILL
before we do the rollback CS part.
Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com>
---
drivers/misc/habanalabs/device.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/misc/habanalabs/device.c b/drivers/misc/habanalabs/device.c
index 471506b54217..2677160c01b8 100644
--- a/drivers/misc/habanalabs/device.c
+++ b/drivers/misc/habanalabs/device.c
@@ -631,8 +631,6 @@ static void device_hard_reset_pending(struct work_struct *work)
container_of(work, struct hl_device_reset_work, reset_work);
struct hl_device *hdev = device_reset_work->hdev;
- device_kill_open_processes(hdev);
-
hl_device_reset(hdev, true, true);
kfree(device_reset_work);
@@ -737,6 +735,13 @@ int hl_device_reset(struct hl_device *hdev, bool hard_reset,
/* Go over all the queues, release all CS and their jobs */
hl_cs_rollback_all(hdev);
+ /* Kill processes here after CS rollback. This is because the process
+ * can't really exit until all its CSs are done, which is what we
+ * do in cs rollback
+ */
+ if (from_hard_reset_thread)
+ device_kill_open_processes(hdev);
+
/* Release kernel context */
if ((hard_reset) && (hl_ctx_put(hdev->kernel_ctx) == 1))
hdev->kernel_ctx = NULL;
@@ -1131,8 +1136,6 @@ void hl_device_fini(struct hl_device *hdev)
hdev->hard_reset_pending = true;
- device_kill_open_processes(hdev);
-
hl_hwmon_fini(hdev);
device_late_fini(hdev);
@@ -1151,6 +1154,12 @@ void hl_device_fini(struct hl_device *hdev)
/* Go over all the queues, release all CS and their jobs */
hl_cs_rollback_all(hdev);
+ /* Kill processes here after CS rollback. This is because the process
+ * can't really exit until all its CSs are done, which is what we
+ * do in cs rollback
+ */
+ device_kill_open_processes(hdev);
+
hl_cb_pool_fini(hdev);
/* Release kernel context */
--
2.17.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 9/9] habanalabs: allow multiple processes to open FD
2019-07-28 11:28 [PATCH 0/9] habanalabs: support open device by multiple processes Oded Gabbay
` (7 preceding siblings ...)
2019-07-28 11:28 ` [PATCH 8/9] habanalabs: kill user process after CS rollback Oded Gabbay
@ 2019-07-28 11:28 ` Oded Gabbay
2019-07-28 11:44 ` Greg KH
8 siblings, 1 reply; 16+ messages in thread
From: Oded Gabbay @ 2019-07-28 11:28 UTC (permalink / raw)
To: linux-kernel, oshpigelman, ttayar, gregkh
This patch removes the limitation of a single process that can open the
device.
Now, there is no limitation on the number of processes that can open the
device and have a valid FD.
However, only a single process can perform compute operations. This is
enforced by allowing only a single process to have a compute context.
Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com>
---
drivers/misc/habanalabs/context.c | 100 +++++++++++++++------
drivers/misc/habanalabs/device.c | 18 ++--
drivers/misc/habanalabs/habanalabs.h | 1 -
drivers/misc/habanalabs/habanalabs_drv.c | 8 --
drivers/misc/habanalabs/habanalabs_ioctl.c | 7 +-
5 files changed, 85 insertions(+), 49 deletions(-)
diff --git a/drivers/misc/habanalabs/context.c b/drivers/misc/habanalabs/context.c
index 57bbe59da9b6..f64220fc3a55 100644
--- a/drivers/misc/habanalabs/context.c
+++ b/drivers/misc/habanalabs/context.c
@@ -56,7 +56,7 @@ void hl_ctx_do_release(struct kref *ref)
kfree(ctx);
}
-int hl_ctx_create(struct hl_device *hdev, struct hl_fpriv *hpriv)
+static int hl_ctx_create(struct hl_device *hdev, struct hl_fpriv *hpriv)
{
struct hl_ctx_mgr *mgr = &hpriv->ctx_mgr;
struct hl_ctx *ctx;
@@ -89,9 +89,6 @@ int hl_ctx_create(struct hl_device *hdev, struct hl_fpriv *hpriv)
/* TODO: remove for multiple contexts per process */
hpriv->ctx = ctx;
- /* TODO: remove the following line for multiple process support */
- hdev->compute_ctx = ctx;
-
return 0;
remove_from_idr:
@@ -206,13 +203,22 @@ bool hl_ctx_is_valid(struct hl_fpriv *hpriv, bool requires_compute_ctx)
int rc;
/* First thing, to minimize latency impact, check if context exists.
- * Also check if it matches the requirements. If so, exit immediately
+ * This is relevant for the "steady state", where a process context
+ * already exists, and we want to minimize the latency in command
+ * submissions. In that case, we want to see if we can quickly exit
+ * with a valid answer.
+ *
+ * If a context doesn't exists, we must grab the mutex. Otherwise,
+ * there can be nasty races in case of multi-threaded application.
+ *
+ * So, if the context exists and we don't need a compute context,
+ * that's fine. If it exists and the context we have is the compute
+ * context, that's also fine. Other then that, we can't check anything
+ * without the mutex.
*/
- if (hpriv->ctx) {
- if ((requires_compute_ctx) && (hdev->compute_ctx != hpriv->ctx))
- return false;
+ if ((hpriv->ctx) && ((!requires_compute_ctx) ||
+ (hdev->compute_ctx == hpriv->ctx)))
return true;
- }
mutex_lock(&hdev->lazy_ctx_creation_lock);
@@ -222,35 +228,73 @@ bool hl_ctx_is_valid(struct hl_fpriv *hpriv, bool requires_compute_ctx)
* creation of a context
*/
if (hpriv->ctx) {
- if ((requires_compute_ctx) && (hdev->compute_ctx != hpriv->ctx))
+ if ((!requires_compute_ctx) ||
+ (hdev->compute_ctx == hpriv->ctx))
+ goto unlock_mutex;
+
+ if (hdev->compute_ctx) {
valid = false;
- goto unlock_mutex;
- }
+ goto unlock_mutex;
+ }
- /* If we already have a compute context, there is no point
- * of creating one in case we are called from ioctl that needs
- * a compute context
- */
- if ((hdev->compute_ctx) && (requires_compute_ctx)) {
+ /* If we reached here, it means we have a non-compute context,
+ * but there is no compute context on the device. Therefore,
+ * we can try to "upgrade" the existing context to a compute
+ * context
+ */
+ dev_dbg_ratelimited(hdev->dev,
+ "Non-compute context %d exists\n",
+ hpriv->ctx->asid);
+
+ } else if ((hdev->compute_ctx) && (requires_compute_ctx)) {
+
+ /* If we already have a compute context in the device, there is
+ * no point of creating one in case we are called from ioctl
+ * that needs a compute context
+ */
dev_err(hdev->dev,
"Can't create new compute context as one already exists\n");
valid = false;
goto unlock_mutex;
- }
+ } else {
+ /* If we reached here it is because there isn't a context for
+ * the process AND there is no compute context or compute
+ * context wasn't required. In any case, must create a context
+ * for the process
+ */
- rc = hl_ctx_create(hdev, hpriv);
- if (rc) {
- dev_err(hdev->dev, "Failed to create context %d\n", rc);
- valid = false;
- goto unlock_mutex;
+ rc = hl_ctx_create(hdev, hpriv);
+ if (rc) {
+ dev_err(hdev->dev, "Failed to create context %d\n", rc);
+ valid = false;
+ goto unlock_mutex;
+ }
+
+ dev_dbg_ratelimited(hdev->dev, "Created context %d\n",
+ hpriv->ctx->asid);
}
- /* Device is IDLE at this point so it is legal to change PLLs.
- * There is no need to check anything because if the PLL is
- * already HIGH, the set function will return without doing
- * anything
+ /* If we reached here then either we have a new context, or we can
+ * upgrade a non-compute context to a compute context. Do the upgrade
+ * only if the caller required a compute context
*/
- hl_device_set_frequency(hdev, PLL_HIGH);
+ if (requires_compute_ctx) {
+ WARN(hdev->compute_ctx,
+ "Compute context exists but driver is setting a new one");
+
+ dev_dbg_ratelimited(hdev->dev,
+ "Setting context %d as compute\n",
+ hpriv->ctx->asid);
+
+ hdev->compute_ctx = hpriv->ctx;
+
+ /* Device is IDLE at this point so it is legal to change PLLs.
+ * There is no need to check anything because if the PLL is
+ * already HIGH, the set function will return without doing
+ * anything
+ */
+ hl_device_set_frequency(hdev, PLL_HIGH);
+ }
unlock_mutex:
mutex_unlock(&hdev->lazy_ctx_creation_lock);
diff --git a/drivers/misc/habanalabs/device.c b/drivers/misc/habanalabs/device.c
index 2677160c01b8..e99a6d2b9893 100644
--- a/drivers/misc/habanalabs/device.c
+++ b/drivers/misc/habanalabs/device.c
@@ -42,10 +42,12 @@ static void hpriv_release(struct kref *ref)
{
struct hl_fpriv *hpriv;
struct hl_device *hdev;
+ struct hl_ctx *ctx;
hpriv = container_of(ref, struct hl_fpriv, refcount);
hdev = hpriv->hdev;
+ ctx = hpriv->ctx;
put_pid(hpriv->taskpid);
@@ -53,9 +55,6 @@ static void hpriv_release(struct kref *ref)
mutex_destroy(&hpriv->restore_phase_mutex);
- /* Remove private data node from the device list. This enables
- * another process to open the device
- */
mutex_lock(&hdev->fpriv_list_lock);
list_del(&hpriv->dev_node);
mutex_unlock(&hdev->fpriv_list_lock);
@@ -63,7 +62,8 @@ static void hpriv_release(struct kref *ref)
kfree(hpriv);
mutex_lock(&hdev->lazy_ctx_creation_lock);
- hdev->compute_ctx = NULL;
+ if (hdev->compute_ctx == ctx)
+ hdev->compute_ctx = NULL;
mutex_unlock(&hdev->lazy_ctx_creation_lock);
}
@@ -567,6 +567,7 @@ int hl_device_resume(struct hl_device *hdev)
static void device_kill_open_processes(struct hl_device *hdev)
{
u16 pending_total, pending_cnt;
+ struct hl_fpriv *hpriv;
struct task_struct *task = NULL;
if (hdev->pldm)
@@ -589,13 +590,12 @@ static void device_kill_open_processes(struct hl_device *hdev)
/* This section must be protected because we are dereferencing
* pointers that are freed if the process exits
*/
- if (!list_empty(&hdev->fpriv_list)) {
- task = get_pid_task(hdev->compute_ctx->hpriv->taskpid,
- PIDTYPE_PID);
+ list_for_each_entry(hpriv, &hdev->fpriv_list, dev_node) {
+ task = get_pid_task(hpriv->taskpid, PIDTYPE_PID);
if (task) {
- dev_info(hdev->dev, "Killing user processes\n");
+ dev_info(hdev->dev, "Killing user process\n");
send_sig(SIGKILL, task, 1);
- msleep(100);
+ usleep_range(1000, 10000);
put_task_struct(task);
}
diff --git a/drivers/misc/habanalabs/habanalabs.h b/drivers/misc/habanalabs/habanalabs.h
index 6a6c71fd5eef..43123d00d046 100644
--- a/drivers/misc/habanalabs/habanalabs.h
+++ b/drivers/misc/habanalabs/habanalabs.h
@@ -1412,7 +1412,6 @@ void hl_asid_fini(struct hl_device *hdev);
unsigned long hl_asid_alloc(struct hl_device *hdev);
void hl_asid_free(struct hl_device *hdev, unsigned long asid);
-int hl_ctx_create(struct hl_device *hdev, struct hl_fpriv *hpriv);
void hl_ctx_free(struct hl_device *hdev, struct hl_ctx *ctx);
int hl_ctx_init(struct hl_device *hdev, struct hl_ctx *ctx, bool is_kernel_ctx);
void hl_ctx_do_release(struct kref *ref);
diff --git a/drivers/misc/habanalabs/habanalabs_drv.c b/drivers/misc/habanalabs/habanalabs_drv.c
index d990b30c0701..b21f9724a652 100644
--- a/drivers/misc/habanalabs/habanalabs_drv.c
+++ b/drivers/misc/habanalabs/habanalabs_drv.c
@@ -117,14 +117,6 @@ int hl_device_open(struct inode *inode, struct file *filp)
goto out_err;
}
- if (!list_empty(&hdev->fpriv_list)) {
- dev_info_ratelimited(hdev->dev,
- "Can't open %s because another user is working on it\n",
- dev_name(hdev->dev));
- rc = -EBUSY;
- goto out_err;
- }
-
list_add(&hpriv->dev_node, &hdev->fpriv_list);
mutex_unlock(&hdev->fpriv_list_lock);
diff --git a/drivers/misc/habanalabs/habanalabs_ioctl.c b/drivers/misc/habanalabs/habanalabs_ioctl.c
index 8d84f2ac302d..452fd419a0cd 100644
--- a/drivers/misc/habanalabs/habanalabs_ioctl.c
+++ b/drivers/misc/habanalabs/habanalabs_ioctl.c
@@ -89,8 +89,9 @@ static int hw_events_info(struct hl_device *hdev, struct hl_info_args *args)
return copy_to_user(out, arr, min(max_size, size)) ? -EFAULT : 0;
}
-static int dram_usage_info(struct hl_device *hdev, struct hl_info_args *args)
+static int dram_usage_info(struct hl_fpriv *hpriv, struct hl_info_args *args)
{
+ struct hl_device *hdev = hpriv->hdev;
struct hl_info_dram_usage dram_usage = {0};
u32 max_size = args->return_size;
void __user *out = (void __user *) (uintptr_t) args->return_pointer;
@@ -105,7 +106,7 @@ static int dram_usage_info(struct hl_device *hdev, struct hl_info_args *args)
dram_usage.dram_free_mem = (prop->dram_size - dram_kmd_size) -
atomic64_read(&hdev->dram_used_mem);
dram_usage.ctx_dram_mem =
- atomic64_read(&hdev->compute_ctx->dram_phys_mem);
+ atomic64_read(&hpriv->ctx->dram_phys_mem);
return copy_to_user(out, &dram_usage,
min((size_t) max_size, sizeof(dram_usage))) ? -EFAULT : 0;
@@ -225,7 +226,7 @@ static int hl_info_ioctl(struct hl_fpriv *hpriv, void *data)
break;
case HL_INFO_DRAM_USAGE:
- rc = dram_usage_info(hdev, args);
+ rc = dram_usage_info(hpriv, args);
break;
case HL_INFO_HW_IDLE:
--
2.17.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 9/9] habanalabs: allow multiple processes to open FD
2019-07-28 11:28 ` [PATCH 9/9] habanalabs: allow multiple processes to open FD Oded Gabbay
@ 2019-07-28 11:44 ` Greg KH
2019-07-28 11:56 ` Oded Gabbay
0 siblings, 1 reply; 16+ messages in thread
From: Greg KH @ 2019-07-28 11:44 UTC (permalink / raw)
To: Oded Gabbay; +Cc: linux-kernel, oshpigelman, ttayar
On Sun, Jul 28, 2019 at 02:28:18PM +0300, Oded Gabbay wrote:
> This patch removes the limitation of a single process that can open the
> device.
>
> Now, there is no limitation on the number of processes that can open the
> device and have a valid FD.
>
> However, only a single process can perform compute operations. This is
> enforced by allowing only a single process to have a compute context.
>
> Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com>
> ---
> drivers/misc/habanalabs/context.c | 100 +++++++++++++++------
> drivers/misc/habanalabs/device.c | 18 ++--
> drivers/misc/habanalabs/habanalabs.h | 1 -
> drivers/misc/habanalabs/habanalabs_drv.c | 8 --
> drivers/misc/habanalabs/habanalabs_ioctl.c | 7 +-
> 5 files changed, 85 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/misc/habanalabs/context.c b/drivers/misc/habanalabs/context.c
> index 57bbe59da9b6..f64220fc3a55 100644
> --- a/drivers/misc/habanalabs/context.c
> +++ b/drivers/misc/habanalabs/context.c
> @@ -56,7 +56,7 @@ void hl_ctx_do_release(struct kref *ref)
> kfree(ctx);
> }
>
> -int hl_ctx_create(struct hl_device *hdev, struct hl_fpriv *hpriv)
> +static int hl_ctx_create(struct hl_device *hdev, struct hl_fpriv *hpriv)
> {
> struct hl_ctx_mgr *mgr = &hpriv->ctx_mgr;
> struct hl_ctx *ctx;
> @@ -89,9 +89,6 @@ int hl_ctx_create(struct hl_device *hdev, struct hl_fpriv *hpriv)
> /* TODO: remove for multiple contexts per process */
> hpriv->ctx = ctx;
>
> - /* TODO: remove the following line for multiple process support */
> - hdev->compute_ctx = ctx;
> -
> return 0;
>
> remove_from_idr:
> @@ -206,13 +203,22 @@ bool hl_ctx_is_valid(struct hl_fpriv *hpriv, bool requires_compute_ctx)
> int rc;
>
> /* First thing, to minimize latency impact, check if context exists.
> - * Also check if it matches the requirements. If so, exit immediately
> + * This is relevant for the "steady state", where a process context
> + * already exists, and we want to minimize the latency in command
> + * submissions. In that case, we want to see if we can quickly exit
> + * with a valid answer.
> + *
> + * If a context doesn't exists, we must grab the mutex. Otherwise,
> + * there can be nasty races in case of multi-threaded application.
> + *
> + * So, if the context exists and we don't need a compute context,
> + * that's fine. If it exists and the context we have is the compute
> + * context, that's also fine. Other then that, we can't check anything
> + * without the mutex.
> */
> - if (hpriv->ctx) {
> - if ((requires_compute_ctx) && (hdev->compute_ctx != hpriv->ctx))
> - return false;
> + if ((hpriv->ctx) && ((!requires_compute_ctx) ||
> + (hdev->compute_ctx == hpriv->ctx)))
> return true;
> - }
>
> mutex_lock(&hdev->lazy_ctx_creation_lock);
>
> @@ -222,35 +228,73 @@ bool hl_ctx_is_valid(struct hl_fpriv *hpriv, bool requires_compute_ctx)
> * creation of a context
> */
> if (hpriv->ctx) {
> - if ((requires_compute_ctx) && (hdev->compute_ctx != hpriv->ctx))
> + if ((!requires_compute_ctx) ||
> + (hdev->compute_ctx == hpriv->ctx))
> + goto unlock_mutex;
> +
> + if (hdev->compute_ctx) {
> valid = false;
> - goto unlock_mutex;
> - }
> + goto unlock_mutex;
> + }
>
> - /* If we already have a compute context, there is no point
> - * of creating one in case we are called from ioctl that needs
> - * a compute context
> - */
> - if ((hdev->compute_ctx) && (requires_compute_ctx)) {
> + /* If we reached here, it means we have a non-compute context,
> + * but there is no compute context on the device. Therefore,
> + * we can try to "upgrade" the existing context to a compute
> + * context
> + */
> + dev_dbg_ratelimited(hdev->dev,
> + "Non-compute context %d exists\n",
> + hpriv->ctx->asid);
> +
> + } else if ((hdev->compute_ctx) && (requires_compute_ctx)) {
> +
> + /* If we already have a compute context in the device, there is
> + * no point of creating one in case we are called from ioctl
> + * that needs a compute context
> + */
> dev_err(hdev->dev,
> "Can't create new compute context as one already exists\n");
> valid = false;
> goto unlock_mutex;
> - }
> + } else {
> + /* If we reached here it is because there isn't a context for
> + * the process AND there is no compute context or compute
> + * context wasn't required. In any case, must create a context
> + * for the process
> + */
>
> - rc = hl_ctx_create(hdev, hpriv);
> - if (rc) {
> - dev_err(hdev->dev, "Failed to create context %d\n", rc);
> - valid = false;
> - goto unlock_mutex;
> + rc = hl_ctx_create(hdev, hpriv);
> + if (rc) {
> + dev_err(hdev->dev, "Failed to create context %d\n", rc);
> + valid = false;
> + goto unlock_mutex;
> + }
> +
> + dev_dbg_ratelimited(hdev->dev, "Created context %d\n",
> + hpriv->ctx->asid);
> }
>
> - /* Device is IDLE at this point so it is legal to change PLLs.
> - * There is no need to check anything because if the PLL is
> - * already HIGH, the set function will return without doing
> - * anything
> + /* If we reached here then either we have a new context, or we can
> + * upgrade a non-compute context to a compute context. Do the upgrade
> + * only if the caller required a compute context
> */
> - hl_device_set_frequency(hdev, PLL_HIGH);
> + if (requires_compute_ctx) {
> + WARN(hdev->compute_ctx,
> + "Compute context exists but driver is setting a new one");
This will trigger syzbot and will reboot machines that have
'panic-on-warn' set (i.e. all cloud systems). So be _VERY_ careful
about this.
If a user can trigger this, do not use WARN(), that's not what it is
for.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 9/9] habanalabs: allow multiple processes to open FD
2019-07-28 11:44 ` Greg KH
@ 2019-07-28 11:56 ` Oded Gabbay
2019-07-28 12:04 ` Greg KH
0 siblings, 1 reply; 16+ messages in thread
From: Oded Gabbay @ 2019-07-28 11:56 UTC (permalink / raw)
To: Greg KH; +Cc: Linux-Kernel@Vger. Kernel. Org, Omer Shpigelman, Tomer Tayar
On Sun, Jul 28, 2019 at 2:44 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Sun, Jul 28, 2019 at 02:28:18PM +0300, Oded Gabbay wrote:
> > This patch removes the limitation of a single process that can open the
> > device.
> >
> > Now, there is no limitation on the number of processes that can open the
> > device and have a valid FD.
> >
> > However, only a single process can perform compute operations. This is
> > enforced by allowing only a single process to have a compute context.
> >
> > Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com>
> > ---
> > drivers/misc/habanalabs/context.c | 100 +++++++++++++++------
> > drivers/misc/habanalabs/device.c | 18 ++--
> > drivers/misc/habanalabs/habanalabs.h | 1 -
> > drivers/misc/habanalabs/habanalabs_drv.c | 8 --
> > drivers/misc/habanalabs/habanalabs_ioctl.c | 7 +-
> > 5 files changed, 85 insertions(+), 49 deletions(-)
> >
> > diff --git a/drivers/misc/habanalabs/context.c b/drivers/misc/habanalabs/context.c
> > index 57bbe59da9b6..f64220fc3a55 100644
> > --- a/drivers/misc/habanalabs/context.c
> > +++ b/drivers/misc/habanalabs/context.c
> > @@ -56,7 +56,7 @@ void hl_ctx_do_release(struct kref *ref)
> > kfree(ctx);
> > }
> >
> > -int hl_ctx_create(struct hl_device *hdev, struct hl_fpriv *hpriv)
> > +static int hl_ctx_create(struct hl_device *hdev, struct hl_fpriv *hpriv)
> > {
> > struct hl_ctx_mgr *mgr = &hpriv->ctx_mgr;
> > struct hl_ctx *ctx;
> > @@ -89,9 +89,6 @@ int hl_ctx_create(struct hl_device *hdev, struct hl_fpriv *hpriv)
> > /* TODO: remove for multiple contexts per process */
> > hpriv->ctx = ctx;
> >
> > - /* TODO: remove the following line for multiple process support */
> > - hdev->compute_ctx = ctx;
> > -
> > return 0;
> >
> > remove_from_idr:
> > @@ -206,13 +203,22 @@ bool hl_ctx_is_valid(struct hl_fpriv *hpriv, bool requires_compute_ctx)
> > int rc;
> >
> > /* First thing, to minimize latency impact, check if context exists.
> > - * Also check if it matches the requirements. If so, exit immediately
> > + * This is relevant for the "steady state", where a process context
> > + * already exists, and we want to minimize the latency in command
> > + * submissions. In that case, we want to see if we can quickly exit
> > + * with a valid answer.
> > + *
> > + * If a context doesn't exists, we must grab the mutex. Otherwise,
> > + * there can be nasty races in case of multi-threaded application.
> > + *
> > + * So, if the context exists and we don't need a compute context,
> > + * that's fine. If it exists and the context we have is the compute
> > + * context, that's also fine. Other then that, we can't check anything
> > + * without the mutex.
> > */
> > - if (hpriv->ctx) {
> > - if ((requires_compute_ctx) && (hdev->compute_ctx != hpriv->ctx))
> > - return false;
> > + if ((hpriv->ctx) && ((!requires_compute_ctx) ||
> > + (hdev->compute_ctx == hpriv->ctx)))
> > return true;
> > - }
> >
> > mutex_lock(&hdev->lazy_ctx_creation_lock);
> >
> > @@ -222,35 +228,73 @@ bool hl_ctx_is_valid(struct hl_fpriv *hpriv, bool requires_compute_ctx)
> > * creation of a context
> > */
> > if (hpriv->ctx) {
> > - if ((requires_compute_ctx) && (hdev->compute_ctx != hpriv->ctx))
> > + if ((!requires_compute_ctx) ||
> > + (hdev->compute_ctx == hpriv->ctx))
> > + goto unlock_mutex;
> > +
> > + if (hdev->compute_ctx) {
> > valid = false;
> > - goto unlock_mutex;
> > - }
> > + goto unlock_mutex;
> > + }
> >
> > - /* If we already have a compute context, there is no point
> > - * of creating one in case we are called from ioctl that needs
> > - * a compute context
> > - */
> > - if ((hdev->compute_ctx) && (requires_compute_ctx)) {
> > + /* If we reached here, it means we have a non-compute context,
> > + * but there is no compute context on the device. Therefore,
> > + * we can try to "upgrade" the existing context to a compute
> > + * context
> > + */
> > + dev_dbg_ratelimited(hdev->dev,
> > + "Non-compute context %d exists\n",
> > + hpriv->ctx->asid);
> > +
> > + } else if ((hdev->compute_ctx) && (requires_compute_ctx)) {
> > +
> > + /* If we already have a compute context in the device, there is
> > + * no point of creating one in case we are called from ioctl
> > + * that needs a compute context
> > + */
> > dev_err(hdev->dev,
> > "Can't create new compute context as one already exists\n");
> > valid = false;
> > goto unlock_mutex;
> > - }
> > + } else {
> > + /* If we reached here it is because there isn't a context for
> > + * the process AND there is no compute context or compute
> > + * context wasn't required. In any case, must create a context
> > + * for the process
> > + */
> >
> > - rc = hl_ctx_create(hdev, hpriv);
> > - if (rc) {
> > - dev_err(hdev->dev, "Failed to create context %d\n", rc);
> > - valid = false;
> > - goto unlock_mutex;
> > + rc = hl_ctx_create(hdev, hpriv);
> > + if (rc) {
> > + dev_err(hdev->dev, "Failed to create context %d\n", rc);
> > + valid = false;
> > + goto unlock_mutex;
> > + }
> > +
> > + dev_dbg_ratelimited(hdev->dev, "Created context %d\n",
> > + hpriv->ctx->asid);
> > }
> >
> > - /* Device is IDLE at this point so it is legal to change PLLs.
> > - * There is no need to check anything because if the PLL is
> > - * already HIGH, the set function will return without doing
> > - * anything
> > + /* If we reached here then either we have a new context, or we can
> > + * upgrade a non-compute context to a compute context. Do the upgrade
> > + * only if the caller required a compute context
> > */
> > - hl_device_set_frequency(hdev, PLL_HIGH);
> > + if (requires_compute_ctx) {
> > + WARN(hdev->compute_ctx,
> > + "Compute context exists but driver is setting a new one");
>
> This will trigger syzbot and will reboot machines that have
> 'panic-on-warn' set (i.e. all cloud systems). So be _VERY_ careful
> about this.
>
> If a user can trigger this, do not use WARN(), that's not what it is
> for.
>
> thanks,
>
> greg k-h
I see...
I'll replace it with dev_crit, but I wanted to ask if you recommend to
never use WARN in drivers ? Just use it in kernel core code ?
Thanks,
Oded
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 9/9] habanalabs: allow multiple processes to open FD
2019-07-28 11:56 ` Oded Gabbay
@ 2019-07-28 12:04 ` Greg KH
2019-07-28 12:06 ` Oded Gabbay
0 siblings, 1 reply; 16+ messages in thread
From: Greg KH @ 2019-07-28 12:04 UTC (permalink / raw)
To: Oded Gabbay; +Cc: Linux-Kernel@Vger. Kernel. Org, Omer Shpigelman, Tomer Tayar
On Sun, Jul 28, 2019 at 02:56:40PM +0300, Oded Gabbay wrote:
> On Sun, Jul 28, 2019 at 2:44 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Sun, Jul 28, 2019 at 02:28:18PM +0300, Oded Gabbay wrote:
> > > This patch removes the limitation of a single process that can open the
> > > device.
> > >
> > > Now, there is no limitation on the number of processes that can open the
> > > device and have a valid FD.
> > >
> > > However, only a single process can perform compute operations. This is
> > > enforced by allowing only a single process to have a compute context.
> > >
> > > Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com>
> > > ---
> > > drivers/misc/habanalabs/context.c | 100 +++++++++++++++------
> > > drivers/misc/habanalabs/device.c | 18 ++--
> > > drivers/misc/habanalabs/habanalabs.h | 1 -
> > > drivers/misc/habanalabs/habanalabs_drv.c | 8 --
> > > drivers/misc/habanalabs/habanalabs_ioctl.c | 7 +-
> > > 5 files changed, 85 insertions(+), 49 deletions(-)
> > >
> > > diff --git a/drivers/misc/habanalabs/context.c b/drivers/misc/habanalabs/context.c
> > > index 57bbe59da9b6..f64220fc3a55 100644
> > > --- a/drivers/misc/habanalabs/context.c
> > > +++ b/drivers/misc/habanalabs/context.c
> > > @@ -56,7 +56,7 @@ void hl_ctx_do_release(struct kref *ref)
> > > kfree(ctx);
> > > }
> > >
> > > -int hl_ctx_create(struct hl_device *hdev, struct hl_fpriv *hpriv)
> > > +static int hl_ctx_create(struct hl_device *hdev, struct hl_fpriv *hpriv)
> > > {
> > > struct hl_ctx_mgr *mgr = &hpriv->ctx_mgr;
> > > struct hl_ctx *ctx;
> > > @@ -89,9 +89,6 @@ int hl_ctx_create(struct hl_device *hdev, struct hl_fpriv *hpriv)
> > > /* TODO: remove for multiple contexts per process */
> > > hpriv->ctx = ctx;
> > >
> > > - /* TODO: remove the following line for multiple process support */
> > > - hdev->compute_ctx = ctx;
> > > -
> > > return 0;
> > >
> > > remove_from_idr:
> > > @@ -206,13 +203,22 @@ bool hl_ctx_is_valid(struct hl_fpriv *hpriv, bool requires_compute_ctx)
> > > int rc;
> > >
> > > /* First thing, to minimize latency impact, check if context exists.
> > > - * Also check if it matches the requirements. If so, exit immediately
> > > + * This is relevant for the "steady state", where a process context
> > > + * already exists, and we want to minimize the latency in command
> > > + * submissions. In that case, we want to see if we can quickly exit
> > > + * with a valid answer.
> > > + *
> > > + * If a context doesn't exists, we must grab the mutex. Otherwise,
> > > + * there can be nasty races in case of multi-threaded application.
> > > + *
> > > + * So, if the context exists and we don't need a compute context,
> > > + * that's fine. If it exists and the context we have is the compute
> > > + * context, that's also fine. Other then that, we can't check anything
> > > + * without the mutex.
> > > */
> > > - if (hpriv->ctx) {
> > > - if ((requires_compute_ctx) && (hdev->compute_ctx != hpriv->ctx))
> > > - return false;
> > > + if ((hpriv->ctx) && ((!requires_compute_ctx) ||
> > > + (hdev->compute_ctx == hpriv->ctx)))
> > > return true;
> > > - }
> > >
> > > mutex_lock(&hdev->lazy_ctx_creation_lock);
> > >
> > > @@ -222,35 +228,73 @@ bool hl_ctx_is_valid(struct hl_fpriv *hpriv, bool requires_compute_ctx)
> > > * creation of a context
> > > */
> > > if (hpriv->ctx) {
> > > - if ((requires_compute_ctx) && (hdev->compute_ctx != hpriv->ctx))
> > > + if ((!requires_compute_ctx) ||
> > > + (hdev->compute_ctx == hpriv->ctx))
> > > + goto unlock_mutex;
> > > +
> > > + if (hdev->compute_ctx) {
> > > valid = false;
> > > - goto unlock_mutex;
> > > - }
> > > + goto unlock_mutex;
> > > + }
> > >
> > > - /* If we already have a compute context, there is no point
> > > - * of creating one in case we are called from ioctl that needs
> > > - * a compute context
> > > - */
> > > - if ((hdev->compute_ctx) && (requires_compute_ctx)) {
> > > + /* If we reached here, it means we have a non-compute context,
> > > + * but there is no compute context on the device. Therefore,
> > > + * we can try to "upgrade" the existing context to a compute
> > > + * context
> > > + */
> > > + dev_dbg_ratelimited(hdev->dev,
> > > + "Non-compute context %d exists\n",
> > > + hpriv->ctx->asid);
> > > +
> > > + } else if ((hdev->compute_ctx) && (requires_compute_ctx)) {
> > > +
> > > + /* If we already have a compute context in the device, there is
> > > + * no point of creating one in case we are called from ioctl
> > > + * that needs a compute context
> > > + */
> > > dev_err(hdev->dev,
> > > "Can't create new compute context as one already exists\n");
> > > valid = false;
> > > goto unlock_mutex;
> > > - }
> > > + } else {
> > > + /* If we reached here it is because there isn't a context for
> > > + * the process AND there is no compute context or compute
> > > + * context wasn't required. In any case, must create a context
> > > + * for the process
> > > + */
> > >
> > > - rc = hl_ctx_create(hdev, hpriv);
> > > - if (rc) {
> > > - dev_err(hdev->dev, "Failed to create context %d\n", rc);
> > > - valid = false;
> > > - goto unlock_mutex;
> > > + rc = hl_ctx_create(hdev, hpriv);
> > > + if (rc) {
> > > + dev_err(hdev->dev, "Failed to create context %d\n", rc);
> > > + valid = false;
> > > + goto unlock_mutex;
> > > + }
> > > +
> > > + dev_dbg_ratelimited(hdev->dev, "Created context %d\n",
> > > + hpriv->ctx->asid);
> > > }
> > >
> > > - /* Device is IDLE at this point so it is legal to change PLLs.
> > > - * There is no need to check anything because if the PLL is
> > > - * already HIGH, the set function will return without doing
> > > - * anything
> > > + /* If we reached here then either we have a new context, or we can
> > > + * upgrade a non-compute context to a compute context. Do the upgrade
> > > + * only if the caller required a compute context
> > > */
> > > - hl_device_set_frequency(hdev, PLL_HIGH);
> > > + if (requires_compute_ctx) {
> > > + WARN(hdev->compute_ctx,
> > > + "Compute context exists but driver is setting a new one");
> >
> > This will trigger syzbot and will reboot machines that have
> > 'panic-on-warn' set (i.e. all cloud systems). So be _VERY_ careful
> > about this.
> >
> > If a user can trigger this, do not use WARN(), that's not what it is
> > for.
> >
> > thanks,
> >
> > greg k-h
>
> I see...
> I'll replace it with dev_crit, but I wanted to ask if you recommend to
> never use WARN in drivers ? Just use it in kernel core code ?
It should never be used anywhere, unless you are about to crash. You
should just properly fix things up, log the error, and move on. Same
goes for a driver as well as "core" kernel code.
If a user can trigger a WARN message, then that's a real big problem.
Again, think of 'panic-on-warn' systems.
If the hardware has hosed the system so bad that you can not do anything
else, just stop allowing access to the hardware. You shouldn't cause
the system to crash/reboot whenever possible.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 9/9] habanalabs: allow multiple processes to open FD
2019-07-28 12:04 ` Greg KH
@ 2019-07-28 12:06 ` Oded Gabbay
2019-07-28 12:12 ` Greg KH
0 siblings, 1 reply; 16+ messages in thread
From: Oded Gabbay @ 2019-07-28 12:06 UTC (permalink / raw)
To: Greg KH; +Cc: Linux-Kernel@Vger. Kernel. Org, Omer Shpigelman, Tomer Tayar
On Sun, Jul 28, 2019 at 3:04 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Sun, Jul 28, 2019 at 02:56:40PM +0300, Oded Gabbay wrote:
> > On Sun, Jul 28, 2019 at 2:44 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Sun, Jul 28, 2019 at 02:28:18PM +0300, Oded Gabbay wrote:
> > > > This patch removes the limitation of a single process that can open the
> > > > device.
> > > >
> > > > Now, there is no limitation on the number of processes that can open the
> > > > device and have a valid FD.
> > > >
> > > > However, only a single process can perform compute operations. This is
> > > > enforced by allowing only a single process to have a compute context.
> > > >
> > > > Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com>
> > > > ---
> > > > drivers/misc/habanalabs/context.c | 100 +++++++++++++++------
> > > > drivers/misc/habanalabs/device.c | 18 ++--
> > > > drivers/misc/habanalabs/habanalabs.h | 1 -
> > > > drivers/misc/habanalabs/habanalabs_drv.c | 8 --
> > > > drivers/misc/habanalabs/habanalabs_ioctl.c | 7 +-
> > > > 5 files changed, 85 insertions(+), 49 deletions(-)
> > > >
> > > > diff --git a/drivers/misc/habanalabs/context.c b/drivers/misc/habanalabs/context.c
> > > > index 57bbe59da9b6..f64220fc3a55 100644
> > > > --- a/drivers/misc/habanalabs/context.c
> > > > +++ b/drivers/misc/habanalabs/context.c
> > > > @@ -56,7 +56,7 @@ void hl_ctx_do_release(struct kref *ref)
> > > > kfree(ctx);
> > > > }
> > > >
> > > > -int hl_ctx_create(struct hl_device *hdev, struct hl_fpriv *hpriv)
> > > > +static int hl_ctx_create(struct hl_device *hdev, struct hl_fpriv *hpriv)
> > > > {
> > > > struct hl_ctx_mgr *mgr = &hpriv->ctx_mgr;
> > > > struct hl_ctx *ctx;
> > > > @@ -89,9 +89,6 @@ int hl_ctx_create(struct hl_device *hdev, struct hl_fpriv *hpriv)
> > > > /* TODO: remove for multiple contexts per process */
> > > > hpriv->ctx = ctx;
> > > >
> > > > - /* TODO: remove the following line for multiple process support */
> > > > - hdev->compute_ctx = ctx;
> > > > -
> > > > return 0;
> > > >
> > > > remove_from_idr:
> > > > @@ -206,13 +203,22 @@ bool hl_ctx_is_valid(struct hl_fpriv *hpriv, bool requires_compute_ctx)
> > > > int rc;
> > > >
> > > > /* First thing, to minimize latency impact, check if context exists.
> > > > - * Also check if it matches the requirements. If so, exit immediately
> > > > + * This is relevant for the "steady state", where a process context
> > > > + * already exists, and we want to minimize the latency in command
> > > > + * submissions. In that case, we want to see if we can quickly exit
> > > > + * with a valid answer.
> > > > + *
> > > > + * If a context doesn't exists, we must grab the mutex. Otherwise,
> > > > + * there can be nasty races in case of multi-threaded application.
> > > > + *
> > > > + * So, if the context exists and we don't need a compute context,
> > > > + * that's fine. If it exists and the context we have is the compute
> > > > + * context, that's also fine. Other then that, we can't check anything
> > > > + * without the mutex.
> > > > */
> > > > - if (hpriv->ctx) {
> > > > - if ((requires_compute_ctx) && (hdev->compute_ctx != hpriv->ctx))
> > > > - return false;
> > > > + if ((hpriv->ctx) && ((!requires_compute_ctx) ||
> > > > + (hdev->compute_ctx == hpriv->ctx)))
> > > > return true;
> > > > - }
> > > >
> > > > mutex_lock(&hdev->lazy_ctx_creation_lock);
> > > >
> > > > @@ -222,35 +228,73 @@ bool hl_ctx_is_valid(struct hl_fpriv *hpriv, bool requires_compute_ctx)
> > > > * creation of a context
> > > > */
> > > > if (hpriv->ctx) {
> > > > - if ((requires_compute_ctx) && (hdev->compute_ctx != hpriv->ctx))
> > > > + if ((!requires_compute_ctx) ||
> > > > + (hdev->compute_ctx == hpriv->ctx))
> > > > + goto unlock_mutex;
> > > > +
> > > > + if (hdev->compute_ctx) {
> > > > valid = false;
> > > > - goto unlock_mutex;
> > > > - }
> > > > + goto unlock_mutex;
> > > > + }
> > > >
> > > > - /* If we already have a compute context, there is no point
> > > > - * of creating one in case we are called from ioctl that needs
> > > > - * a compute context
> > > > - */
> > > > - if ((hdev->compute_ctx) && (requires_compute_ctx)) {
> > > > + /* If we reached here, it means we have a non-compute context,
> > > > + * but there is no compute context on the device. Therefore,
> > > > + * we can try to "upgrade" the existing context to a compute
> > > > + * context
> > > > + */
> > > > + dev_dbg_ratelimited(hdev->dev,
> > > > + "Non-compute context %d exists\n",
> > > > + hpriv->ctx->asid);
> > > > +
> > > > + } else if ((hdev->compute_ctx) && (requires_compute_ctx)) {
> > > > +
> > > > + /* If we already have a compute context in the device, there is
> > > > + * no point of creating one in case we are called from ioctl
> > > > + * that needs a compute context
> > > > + */
> > > > dev_err(hdev->dev,
> > > > "Can't create new compute context as one already exists\n");
> > > > valid = false;
> > > > goto unlock_mutex;
> > > > - }
> > > > + } else {
> > > > + /* If we reached here it is because there isn't a context for
> > > > + * the process AND there is no compute context or compute
> > > > + * context wasn't required. In any case, must create a context
> > > > + * for the process
> > > > + */
> > > >
> > > > - rc = hl_ctx_create(hdev, hpriv);
> > > > - if (rc) {
> > > > - dev_err(hdev->dev, "Failed to create context %d\n", rc);
> > > > - valid = false;
> > > > - goto unlock_mutex;
> > > > + rc = hl_ctx_create(hdev, hpriv);
> > > > + if (rc) {
> > > > + dev_err(hdev->dev, "Failed to create context %d\n", rc);
> > > > + valid = false;
> > > > + goto unlock_mutex;
> > > > + }
> > > > +
> > > > + dev_dbg_ratelimited(hdev->dev, "Created context %d\n",
> > > > + hpriv->ctx->asid);
> > > > }
> > > >
> > > > - /* Device is IDLE at this point so it is legal to change PLLs.
> > > > - * There is no need to check anything because if the PLL is
> > > > - * already HIGH, the set function will return without doing
> > > > - * anything
> > > > + /* If we reached here then either we have a new context, or we can
> > > > + * upgrade a non-compute context to a compute context. Do the upgrade
> > > > + * only if the caller required a compute context
> > > > */
> > > > - hl_device_set_frequency(hdev, PLL_HIGH);
> > > > + if (requires_compute_ctx) {
> > > > + WARN(hdev->compute_ctx,
> > > > + "Compute context exists but driver is setting a new one");
> > >
> > > This will trigger syzbot and will reboot machines that have
> > > 'panic-on-warn' set (i.e. all cloud systems). So be _VERY_ careful
> > > about this.
> > >
> > > If a user can trigger this, do not use WARN(), that's not what it is
> > > for.
> > >
> > > thanks,
> > >
> > > greg k-h
> >
> > I see...
> > I'll replace it with dev_crit, but I wanted to ask if you recommend to
> > never use WARN in drivers ? Just use it in kernel core code ?
>
> It should never be used anywhere, unless you are about to crash. You
> should just properly fix things up, log the error, and move on. Same
> goes for a driver as well as "core" kernel code.
>
> If a user can trigger a WARN message, then that's a real big problem.
> Again, think of 'panic-on-warn' systems.
>
> If the hardware has hosed the system so bad that you can not do anything
> else, just stop allowing access to the hardware. You shouldn't cause
> the system to crash/reboot whenever possible.
>
> thanks,
>
> greg k-h
I understand. I always thought the above applies mostly to BUG() and
that's why it is frowned upon, and instead we should use WARN().
But I get your point about the "panic-on-warn" systems.
I'll avoid that in the future.
Thanks for the review.
Oded
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 9/9] habanalabs: allow multiple processes to open FD
2019-07-28 12:06 ` Oded Gabbay
@ 2019-07-28 12:12 ` Greg KH
2019-07-28 12:18 ` Oded Gabbay
0 siblings, 1 reply; 16+ messages in thread
From: Greg KH @ 2019-07-28 12:12 UTC (permalink / raw)
To: Oded Gabbay; +Cc: Linux-Kernel@Vger. Kernel. Org, Omer Shpigelman, Tomer Tayar
On Sun, Jul 28, 2019 at 03:06:16PM +0300, Oded Gabbay wrote:
> On Sun, Jul 28, 2019 at 3:04 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Sun, Jul 28, 2019 at 02:56:40PM +0300, Oded Gabbay wrote:
> > > On Sun, Jul 28, 2019 at 2:44 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Sun, Jul 28, 2019 at 02:28:18PM +0300, Oded Gabbay wrote:
> > > > > This patch removes the limitation of a single process that can open the
> > > > > device.
> > > > >
> > > > > Now, there is no limitation on the number of processes that can open the
> > > > > device and have a valid FD.
> > > > >
> > > > > However, only a single process can perform compute operations. This is
> > > > > enforced by allowing only a single process to have a compute context.
> > > > >
> > > > > Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com>
> > > > > ---
> > > > > drivers/misc/habanalabs/context.c | 100 +++++++++++++++------
> > > > > drivers/misc/habanalabs/device.c | 18 ++--
> > > > > drivers/misc/habanalabs/habanalabs.h | 1 -
> > > > > drivers/misc/habanalabs/habanalabs_drv.c | 8 --
> > > > > drivers/misc/habanalabs/habanalabs_ioctl.c | 7 +-
> > > > > 5 files changed, 85 insertions(+), 49 deletions(-)
> > > > >
> > > > > diff --git a/drivers/misc/habanalabs/context.c b/drivers/misc/habanalabs/context.c
> > > > > index 57bbe59da9b6..f64220fc3a55 100644
> > > > > --- a/drivers/misc/habanalabs/context.c
> > > > > +++ b/drivers/misc/habanalabs/context.c
> > > > > @@ -56,7 +56,7 @@ void hl_ctx_do_release(struct kref *ref)
> > > > > kfree(ctx);
> > > > > }
> > > > >
> > > > > -int hl_ctx_create(struct hl_device *hdev, struct hl_fpriv *hpriv)
> > > > > +static int hl_ctx_create(struct hl_device *hdev, struct hl_fpriv *hpriv)
> > > > > {
> > > > > struct hl_ctx_mgr *mgr = &hpriv->ctx_mgr;
> > > > > struct hl_ctx *ctx;
> > > > > @@ -89,9 +89,6 @@ int hl_ctx_create(struct hl_device *hdev, struct hl_fpriv *hpriv)
> > > > > /* TODO: remove for multiple contexts per process */
> > > > > hpriv->ctx = ctx;
> > > > >
> > > > > - /* TODO: remove the following line for multiple process support */
> > > > > - hdev->compute_ctx = ctx;
> > > > > -
> > > > > return 0;
> > > > >
> > > > > remove_from_idr:
> > > > > @@ -206,13 +203,22 @@ bool hl_ctx_is_valid(struct hl_fpriv *hpriv, bool requires_compute_ctx)
> > > > > int rc;
> > > > >
> > > > > /* First thing, to minimize latency impact, check if context exists.
> > > > > - * Also check if it matches the requirements. If so, exit immediately
> > > > > + * This is relevant for the "steady state", where a process context
> > > > > + * already exists, and we want to minimize the latency in command
> > > > > + * submissions. In that case, we want to see if we can quickly exit
> > > > > + * with a valid answer.
> > > > > + *
> > > > > + * If a context doesn't exists, we must grab the mutex. Otherwise,
> > > > > + * there can be nasty races in case of multi-threaded application.
> > > > > + *
> > > > > + * So, if the context exists and we don't need a compute context,
> > > > > + * that's fine. If it exists and the context we have is the compute
> > > > > + * context, that's also fine. Other then that, we can't check anything
> > > > > + * without the mutex.
> > > > > */
> > > > > - if (hpriv->ctx) {
> > > > > - if ((requires_compute_ctx) && (hdev->compute_ctx != hpriv->ctx))
> > > > > - return false;
> > > > > + if ((hpriv->ctx) && ((!requires_compute_ctx) ||
> > > > > + (hdev->compute_ctx == hpriv->ctx)))
> > > > > return true;
> > > > > - }
> > > > >
> > > > > mutex_lock(&hdev->lazy_ctx_creation_lock);
> > > > >
> > > > > @@ -222,35 +228,73 @@ bool hl_ctx_is_valid(struct hl_fpriv *hpriv, bool requires_compute_ctx)
> > > > > * creation of a context
> > > > > */
> > > > > if (hpriv->ctx) {
> > > > > - if ((requires_compute_ctx) && (hdev->compute_ctx != hpriv->ctx))
> > > > > + if ((!requires_compute_ctx) ||
> > > > > + (hdev->compute_ctx == hpriv->ctx))
> > > > > + goto unlock_mutex;
> > > > > +
> > > > > + if (hdev->compute_ctx) {
> > > > > valid = false;
> > > > > - goto unlock_mutex;
> > > > > - }
> > > > > + goto unlock_mutex;
> > > > > + }
> > > > >
> > > > > - /* If we already have a compute context, there is no point
> > > > > - * of creating one in case we are called from ioctl that needs
> > > > > - * a compute context
> > > > > - */
> > > > > - if ((hdev->compute_ctx) && (requires_compute_ctx)) {
> > > > > + /* If we reached here, it means we have a non-compute context,
> > > > > + * but there is no compute context on the device. Therefore,
> > > > > + * we can try to "upgrade" the existing context to a compute
> > > > > + * context
> > > > > + */
> > > > > + dev_dbg_ratelimited(hdev->dev,
> > > > > + "Non-compute context %d exists\n",
> > > > > + hpriv->ctx->asid);
> > > > > +
> > > > > + } else if ((hdev->compute_ctx) && (requires_compute_ctx)) {
> > > > > +
> > > > > + /* If we already have a compute context in the device, there is
> > > > > + * no point of creating one in case we are called from ioctl
> > > > > + * that needs a compute context
> > > > > + */
> > > > > dev_err(hdev->dev,
> > > > > "Can't create new compute context as one already exists\n");
> > > > > valid = false;
> > > > > goto unlock_mutex;
> > > > > - }
> > > > > + } else {
> > > > > + /* If we reached here it is because there isn't a context for
> > > > > + * the process AND there is no compute context or compute
> > > > > + * context wasn't required. In any case, must create a context
> > > > > + * for the process
> > > > > + */
> > > > >
> > > > > - rc = hl_ctx_create(hdev, hpriv);
> > > > > - if (rc) {
> > > > > - dev_err(hdev->dev, "Failed to create context %d\n", rc);
> > > > > - valid = false;
> > > > > - goto unlock_mutex;
> > > > > + rc = hl_ctx_create(hdev, hpriv);
> > > > > + if (rc) {
> > > > > + dev_err(hdev->dev, "Failed to create context %d\n", rc);
> > > > > + valid = false;
> > > > > + goto unlock_mutex;
> > > > > + }
> > > > > +
> > > > > + dev_dbg_ratelimited(hdev->dev, "Created context %d\n",
> > > > > + hpriv->ctx->asid);
> > > > > }
> > > > >
> > > > > - /* Device is IDLE at this point so it is legal to change PLLs.
> > > > > - * There is no need to check anything because if the PLL is
> > > > > - * already HIGH, the set function will return without doing
> > > > > - * anything
> > > > > + /* If we reached here then either we have a new context, or we can
> > > > > + * upgrade a non-compute context to a compute context. Do the upgrade
> > > > > + * only if the caller required a compute context
> > > > > */
> > > > > - hl_device_set_frequency(hdev, PLL_HIGH);
> > > > > + if (requires_compute_ctx) {
> > > > > + WARN(hdev->compute_ctx,
> > > > > + "Compute context exists but driver is setting a new one");
> > > >
> > > > This will trigger syzbot and will reboot machines that have
> > > > 'panic-on-warn' set (i.e. all cloud systems). So be _VERY_ careful
> > > > about this.
> > > >
> > > > If a user can trigger this, do not use WARN(), that's not what it is
> > > > for.
> > > >
> > > > thanks,
> > > >
> > > > greg k-h
> > >
> > > I see...
> > > I'll replace it with dev_crit, but I wanted to ask if you recommend to
> > > never use WARN in drivers ? Just use it in kernel core code ?
> >
> > It should never be used anywhere, unless you are about to crash. You
> > should just properly fix things up, log the error, and move on. Same
> > goes for a driver as well as "core" kernel code.
> >
> > If a user can trigger a WARN message, then that's a real big problem.
> > Again, think of 'panic-on-warn' systems.
> >
> > If the hardware has hosed the system so bad that you can not do anything
> > else, just stop allowing access to the hardware. You shouldn't cause
> > the system to crash/reboot whenever possible.
> >
> > thanks,
> >
> > greg k-h
>
> I understand. I always thought the above applies mostly to BUG() and
> that's why it is frowned upon, and instead we should use WARN().
> But I get your point about the "panic-on-warn" systems.
If you just want to warn the user about something that they can do
something about (and not just spam the kernel log), use dev_warn(),
that's what it is there for :)
thanks,
greg k-h
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 9/9] habanalabs: allow multiple processes to open FD
2019-07-28 12:12 ` Greg KH
@ 2019-07-28 12:18 ` Oded Gabbay
0 siblings, 0 replies; 16+ messages in thread
From: Oded Gabbay @ 2019-07-28 12:18 UTC (permalink / raw)
To: Greg KH; +Cc: Linux-Kernel@Vger. Kernel. Org, Omer Shpigelman, Tomer Tayar
On Sun, Jul 28, 2019 at 3:12 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Sun, Jul 28, 2019 at 03:06:16PM +0300, Oded Gabbay wrote:
> > On Sun, Jul 28, 2019 at 3:04 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Sun, Jul 28, 2019 at 02:56:40PM +0300, Oded Gabbay wrote:
> > > > On Sun, Jul 28, 2019 at 2:44 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Sun, Jul 28, 2019 at 02:28:18PM +0300, Oded Gabbay wrote:
> > > > > > This patch removes the limitation of a single process that can open the
> > > > > > device.
> > > > > >
> > > > > > Now, there is no limitation on the number of processes that can open the
> > > > > > device and have a valid FD.
> > > > > >
> > > > > > However, only a single process can perform compute operations. This is
> > > > > > enforced by allowing only a single process to have a compute context.
> > > > > >
> > > > > > Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com>
> > > > > > ---
> > > > > > drivers/misc/habanalabs/context.c | 100 +++++++++++++++------
> > > > > > drivers/misc/habanalabs/device.c | 18 ++--
> > > > > > drivers/misc/habanalabs/habanalabs.h | 1 -
> > > > > > drivers/misc/habanalabs/habanalabs_drv.c | 8 --
> > > > > > drivers/misc/habanalabs/habanalabs_ioctl.c | 7 +-
> > > > > > 5 files changed, 85 insertions(+), 49 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/misc/habanalabs/context.c b/drivers/misc/habanalabs/context.c
> > > > > > index 57bbe59da9b6..f64220fc3a55 100644
> > > > > > --- a/drivers/misc/habanalabs/context.c
> > > > > > +++ b/drivers/misc/habanalabs/context.c
> > > > > > @@ -56,7 +56,7 @@ void hl_ctx_do_release(struct kref *ref)
> > > > > > kfree(ctx);
> > > > > > }
> > > > > >
> > > > > > -int hl_ctx_create(struct hl_device *hdev, struct hl_fpriv *hpriv)
> > > > > > +static int hl_ctx_create(struct hl_device *hdev, struct hl_fpriv *hpriv)
> > > > > > {
> > > > > > struct hl_ctx_mgr *mgr = &hpriv->ctx_mgr;
> > > > > > struct hl_ctx *ctx;
> > > > > > @@ -89,9 +89,6 @@ int hl_ctx_create(struct hl_device *hdev, struct hl_fpriv *hpriv)
> > > > > > /* TODO: remove for multiple contexts per process */
> > > > > > hpriv->ctx = ctx;
> > > > > >
> > > > > > - /* TODO: remove the following line for multiple process support */
> > > > > > - hdev->compute_ctx = ctx;
> > > > > > -
> > > > > > return 0;
> > > > > >
> > > > > > remove_from_idr:
> > > > > > @@ -206,13 +203,22 @@ bool hl_ctx_is_valid(struct hl_fpriv *hpriv, bool requires_compute_ctx)
> > > > > > int rc;
> > > > > >
> > > > > > /* First thing, to minimize latency impact, check if context exists.
> > > > > > - * Also check if it matches the requirements. If so, exit immediately
> > > > > > + * This is relevant for the "steady state", where a process context
> > > > > > + * already exists, and we want to minimize the latency in command
> > > > > > + * submissions. In that case, we want to see if we can quickly exit
> > > > > > + * with a valid answer.
> > > > > > + *
> > > > > > + * If a context doesn't exists, we must grab the mutex. Otherwise,
> > > > > > + * there can be nasty races in case of multi-threaded application.
> > > > > > + *
> > > > > > + * So, if the context exists and we don't need a compute context,
> > > > > > + * that's fine. If it exists and the context we have is the compute
> > > > > > + * context, that's also fine. Other then that, we can't check anything
> > > > > > + * without the mutex.
> > > > > > */
> > > > > > - if (hpriv->ctx) {
> > > > > > - if ((requires_compute_ctx) && (hdev->compute_ctx != hpriv->ctx))
> > > > > > - return false;
> > > > > > + if ((hpriv->ctx) && ((!requires_compute_ctx) ||
> > > > > > + (hdev->compute_ctx == hpriv->ctx)))
> > > > > > return true;
> > > > > > - }
> > > > > >
> > > > > > mutex_lock(&hdev->lazy_ctx_creation_lock);
> > > > > >
> > > > > > @@ -222,35 +228,73 @@ bool hl_ctx_is_valid(struct hl_fpriv *hpriv, bool requires_compute_ctx)
> > > > > > * creation of a context
> > > > > > */
> > > > > > if (hpriv->ctx) {
> > > > > > - if ((requires_compute_ctx) && (hdev->compute_ctx != hpriv->ctx))
> > > > > > + if ((!requires_compute_ctx) ||
> > > > > > + (hdev->compute_ctx == hpriv->ctx))
> > > > > > + goto unlock_mutex;
> > > > > > +
> > > > > > + if (hdev->compute_ctx) {
> > > > > > valid = false;
> > > > > > - goto unlock_mutex;
> > > > > > - }
> > > > > > + goto unlock_mutex;
> > > > > > + }
> > > > > >
> > > > > > - /* If we already have a compute context, there is no point
> > > > > > - * of creating one in case we are called from ioctl that needs
> > > > > > - * a compute context
> > > > > > - */
> > > > > > - if ((hdev->compute_ctx) && (requires_compute_ctx)) {
> > > > > > + /* If we reached here, it means we have a non-compute context,
> > > > > > + * but there is no compute context on the device. Therefore,
> > > > > > + * we can try to "upgrade" the existing context to a compute
> > > > > > + * context
> > > > > > + */
> > > > > > + dev_dbg_ratelimited(hdev->dev,
> > > > > > + "Non-compute context %d exists\n",
> > > > > > + hpriv->ctx->asid);
> > > > > > +
> > > > > > + } else if ((hdev->compute_ctx) && (requires_compute_ctx)) {
> > > > > > +
> > > > > > + /* If we already have a compute context in the device, there is
> > > > > > + * no point of creating one in case we are called from ioctl
> > > > > > + * that needs a compute context
> > > > > > + */
> > > > > > dev_err(hdev->dev,
> > > > > > "Can't create new compute context as one already exists\n");
> > > > > > valid = false;
> > > > > > goto unlock_mutex;
> > > > > > - }
> > > > > > + } else {
> > > > > > + /* If we reached here it is because there isn't a context for
> > > > > > + * the process AND there is no compute context or compute
> > > > > > + * context wasn't required. In any case, must create a context
> > > > > > + * for the process
> > > > > > + */
> > > > > >
> > > > > > - rc = hl_ctx_create(hdev, hpriv);
> > > > > > - if (rc) {
> > > > > > - dev_err(hdev->dev, "Failed to create context %d\n", rc);
> > > > > > - valid = false;
> > > > > > - goto unlock_mutex;
> > > > > > + rc = hl_ctx_create(hdev, hpriv);
> > > > > > + if (rc) {
> > > > > > + dev_err(hdev->dev, "Failed to create context %d\n", rc);
> > > > > > + valid = false;
> > > > > > + goto unlock_mutex;
> > > > > > + }
> > > > > > +
> > > > > > + dev_dbg_ratelimited(hdev->dev, "Created context %d\n",
> > > > > > + hpriv->ctx->asid);
> > > > > > }
> > > > > >
> > > > > > - /* Device is IDLE at this point so it is legal to change PLLs.
> > > > > > - * There is no need to check anything because if the PLL is
> > > > > > - * already HIGH, the set function will return without doing
> > > > > > - * anything
> > > > > > + /* If we reached here then either we have a new context, or we can
> > > > > > + * upgrade a non-compute context to a compute context. Do the upgrade
> > > > > > + * only if the caller required a compute context
> > > > > > */
> > > > > > - hl_device_set_frequency(hdev, PLL_HIGH);
> > > > > > + if (requires_compute_ctx) {
> > > > > > + WARN(hdev->compute_ctx,
> > > > > > + "Compute context exists but driver is setting a new one");
> > > > >
> > > > > This will trigger syzbot and will reboot machines that have
> > > > > 'panic-on-warn' set (i.e. all cloud systems). So be _VERY_ careful
> > > > > about this.
> > > > >
> > > > > If a user can trigger this, do not use WARN(), that's not what it is
> > > > > for.
> > > > >
> > > > > thanks,
> > > > >
> > > > > greg k-h
> > > >
> > > > I see...
> > > > I'll replace it with dev_crit, but I wanted to ask if you recommend to
> > > > never use WARN in drivers ? Just use it in kernel core code ?
> > >
> > > It should never be used anywhere, unless you are about to crash. You
> > > should just properly fix things up, log the error, and move on. Same
> > > goes for a driver as well as "core" kernel code.
> > >
> > > If a user can trigger a WARN message, then that's a real big problem.
> > > Again, think of 'panic-on-warn' systems.
> > >
> > > If the hardware has hosed the system so bad that you can not do anything
> > > else, just stop allowing access to the hardware. You shouldn't cause
> > > the system to crash/reboot whenever possible.
> > >
> > > thanks,
> > >
> > > greg k-h
> >
> > I understand. I always thought the above applies mostly to BUG() and
> > that's why it is frowned upon, and instead we should use WARN().
> > But I get your point about the "panic-on-warn" systems.
>
> If you just want to warn the user about something that they can do
> something about (and not just spam the kernel log), use dev_warn(),
> that's what it is there for :)
>
> thanks,
>
> greg k-h
This specific message is about something the user can do. This
indicates a bug in the driver's code. Think of it as a sanity check.
It can't be affected by the user request/action.
Oded
^ permalink raw reply [flat|nested] 16+ messages in thread