[Mesa-dev] [RFC] opencl: mega-cl

[Rob Clark]

On Mon, Feb 26, 2018 at 6:12 PM, Francisco Jerez <currojerez at riseup.net> wrote:
> Rob Clark <robdclark at gmail.com> writes:
>
>> On Sun, Feb 25, 2018 at 3:00 PM, Francisco Jerez <currojerez at riseup.net> wrote:
>>> Seems like a serious hack to me to work around broken linking...  IMO we
>>> should just fix the linking issue.  The symbols for the various GLSL
>>> types need to be linked with the proper binding and visibility -- I
>>> assume that the cause of your problem is that people are making
>>> assumptions about the equality of GLSL types based on their memory
>>> addresses *and* marking the symbols as hidden *and* passing pointers to
>>> GLSL types across shared objects?  That sounds like a recipe for
>>> disaster.
>>
>> tbh, maybe hack, or I think more likely, maybe a good idea.. I'm not
>> terribly sold on the idea of dynamically loading pipe driver and
>> linking a lot of shared code into N different pipe_${driver}.so on
>> disk, since the # of drivers seems to be greater than # of state
>> trackers.. not to mention multiple copies of shared gallium code in
>> memory due to being statically linked into both state tracker and
>> driver.
>>
>
> It would probably make sense to link that shared code dynamically into
> the pipe-loader modules in order to avoid the duplication you're talking
> about (e.g. by building them as a meson shared_module() instead of a
> shared_library() in order to allow the common symbols to be undefined in
> the pipe-loader module which will then be pulled in by the state
> tracker) -- Actually that's likely to fix your problem too.

yes, for pipe_loader_dynamic case, this is the only way I can see for
it to work (although I think we still want to link statically in the
pipe_loader_static case).  Well that, or making the glsl_types symbols
not -Bsymbolic.

The potential problem here is, what if an app links in both (for ex.)
CL + GL, but somehow from different mesa versions.  I suppose that
might be a corner case, but it would fall over badly w/ nir/glsl_types
in a .so.  But I think the way things are packaged in most distro's, a
user could upgrade specifically the mesa-gl package but not the
mesa-cl package.  (Also, we don't really want to export glsl/nir
symbols lest someone mistake them for a stable ABI.)

I guess I am ok with the approach of (a) adding support for "mega-cl"
using piple_loader_static plus (b) adding a libnir.so for the dynamic
case, and letting users of pipe_loader_dynamic keep both pieces in the
broken corner cases.

>> That said, glsl_type's defn does seem to expect to only exist once in
>> a process, ie. == is ptr comparison and when you link nir/glsl_types
>> into both state tracker and driver, glsl_type ptrs are getting passed
>> across that boundary.  I'm not really sure that is worth fixing (ie.
>> why should it exist twice in a process in the first place?)
>>
>
> *Shrug* It's kind of a micro-optimization with nasty side effects
> (namely that you cannot pass GLSL IR or NIR across shared object
> boundaries without things exploding) and dubious payoff, it would
> probably be reasonable to simply drop the micro-optimization, but that
> wasn't the solution I was suggesting (but rather, making the
> optimization work by exporting the symbols GLSL type equality cares
> about with the right linkage).

well, I think it is long past too late to try to solve that.  If it
were only in c++ code we could overload == but it is a mix of c++ and
c, and tracking down all the places that do pointer comparisons seems
like it would be tricky to avoid missing some and having subtle bugs
as a result.

>> Maybe there are some linker tricks to solve this, idk.. that is a bit
>> outside my area of expertise.
>
> More than adding new linker tricks I think you just need to make sure
> that existing linker tricks we're doing right now ('-fvisibility=hidden'
> and various linker scripts) don't break the build by forcing common
> symbols to be local, along the lines of:
>
> | diff --git a/src/compiler/glsl_types.h b/src/compiler/glsl_types.h
> | index ab0b2637649..9d8a5ea341c 100644
> | --- a/src/compiler/glsl_types.h
> | +++ b/src/compiler/glsl_types.h
> | @@ -243,7 +243,7 @@ public:
> |     /*@{*/
> |  #undef  DECL_TYPE
> |  #define DECL_TYPE(NAME, ...) \
> | -   static const glsl_type *const NAME##_type;
> | +   static const glsl_type *const NAME##_type __attribute__((visibility("default")));
> |  #undef  STRUCT_TYPE
> |  #define STRUCT_TYPE(NAME) \
> |     static const glsl_type *const struct_##NAME##_type;
> | diff --git a/src/gallium/targets/opencl/opencl.sym b/src/gallium/targets/opencl/opencl.sym
> | index 9fcc57692b8..fadc2eb6c1d 100644
> | --- a/src/gallium/targets/opencl/opencl.sym
> | +++ b/src/gallium/targets/opencl/opencl.sym
> | @@ -2,6 +2,10 @@
> |       global:
> |               cl*;
> |               opencl_dri_*;
> | +
> | +             extern "C++" {
> | +                     glsl_type::*;
> | +             };
> |       local:
> |               *;
> |  };
> | diff --git a/src/gallium/targets/pipe-loader/pipe.sym b/src/gallium/targets/pipe-loader/pipe.sym
> | index 605cb83d802..801b0f44d56 100644
> | --- a/src/gallium/targets/pipe-loader/pipe.sym
> | +++ b/src/gallium/targets/pipe-loader/pipe.sym
> | @@ -7,6 +7,10 @@
> |               # due to LLVM being initialized multiple times.
> |               radeon_drm_winsys_create;
> |               amdgpu_winsys_create;
> | +
> | +             extern "C++" {
> | +                     glsl_type::*;
> | +             };
> |       local:
> |               *;
> |  };
>

iiuc, this would have the same corner case of moving nir/glsl_types to
a separate .so when mixing state tracker versions in a process.

>> To me, the mega-driver approach worked well for gl, and many of the
>> other state trackers are already using libpipe_loader_static.. why not
>> just fix the problems with that approach and abandon
>> libpipe_loader_dynamic?  This seems like it would work out better for
>> memory footprint since pages for drivers not used simply wouldn't get
>> paged in.  And you aren't ending up with two copies of libgallium or
>> libnir code in memory.
>>
>
> The dynamic pipe loader would give you better disk and memory footprint
> than the static one assuming that the problem with linking shared code
> statically into each pipe-loader module was fixed, because that would
> allow the pipe driver code to be shared in memory and disk among
> different state trackers, but I guess that the benefit is slight at this
> point, I understand your reluctance to fix it ;).
>

I guess I haven't measured, but I wonder if this is even true.  The
cost increase is duplicating pipe_driver code in each state tracker
(but I guess there are only 3 or 4 that typically end up on users
filesys, and of those only mesa/st really needs to link in *all* the
drivers, clover and the video state trackers are only supported by a
few drivers each), but is that offset by de-duplicating shared code
(gallium/aux, libnir, etc)?

I did have a thought earlier today.  Since we have glvnd, and
opencl-icd (and I guess vdpau and vaapi have a sort of loader), where
the state tracker isn't directly exporting the API symbols but rather
just a jump-table used by the icd .so, we could actually perhaps do a
mega-mega driver where all the state trackers and all the drivers are
linked into a single .so.. That would reduce the disk and memory
footprint the most.  (Well, ok, it would cost maybe some virtual
address space, but pages associated w/ unused state trackers wouldn't
be demand-paged in, so meh?)

BR,
-R


>> BR,
>> -R
>>[...]