Re: [PATCH 6/7] dma-iommu: implement ->alloc_noncontiguous

[Sergey Senozhatsky]

On (21/03/01 08:21), Christoph Hellwig wrote:
> On Mon, Mar 01, 2021 at 04:17:42PM +0900, Sergey Senozhatsky wrote:
> > > > Do you think we could add the attrs parameter to the
> > > > dma_alloc_noncontiguous() API?
> > > 
> > > Yes, we could probably do that.
> > 
> > I can cook a patch, unless somebody is already looking into it.
> 
> I plan to resend the whole series with the comments very soon.

Oh, OK.

I thought the series was in linux-next already so a single patch
would do.

-ss
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 6/7] dma-iommu: implement ->alloc_noncontiguous

[Sergey Senozhatsky]

On (21/02/16 09:49), Christoph Hellwig wrote:
> > When working on the videobuf2 integration with Sergey I noticed that
> > we always pass 0 as DMA attrs here, which removes the ability for
> > drivers to use DMA_ATTR_ALLOC_SINGLE_PAGES.
> > 
> > It's quite important from a system stability point of view, because by
> > default the iommu_dma allocator would prefer big order allocations for
> > TLB locality reasons. For many devices, though, it doesn't really
> > affect the performance, because of random access patterns, so single
> > pages are good enough and reduce the risk of allocation failures or
> > latency due to fragmentation.
> > 
> > Do you think we could add the attrs parameter to the
> > dma_alloc_noncontiguous() API?
> 
> Yes, we could probably do that.

I can cook a patch, unless somebody is already looking into it.

-ss
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 5/6] media: uvcvideo: Use dma_alloc_noncontiguos API

[Sergey Senozhatsky via iommu]

On Tue, Dec 8, 2020 at 1:54 PM Tomasz Figa  wrote:

>
> In any case, Sergey is going to share a preliminary patch on how the
> current API would be used in the V4L2 videobuf2 framework. That should
> give us more input on how such a helper could look.
>


My current WIP (deep WIP) series can be found at [0]. The patch that adds
new
DMA API support is at the head of the series [1]. New DMA API requires us to
have several internal videobuf2 API changes before we can proceed [2]: v4l2
and
videobuf2 core do not pass enough information to the vb2 allocators now.
Previously,
if user space requests non-coherent allocation v4l2 would set queue
dma_attr bit,
videobuf2 core would pass queue->dma_attrs to vb2 allocator, which would
those dma_attrs for dma_alloc(). So everything was transparent (sort of).
Since we
don't have that dma_attr flag anymore, there is no way for v4l2 to pass the
request
information (coherent or non-coherent) to the vb2 allocator. Hence we need
to rework
the vb2 allocator API. I currently pass vb2 pointer, but we decided to
rework it again
and to pass dedicated VB2_ALLOC_FLAGS from the videobuf2 core to the
allocator. This is still in my private tree and not completely ready, will
push those
patches to github later.

Another thing to notice is that the new API requires us to have two
execution branches
in allocators - one for the current API; and one for the new API (if it's
supported and
if user-space requested non-coherent allocation).

[0] https://github.com/sergey-senozhatsky/linux-next-ss/commits/master
[1]
https://github.com/sergey-senozhatsky/linux-next-ss/commit/a45f48b483daee59594c62e4aaf01790aab960c8
[2]
https://github.com/sergey-senozhatsky/linux-next-ss/commit/b784145101c398da7fe9e2608b6001e58e05a9b5

-ss
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 5/6] media: uvcvideo: Use dma_alloc_noncontiguos API

[Sergey Senozhatsky]

On (20/12/08 13:54), Tomasz Figa wrote:
> 
> In any case, Sergey is going to share a preliminary patch on how the
> current API would be used in the V4L2 videobuf2 framework. That should
> give us more input on how such a helper could look.

HUGE apologies for the previous screw up! I replied in the
gmail web-interface and that did not work out as expected
(at all, big times).

I'm sorry about that!

Take two, no html this time around.

---

My current WIP (deep WIP) series can be found at [0]. The patch that adds new
DMA API support is at the head of the series [1]. New DMA API requires us to
have several internal videobuf2 API changes before we can proceed [2]: v4l2 and
videobuf2 core do not pass enough information to the vb2 allocators now. 
Previously,
if user space requests non-coherent allocation v4l2 would set queue dma_attr 
bit,
videobuf2 core would pass queue->dma_attrs to vb2 allocator, which would 
those dma_attrs for dma_alloc(). So everything was transparent (sort of). Since 
we
don't have that dma_attr flag anymore, there is no way for v4l2 to pass the 
request
information (coherent or non-coherent) to the vb2 allocator. Hence we need to 
rework
the vb2 allocator API. I currently pass vb2 pointer, but we decided to rework 
it again
and to pass dedicated VB2_ALLOC_FLAGS from the videobuf2 core to the 
allocator. This is still in my private tree and not completely ready, will push 
those
patches to github later.

Another thing to notice is that the new API requires us to have two execution 
branches
in allocators - one for the current API; and one for the new API (if it's 
supported and
if user-space requested non-coherent allocation).

[0] https://github.com/sergey-senozhatsky/linux-next-ss/commits/master
[1] 
https://github.com/sergey-senozhatsky/linux-next-ss/commit/a45f48b483daee59594c62e4aaf01790aab960c8
[2] 
https://github.com/sergey-senozhatsky/linux-next-ss/commit/b784145101c398da7fe9e2608b6001e58e05a9b5

-ss
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 5/6] media: uvcvideo: Use dma_alloc_noncontiguos API

[Sergey Senozhatsky]

On (20/11/30 09:34), Christoph Hellwig wrote:
> 
> > +   uvc_urb->pages = dma_alloc_noncontiguous(dma_dev, stream->urb_size,
> > +_urb->dma,
> > +gfp_flags | __GFP_NOWARN, 0);
> > +   if (!uvc_urb->pages)
> > +   return false;
> > +
> > +   uvc_urb->buffer = vmap(uvc_urb->pages,
> > +  PAGE_ALIGN(stream->urb_size) >> PAGE_SHIFT,
> > +  VM_DMA_COHERENT, PAGE_KERNEL);
> > +   if (!uvc_urb->buffer) {
> > +   dma_free_noncontiguous(dma_dev, stream->urb_size,
> > +  uvc_urb->pages, uvc_urb->dma);
> > +   return false;
> > +   }
> > +
> > +   if (sg_alloc_table_from_pages(_urb->sgt, uvc_urb->pages,
> > +   PAGE_ALIGN(stream->urb_size) >> PAGE_SHIFT, 0,
> > +   stream->urb_size, GFP_KERNEL)) {
> > +   vunmap(uvc_urb->buffer);
> > +   dma_free_noncontiguous(dma_dev, stream->urb_size,
> > +  uvc_urb->pages, uvc_urb->dma);
> > +   return false;
> > +   }
> 
> I wonder if we should lift this into a helper.  On the one hand I had
> proliferating struct scatterlist usage, on the other hand it is all over
> the media and drm code anyway, and duplicating this doesn't help anyone.

Not that I have any sound experience in this area, but the helper
probably won't hurt. Do you also plan to add vmap() to that helper
or dma_alloc_noncontiguous()/sg_alloc_table_from_pages() only?

helper()
{
dma_alloc_noncontiguous();
sg_alloc_table_from_pages();

if ((dma_attrs & DMA_ATTR_NO_KERNEL_MAPPING) == 0)
vmap();
}

videobuf2-dma-contig still has to carry around two versions: one that
deals with the noncontig pages and sgt (new API); and the current one.
But if the helper will include fallback to coherent allocations then
this may change, depending on the helper implementation.

-ss
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 5/6] media: uvcvideo: Use dma_alloc_noncontiguos API

[Sergey Senozhatsky]

On (20/11/25 23:19), Ricardo Ribalda wrote:
[..]
> + if (uvc_urb->pages)
> + dma_sync_sgtable_for_device(stream_to_dmadev(uvc_urb->stream),
> + _urb->sgt, DMA_FROM_DEVICE);

[..]

> + if (uvc_urb->pages)
> + dma_sync_sgtable_for_cpu(stream_to_dmadev(stream),
> +  _urb->sgt, DMA_FROM_DEVICE);

[..]

> + uvc_urb->pages = dma_alloc_noncontiguous(dma_dev, stream->urb_size,
> +  _urb->dma,
> +  gfp_flags | __GFP_NOWARN, 0);

Do we need to pass __GFP_NOWARN? It seems that

dma_alloc_noncontiguous()
  __iommu_dma_alloc_noncontiguous()
__iommu_dma_alloc_pages()

does this internally.

> + if (!uvc_urb->pages)
> + return false;
> +
> + uvc_urb->buffer = vmap(uvc_urb->pages,
> +PAGE_ALIGN(stream->urb_size) >> PAGE_SHIFT,
> +VM_DMA_COHERENT, PAGE_KERNEL);

This is not related to Ricardo's patch, just a side note:

  I think VM_DMA_COHERENT needs to be renamed. I found it a bit confusing
  to see DMA_COHERENT mapping being dma_sync-ed. It turned out that the
  flag has different meaning.

-ss
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 10/28] mm: only allow page table mappings for built-in zsmalloc

[Sergey Senozhatsky]

On (20/04/09 10:08), Minchan Kim wrote:
> > > Even though I don't know how many usecase we have using zsmalloc as
> > > module(I heard only once by dumb reason), it could affect existing
> > > users. Thus, please include concrete explanation in the patch to
> > > justify when the complain occurs.
> > 
> > The justification is 'we can unexport functions that have no sane reason
> > of being exported in the first place'.
> > 
> > The Changelog pretty much says that.
> 
> Okay, I hope there is no affected user since this patch.
> If there are someone, they need to provide sane reason why they want
> to have zsmalloc as module.

I'm one of those who use zsmalloc as a module - mainly because I use zram
as a compressing general purpose block device, not as a swap device.
I create zram0, mkfs, mount, checkout and compile code, once done -
umount, rmmod. This reduces the number of writes to SSD. Some people use
tmpfs, but zram device(-s) can be much larger in size. That's a niche use
case and I'm not against the patch.

-ss
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 07/17] debugobjects: Move printk out of db lock critical sections

[Sergey Senozhatsky]

On (11/23/18 12:48), Petr Mladek wrote:
[..]
> > This should make serial consoles re-entrant.
> > So printk->console_driver_write() hopefully will not deadlock.
> 
> Is the re-entrance safe? Some risk might be acceptable in Oops/panic
> situations. It is much less acceptable for random warnings.

Good question.

But what's the alternative? A deadlock in a serial console driver; such
that even panic() is not guaranteed to make through it (at least of now).
debug objects are used from the code which cannot re-entrant console
drivers.

bust_spinlock is called from various paths, not only panic.
git grep bust_spinlocks | wc -l
62

So we already switch to re-entrant consoles (and accept the risks) in
mm/fault.c, kernel/traps.c and so on. Which, I guess, makes us a little
more confident, faults/traps happen often enough.

It seems, that, more or less, serial consoles are ready to handle it.
UART consoles in ->write() callbacks just do a bunch of writel() [for
every char + \r\n].

-ss
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 07/17] debugobjects: Move printk out of db lock critical sections

[Sergey Senozhatsky]

On (11/22/18 11:16), Peter Zijlstra wrote:
> > So maybe we need to switch debug objects print-outs to _always_
> > printk_deferred(). Debug objects can be used in code which cannot
> > do direct printk() - timekeeping is just one example.
> 
> No, printk_deferred() is a disease, it needs to be eradicated, not
> spread around.

deadlock-free printk() is deferred, but OK.


Another idea then:

---

diff --git a/lib/debugobjects.c b/lib/debugobjects.c
index 70935ed91125..3928c2b2f77c 100644
--- a/lib/debugobjects.c
+++ b/lib/debugobjects.c
@@ -323,10 +323,13 @@ static void debug_print_object(struct debug_obj *obj, 
char *msg)
void *hint = descr->debug_hint ?
descr->debug_hint(obj->object) : NULL;
limit++;
+
+   bust_spinlocks(1);
WARN(1, KERN_ERR "ODEBUG: %s %s (active state %u) "
 "object type: %s hint: %pS\n",
msg, obj_states[obj->state], obj->astate,
descr->name, hint);
+   bust_spinlocks(0);
}
debug_objects_warnings++;
 }

---

This should make serial consoles re-entrant.
So printk->console_driver_write() hopefully will not deadlock.

IOW, this turns serial consoles into early_consoles, for a while ;)

-ss
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 07/17] debugobjects: Move printk out of db lock critical sections

[Sergey Senozhatsky]

On (11/22/18 14:57), Waiman Long wrote:
> > [..]
> >> As a side note, one of the test systems that I used generated a
> >> debugobjects splat in the bootup process and the system hanged
> >> afterward. Applying this patch alone fix the hanging problem and the
> >> system booted up successfully. So it is not really a good idea to call
> >> printk() while holding a raw spinlock.
> > Right, I like this patch.
> > And I think that we, maybe, can go even further.
> >
> > Some serial consoles call mod_timer(). So what we could have with the
> > debug objects enabled was
> >
> > mod_timer()
> >  lock_timer_base()
> >   debug_activate()
> >printk()
> > call_console_drivers()
> >  foo_console()
> >   mod_timer()
> >lock_timer_base()   << deadlock
> >
> > That's one possible scenario. The other one can involve console's
> > IRQ handler, uart port spinlock, mod_timer, debug objects, printk,
> > and an eventual deadlock on the uart port spinlock. This one can
> > be mitigated with printk_safe. But mod_timer() deadlock will require
> > a different fix.
> >
> > So maybe we need to switch debug objects print-outs to _always_
> > printk_deferred(). Debug objects can be used in code which cannot
> > do direct printk() - timekeeping is just one example.
> 
> Actually, I don't think that was the cause of the hang.

Oh, I didn't suggest that this was the case. Just talked about more
problems with printk in debug objects. Serial consoles call mod_time,
mod_timer calls debug objects, debug objects call printk and end up
in serial console again. Serial consoles are not re-entrant at this
point.

> The debugobjects splat was caused by debug_object_is_on_stack(), below
> was the output:
>
> [    6.890048] ODEBUG: object (ptrval) is NOT on stack
> (ptrval), but annotated.
> [    6.891000] WARNING: CPU: 28 PID: 1 at lib/debugobjects.c:369
> __debug_object_init.cold.11+0x51/0x2d6
[..]
>    11.270654] systemd[1]: systemd 239 running in system mode. (+PAM
> +AUDIT +SELINUX +IMA -APPARMOR +SMACK +SYSVINIT +UTMP +LIBCRYPTSETUP
> +GCRYPT +GNUTLS +ACL +XZ +LZ4 +SECCOMP +BLKID +ELFUTILS +KMOD +IDN2 -IDN
> +PCRE2 default-hierarchy=legacy)
> [   11.311307] systemd[1]: Detected architecture x86-64.
> [   11.316420] systemd[1]: Running in initial RAM disk.
> 
> Welcome to
> 
> The system is not responsive at this point.
> 
> I am not totally sure what caused this.

Hmm, me neither.

-ss
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 07/17] debugobjects: Move printk out of db lock critical sections

[Sergey Senozhatsky]

On (11/21/18 11:49), Waiman Long wrote:
[..]
> > case ODEBUG_STATE_ACTIVE:
> > -   debug_print_object(obj, "init");
> > state = obj->state;
> > raw_spin_unlock_irqrestore(>lock, flags);
> > +   debug_print_object(obj, "init");
> > debug_object_fixup(descr->fixup_init, addr, state);
> > return;
> >  
> > case ODEBUG_STATE_DESTROYED:
> > -   debug_print_object(obj, "init");
> > +   debug_printobj = true;
> > break;
> > default:
> > break;
> > }
> >  
> > raw_spin_unlock_irqrestore(>lock, flags);
> > +   if (debug_chkstack)
> > +   debug_object_is_on_stack(addr, onstack);
> > +   if (debug_printobj)
> > +   debug_print_object(obj, "init");
> >
[..]
>
> As a side note, one of the test systems that I used generated a
> debugobjects splat in the bootup process and the system hanged
> afterward. Applying this patch alone fix the hanging problem and the
> system booted up successfully. So it is not really a good idea to call
> printk() while holding a raw spinlock.

Right, I like this patch.
And I think that we, maybe, can go even further.

Some serial consoles call mod_timer(). So what we could have with the
debug objects enabled was

mod_timer()
 lock_timer_base()
  debug_activate()
   printk()
call_console_drivers()
 foo_console()
  mod_timer()
   lock_timer_base()   << deadlock

That's one possible scenario. The other one can involve console's
IRQ handler, uart port spinlock, mod_timer, debug objects, printk,
and an eventual deadlock on the uart port spinlock. This one can
be mitigated with printk_safe. But mod_timer() deadlock will require
a different fix.

So maybe we need to switch debug objects print-outs to _always_
printk_deferred(). Debug objects can be used in code which cannot
do direct printk() - timekeeping is just one example.

-ss
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu