Quoting Chris Wilson (2018-11-02 18:12:14)
> Currently, we convert the error state into a string every time we read
> from sysfs (and sysfs reads in page size (4KiB) chunks). We do try to
> window the string and only capture the portion that is being read, but
> that means that we must always convert up to the window to find the
> start. For a very large error state bordering on EXEC_OBJECT_CAPTURE
> abuse, this is noticeable as it degrades to O(N^2)!
>
> As we do not have a convenient hook for sysfs open(), and we would like
> to keep the lazy conversion into a string, do the conversion of the
> whole string on the first read and keep the string until the error state
> is freed.
>
> v2: Don't double advance simple_read_from_buffer
> v3: Due to extreme pain of lack of vrealloc, use a scatterlist
> v4: Keep the forward iterator loosely cached
>
> Reported-by: Jason Ekstrand <jason@xxxxxxxxxxxxxx>
> Testcase: igt/gem_exec_capture/many*
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>
> Cc: Jason Ekstrand <jason@xxxxxxxxxxxxxx>
<SNIP>
> @@ -943,30 +943,28 @@ static int i915_gem_fence_regs_info(struct seq_file *m, void *data)
> static ssize_t gpu_state_read(struct file *file, char __user *ubuf,
> size_t count, loff_t *pos)
> {
> - struct i915_gpu_state *error = file->private_data;
> - struct drm_i915_error_state_buf str;
> + struct i915_gpu_state *error;
> ssize_t ret;
> - loff_t tmp;
> + void *buf;
>
> + error = file->private_data;
> if (!error)
> return 0;
>
> - ret = i915_error_state_buf_init(&str, error->i915, count, *pos);
> - if (ret)
> - return ret;
> -
> - ret = i915_error_state_to_str(&str, error);
> - if (ret)
> - goto out;
> + /* Bounce buffer required because of kernfs __user API convenience. */
> + buf = kmalloc(count, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
>
> - tmp = 0;
> - ret = simple_read_from_buffer(ubuf, count, &tmp, str.buf, str.bytes);
> - if (ret < 0)
> - goto out;
> + ret = i915_gpu_state_copy_to_buffer(error, buf, *pos, count);
> + if (ret > 0) {
if (ret <= 0)
goto out;
One can never onion too early.
> + if (!copy_to_user(ubuf, buf, ret))
> + *pos += ret;
> + else
> + ret = -EFAULT;
> + }
>
> - *pos = str.start + ret;
> -out:
> - i915_error_state_buf_release(&str);
> + kfree(buf);
> return ret;
> }
<SNIP>
> -static bool __i915_error_ok(struct drm_i915_error_state_buf *e)
> +static void __sg_set_buf(struct scatterlist *sg,
> + void *addr, unsigned int len, loff_t it)
> {
> -
> - if (!e->err && WARN(e->bytes > (e->size - 1), "overflow")) {
> - e->err = -ENOSPC;
> - return false;
> - }
> -
> - if (e->bytes == e->size - 1 || e->err)
> - return false;
> -
> - return true;
> + sg->page_link = (unsigned long)virt_to_page(addr);
> + sg->offset = offset_in_page(addr);
> + sg->length = len;
> + sg->dma_address = it;
> }
The least we can do is to extract the sg functions as a follow-up patch
and try to haggle them to scatterlist.h.
>
> -static bool __i915_error_seek(struct drm_i915_error_state_buf *e,
> - unsigned len)
> +static bool __i915_error_grow(struct drm_i915_error_state_buf *e, size_t len)
> {
> - if (e->pos + len <= e->start) {
> - e->pos += len;
> + if (!len)
> return false;
> - }
>
> - /* First vsnprintf needs to fit in its entirety for memmove */
> - if (len >= e->size) {
> - e->err = -EIO;
> - return false;
> - }
> + if (e->bytes + len + 1 > e->size) {
if (e->bytes + len + 1 <= e->size)
return true;
Indent reduced somewhat.
> + if (e->bytes) {
> + __sg_set_buf(e->cur++, e->buf, e->bytes, e->iter);
> + e->iter += e->bytes;
> + e->buf = NULL;
> + e->bytes = 0;
> + }
>
> - return true;
> -}
> + if (e->cur == e->end) {
> + struct scatterlist *sgl;
>
<SNIP>
> @@ -268,6 +268,8 @@ static int compress_page(struct compress *c,
>
> if (zlib_deflate(zstream, Z_NO_FLUSH) != Z_OK)
> return -EIO;
> +
> + touch_nmi_watchdog();
Lost hunk looking for a good home.
> } while (zstream->avail_in);
>
> /* Fallback to uncompressed if we increase size? */
> @@ -635,36 +637,53 @@ static void err_print_uc(struct drm_i915_error_state_buf *m,
> print_error_obj(m, NULL, "GuC log buffer", error_uc->guc_log);
> }
>
> -int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
> - const struct i915_gpu_state *error)
> +static void err_free_sgl(struct scatterlist *sgl)
> {
> - struct drm_i915_private *dev_priv = m->i915;
> + while (sgl) {
> + struct scatterlist *sg;
> +
> + for (sg = sgl; !sg_is_chain(sg); sg++) {
> + kfree(sg_virt(sg));
> + if (sg_is_last(sg))
> + break;
> + }
> +
> + sg = sg_is_last(sg) ? NULL : sg_chain_ptr(sg);
> + free_page((unsigned long)sgl);
> + sgl = sg;
> + }
> +}
> +
> +static int err_print_to_sgl(struct i915_gpu_state *error)
> +{
> + struct drm_i915_error_state_buf m;
> struct drm_i915_error_object *obj;
> struct timespec64 ts;
> int i, j;
>
> - if (!error) {
> - err_printf(m, "No error state collected\n");
> + if (READ_ONCE(error->sgl))
> return 0;
> - }
> +
> + memset(&m, 0, sizeof(m));
> + m.i915 = error->i915;
I'm almost tempted to suggest to wrap the following code with
__err_print_to_sgl to protect from unnecessary motion? I tried
to look through it and it seems OK. But the next time type of
m changes it's going to be huge.
This is (with the assumption that sg innards are extracted as
a follow-up);
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
Regards, Joonas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
