Created attachment 100115 [details] [review]
add API to assign scheduler and priority delta

Comment on attachment 100115 [details] [review]
add API to assign scheduler and priority delta

>diff -urN glib2.0-2.12.12.orig/glib/gthread.h glib2.0-2.12.12.new/glib/gthread.h
>--- glib2.0-2.12.12.orig/glib/gthread.h	2007-09-18 14:28:30.000000000 +0300
>+++ glib2.0-2.12.12.new/glib/gthread.h	2007-11-23 12:29:35.000000000 +0200
>@@ -106,6 +115,11 @@ GThreadFunctions
>   void      (*thread_self)        (gpointer              thread);
>   gboolean  (*thread_equal)       (gpointer              thread1,
> 				   gpointer              thread2);
>+  gint      (*priority_min)       (GThreadScheduler      scheduler);
>+  gint      (*priority_max)       (GThreadScheduler      scheduler);
>+  gboolean  (*scheduler_set)      (gpointer              thread,
>+                                   GThreadScheduler      scheduler,
>+                                   gint                  priority);
> };

This is going to break the ABI, because in g_thread_init(GThreadFunctions *init) you do:

GThreadFunctions g_thread_functions_for_glib_use;
g_thread_functions_for_glib_use = *init;

and sizeof (GThreadFunctions in application built against old ABI) < sizeof (GThreadFunctions)

Maybe a safe-ish way to add those would be to introduce g_thread_init_with_size () and add
#define g_thread_init(init) g_thread_init_with_size(init, sizeof(*init))

A few comments:

* We deliberately limited priorities in GLib to a few choices instead of a full implementation dependent integer range. It makes it easier to program portably against GLib. Is a fine-grained priority really indispensable for real-time threads?

* As mentioned, we cannot break the ABI. I'm not sure, what the best solution would be here, maybe removing priority_min, priority_max (not needed if using GThreadPriority) and extending thread_set_priority to also set the scheduler after passing a magic 2nd argument. If old libs are called from new binaries, the second parameter is just ignored. If new libs are called from old binaries, the 2nd argument is not the magic value, so the 3rd argument for the scheduler is ignored. 

* The reason, GThreadPool is not allowing to set priorities, is that you would need to switch the priorities each time, you're assigning a new thread to a task. threads are shared between pools ans as such might end up being reused by pools with other priorities or schedulers. Changing those parameters might very well be a costly operation, so you should have some mechanisms to prevent changing thread priorities/schedulers too often. Your patch is not even adapting priorities/schedulers when reusing threads, let alone implementing such a "caching" mechanism.

(In reply to comment #3)
> against GLib. Is a fine-grained priority really indispensable for real-time
> threads?

The problem is that with current glib interface it is not possible to make a thread realtime (ie. it's always SCHED_OTHER). Delta (priority) range for SCHED_OTHER can be different than for SCHED_RR or SCHED_FIFO. In Linux case, there's only one priority for normal (SCHED_OTHER) threads, 0. For SCHED_RR and SCHED_FIFO it is 1-99.

> * As mentioned, we cannot break the ABI. I'm not sure, what the best solution
> would be here, maybe removing priority_min, priority_max (not needed if using
> GThreadPriority) and extending thread_set_priority to also set the scheduler
> after passing a magic 2nd argument. If old libs are called from new binaries,

Maybe this could be done, I have to check in more detail. It requires a bit of refactoring the code as the priority range is currently determined at thread creation time. But regenerating the priority range in set_priority is of course possible.

I would however, propose that a new API is created or break the existing ABI with additions (once it becomes possible due to other developments) to extend the functionality.

> * The reason, GThreadPool is not allowing to set priorities, is that you would
> need to switch the priorities each time, you're assigning a new thread to a
> task. threads are shared between pools ans as such might end up being reused 

The way I use this priority support is to create two (or more) separate thread pools. One with "time critical" priority and one with "normal priority". When a task is assigned to a thread pool, suitable pool is chosen based on the needs for the particular task.

Created attachment 102899 [details] [review]
Alternative approach to set scheduler

This patch provides an alternative approach for setting scheduler for threads and threadpools.

g_thread_create_full()
GError* is always the last parameter by convention :/

Otherwise this looks better.

What if GLib instead exposed a function which would be called in a thread pool to initialize new threads?  Then you could do whatever you wanted inside that function, including setting scheduling priority or other OS-specific functions.

That feels cleaner to me than wrapping FIFO, RR etc. in GLib; do they have obvious equivalents in Windows and OS X for example?

I see two ways:

1) the callback is called with a new GThread and the app can do some platform-specific post init stuff, like setting a priority

2) the callback is creating the thread and configuring it. This would allow to set other thread parameters that can only be set when creating the thread, but might also need a free-function.

Not sure if we would need a free function in anyway.

Actualy if the api is

typedef GThread* (*GThreadCreateFunction) (GThreadPool *pool,
   gpointer user_data);

GThreadCreateFunction g_thread_pool_set_thread_creation_callback (GThreadPool *pool, GThreadCreateFunction create_func);

Then applications can do:

GThreadCreateFunction def_thread_create_func;

static GThread
my_thread_create_func1 (GThreadPool *pool, gpointer user_data)
{
  GThread *t;

  // use default thread creation
  t = def_thread_create_func(pool->func, NULL);
  if(t) {
    MyData *my_data = (MyData *)user_data;
    // do post creation
  }

  return t;
}

static GThread
my_thread_create_func1 (GThreadPool *pool, gpointer user_data)
{
  GThread *t;
  MyData *my_data = (MyData *)user_data;

  // use own GThread creation
  t = g_thread_create_full (pool->func, ..., my_data->priority, ...);
  if(t) {
    // do post creation
  }

  return t;
}

...
GThreadPool *pool;

pool = g_thread_pool_new ( ...);
def_thread_create_func = g_thread_pool_set_thread_creation_callback (pool, 
    my_thread_create_func1);

...

The discussion about threadpool management has moved on to GTask and e.g. bug 687223

Lets close this one