The repository for this work is here:
https://github.com/kkonopko/gstreamer/tree/filemem-allocator

Created attachment 233601 [details] [review]
Proposed implementation: added documentation and other minor improvements

> Problems
> --------
> 
> - portability
> What is the best approach to not break other platforms support?
> It should be possible to add the support for this allocator on other platform
> as needed but at the moment ideally the allocator files should be complied only
> for *nix system. I'll have a look how to best achieve it unless someone can
> suggest me patterns already used (I haven't been looking for these yet).
> 

In the second version of the patch [1] I added simple handling for unsupported platforms. If the required features are not available, the implementation code is not compiled. The allocator initialization function reports an error on the runtime instead.

> - initialization
> I feel that the initialization is a bit awkward, i. e. the allocator needs to
> know some information in advance (the file size and where to store it). It sort
> of make sense if one think of the allocator as a global resource but I was
> wondering whether it'd be useful to have an ability to create them on a whim,
> i. e. not necessarily add them to the registry.
> 

I'm leaning towards the allocator as a global resource. I have a full production use case where the application initializes the allocator in the main context and passed its name around through object properties. The object (element) that wants to use different allocation schemes uses the allocator name as a property. By default the property is set to NULL for such an object which ends up using the default allocator. Otherwise it picks up selected (e. g. "FileMemory") allocator. 

[1] https://bugzilla.gnome.org/review?bug=691299&attachment=233601

I think the fd and other parameters should be part of the GstMemory and the allocator can be made a singleton then. It should also be possible to e.g. let filesrc create such memory instead of only using it for random allocations. The name should probably be GstMappedFileMemory or GstMMapMemory.

Take a look at gst-plugins-base/gst-libs/gst/allocators btw, that already contains a dmabuf allocator which is very similar (but should be kept separate). Could you merge your allocator into that library too?

There are 2 interesting ways to use this:

 1) as a replacement for the system memory allocator.

 2) as an allocator proposed in allocation queries.

I can see two separate APIs for this:

 1) needs something like 

      alloc = gst_filemem_allocator_new(gsize size, const gchar *templ);

    and then 

      gst_allocator_set_default (alloc);
    
    You could also have elements do that and propose the allocator but I can't
    see why they would right now.

 2) needs something like:

     alloc = gst_filemem_allocator_new_from_fd (gint fd, ...);

    Because if you are to propose this in an allocator you probably have your
    own fd somewhere that you want others to write into. I can only think of
    a ringbuffer or maybe a muxer.

Maybe you also want to make it possible to grow the file automatically or never overwrite things (in the case of a muxer).

(In reply to comment #4)
> I think the fd and other parameters should be part of the GstMemory and the
> allocator can be made a singleton then.
Making it a singleton was my first thought but then I faced scenarios when an app (a long running process or a daemon if you like) created and destroyed GStreamer pipelines quite frequently and I had to add an API to reset the allocator (not included to the attached patch yet).

Passing fd sounds like an interesting idea.

> It should also be possible to e.g. let
> filesrc create such memory instead of only using it for random allocations. The
> name should probably be GstMappedFileMemory or GstMMapMemory.
Sure, I'm strongly considering this.
> 
> Take a look at gst-plugins-base/gst-libs/gst/allocators btw, that already
> contains a dmabuf allocator which is very similar (but should be kept
> separate). Could you merge your allocator into that library too?
Thanks for the hint.  I had a brief look and it seems like a good idea.  I noticed that I should review thread safety in the allocator (I didn't need it at all in my use case due to locking at a different level).

(In reply to comment #5)
> 
>  2) needs something like:
> 
>      alloc = gst_filemem_allocator_new_from_fd (gint fd, ...);
> 
>     Because if you are to propose this in an allocator you probably have your
>     own fd somewhere that you want others to write into. I can only think of
>     a ringbuffer or maybe a muxer.
> 
> Maybe you also want to make it possible to grow the file automatically or never
> overwrite things (in the case of a muxer).

Thanks for the suggestions, worth considering.  My goal was to extend the allocator as new use cases emerge but considering these features might be a good thing for the allocator design (currently focused only on one use case).  Let me do some hacking first and I'll get back to you as soon as I can.

Created attachment 250246 [details] [review]
Moved mapped file allocator to gst-plugins-base

For a good start I moved the allocator to gst-plugins-base and renamed classes to GstMappedFileMemory and GstMappedFileallocator as Sebastian suggested in his comment [1].

This is the most tedious part of the work to get this proposal into better shape.  Now it's ready for more work which will hopefully be also more fun.
  
[1] https://bugzilla.gnome.org/show_bug.cgi?id=691299#c4

I think there should be a GstMMapMemory that is independent of this allocator and only has an fd and the offset inside it, and the mapping functions would then call mmap() or whatever is required. This very specific allocator here would be used to implement a special version of the GstMMapMemory, but there would also be a GstMMapAllocator that just provides the default functions for handling mmap'd memory.

Review of attachment 250246 [details] [review]:

::: gst-libs/gst/allocators/gstmappedfile.c
@@ +162,3 @@
+      maxsize);
+
+  if (allocator->f_offset_next + maxsize > allocator->file_size) {

I think you should implement a better block allocation strategy, that allows to properly deallocate and reallocate blocks

@@ +207,3 @@
+     of blocks available or more advanced if needed. */
+
+#if defined (HAVE_FALLOCATE) && HAVE_DECL_FALLOC_FL_PUNCH_HOLE

So if this is not supported, the memory will slowly run full and then nothing can be allocated anymore? I think the allocator should not be supported at all then

@@ +256,3 @@
+  /* Can't really control the alignment
+     Can we do something about the data offset (GstAllocationParams)? Is it
+     the right place to handle it? */

No the alignment and padding and everything should be handled during allocation

@@ +503,3 @@
+  GST_ERROR ("%s allocator is not supported on this platform",
+      GST_ALLOCATOR_MAPPEDFILE);
+#endif // GST_MAPPEDFILEALLOCATOR_SUPPORTED

No C99 comments please

@@ +532,3 @@
+
+  gst_allocator_register (GST_ALLOCATOR_MAPPEDFILE,
+      GST_ALLOCATOR_CAST (allocator));

Not sure if registering the allocator globally with a fixed temp template makes sense. What if multiple such allocators with different temp templates should be used?

::: tests/check/libs/allocators.c
@@ +29,3 @@
+  GstAllocator *alloc = NULL;
+
+  gst_mappedfile_allocator_init (size, "/tmp/gstmappedfile-XXXXXX");

Use g_get_tmp_dir() here

(In reply to comment #9)
My gut feeling is that what you suggest makes a lot sense but please see my questions below.

> I think there should be a GstMMapMemory that is independent of this allocator
> and only has an fd and the offset inside it, and the mapping functions would
> then call mmap() or whatever is required.

As I understand data redundancy is acceptable here.  I mean GstMMapMemory (or GstMappedFileMemory as I renamed it to) inherits from GstMemory which already contains a reference to the allocator and GstMMapAllocator (or GstMappedFileAllocator as I renamed it to) has got the file descriptor.  My understanding is that any memory is always somehow related to and dependent on its allocator.  Is your intention to decouple things here?

> This very specific allocator here

Ie. which one as opposed to GstMMapAllocator?

> would be used to implement a special version of the GstMMapMemory, but there
> would also be a GstMMapAllocator that just provides the default functions for
> handling mmap'd memory.

Does it mean that the implementation of all of these would live in the same place (file)?  How would GstMMapMemory be reused/shared after making it independent of its allocator?

BTW, I renamed classes to GstMappedFile* as IMHO this is more generic as opposed to implementation specific MMap (what about Windows folks?).

(In reply to comment #10)
> Review of attachment 250246 [details] [review]:
> 
> ::: gst-libs/gst/allocators/gstmappedfile.c
> @@ +162,3 @@
> +      maxsize);
> +
> +  if (allocator->f_offset_next + maxsize > allocator->file_size) {
> 
> I think you should implement a better block allocation strategy, that allows to
> properly deallocate and reallocate blocks
> 
> @@ +207,3 @@
> +     of blocks available or more advanced if needed. */
> +
> +#if defined (HAVE_FALLOCATE) && HAVE_DECL_FALLOC_FL_PUNCH_HOLE
> 
> So if this is not supported, the memory will slowly run full and then nothing
> can be allocated anymore? I think the allocator should not be supported at all
> then

This allocator is bound to run out of space sooner or later as it's supposed to be intialised with a specific total size.  The reason for this is that there's no way to reliably resize the file if any of its mmap()-ed blocks have been given out and are in use.  I'd like it wasn't true so please let me know if I'm wrong here.

I agree that not reusing freed blocks is poor but given my reasoning above it's not the end of the world.  I agree reusing blocks should be supported (and should be a condition to accept this allocator at all) but there are at least two ways to approach it:
- declare the allocator to allocate blocks of a certain size only
This makes it dead simple as it boils down to maintaining a list of free blocks

- support arbitrary block size allocations
This requires merging subsequent free blocks (buddy allocation, defragmentation etc.).

> 
> @@ +532,3 @@
> +
> +  gst_allocator_register (GST_ALLOCATOR_MAPPEDFILE,
> +      GST_ALLOCATOR_CAST (allocator));
> 
> Not sure if registering the allocator globally with a fixed temp template makes
> sense. What if multiple such allocators with different temp templates should be
> used?
>

Yes, that hurts me and I'm keen to make it better based on previous suggestions (pass a file descriptor to a file opened somewhere else etc.).

Thanks for the other spots.  I fixed them immediately as they were minors.  I need a bigger chunk of spare time to tackle bigger issues.  Any additional feedback would be very helpful.

(In reply to comment #11)
> My understanding is that any memory is always somehow related to and dependent on
> its allocator.  Is your intention to decouple things here?
> 
> > This very specific allocator here
> 
> Ie. which one as opposed to GstMMapAllocator?

Yes, the idea would be to have a generic GstMMapMemory that can be created by multiple allocators. E.g. v4l2 could provide such memory too, or filesrc. You can have multiple allocators implementing the same GstMemory.

(In reply to comment #12)
> (In reply to comment #10)
> > Review of attachment 250246 [details] [review] [details]:
> > 
> > ::: gst-libs/gst/allocators/gstmappedfile.c
> > @@ +162,3 @@
> > +      maxsize);
> > +
> > +  if (allocator->f_offset_next + maxsize > allocator->file_size) {
> > 
> > I think you should implement a better block allocation strategy, that allows to
> > properly deallocate and reallocate blocks
> > 
> > @@ +207,3 @@
> > +     of blocks available or more advanced if needed. */
> > +
> > +#if defined (HAVE_FALLOCATE) && HAVE_DECL_FALLOC_FL_PUNCH_HOLE
> > 
> > So if this is not supported, the memory will slowly run full and then nothing
> > can be allocated anymore? I think the allocator should not be supported at all
> > then
> 
> This allocator is bound to run out of space sooner or later as it's supposed to
> be intialised with a specific total size.  The reason for this is that there's
> no way to reliably resize the file if any of its mmap()-ed blocks have been
> given out and are in use.  I'd like it wasn't true so please let me know if I'm
> wrong here.

No, that's true unfortunately. Having a fixed size is ok, as long as blocks can be reused.

> I agree that not reusing freed blocks is poor but given my reasoning above it's
> not the end of the world.  I agree reusing blocks should be supported (and
> should be a condition to accept this allocator at all) but there are at least
> two ways to approach it:
> - declare the allocator to allocate blocks of a certain size only
> This makes it dead simple as it boils down to maintaining a list of free blocks
> 
> - support arbitrary block size allocations
> This requires merging subsequent free blocks (buddy allocation, defragmentation
> etc.).

Yeah I agree that this is not trivial :) But without that this allocator isn't really that useful, it will always go out of memory sooner or later. Going with fixed (configurable) size blocks in the beginning would be a good start I guess.

Created attachment 272026 [details] [review]
Proposed implementation

Here's my recent attempt to implement the allocator.  This time, as suggested, I implemented reusing allocated blocks.  Memory blocks can be of any size (although they are aligned to page size) and are merged when claimed back if they are continuous.  For example, if the user allocates 2 x 1k, 2k and 1k, and then deallocated the blocks, they get merged into a single 4k block.

I also replaced awkward API with a single function to create the allocator which take a file descriptor and the maximum file size (the allocator resizes the file when created).  The file descriptor gets associated with the allocator rather than memory (as in dmabuf) because the intention is for a single file to provide a memory arena where multiple blocks can be requested from.  For this reason the allocator has to do all the bookkeeping.

Cheers,
Kris

Created attachment 272027 [details] [review]
Proposed implementation

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/issues/78.