Re: [PATCH 07/25] drm/i915: Cache the error string

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux