linklist member traversal and output

Luigi Castelli

Hi folks,

I have an external that employs some code similar to the following:

void linklist_member_output(void *src, void *out)
{
    t_source *source = (t_source *)src;
    t_atom argv[4];

    atom_setlong(argv+0, source->a);
    atom_setlong(argv+1, source->b);
    atom_setlong(argv+2, source->c);
    atom_setlong(argv+3, source->d);

    outlet_anything(out, gensym("source"), 4, argv);
}

void myobj_output(t_myobj *x)
{
    linklist_funall(x->source_list, (method)linklist_member_output, x->out);
}

Is it safe to do so ?

I am asking because I know it's bad to protect outlet function calls and I know that linklist functions are thread-safe already (they are already protected...).

Now, with a different construct, the equivalent would be something like:

void myobj_output(t_myobj *x)
{
    t_source *source;
    t_atom argv[4];

    source = linklist_getindex(x->source_list, 0);
    while (source) {
        atom_setlong(argv+0, source->a);
        atom_setlong(argv+1, source->b);
        atom_setlong(argv+2, source->c);
        atom_setlong(argv+3, source->d);
        outlet_anything(x->out, gensym("source"), 4, argv);
        linklist_next(x->source_list, source, (void **)&source);
    }
}

But now, do I need to manually enclose the above code between a critical_enter(0) and critical_exit(0) function to ensure that the whole linklist traversal operation is thread-safe ?

In any case is there something that I have to keep in mind when using one construct instead of the other ?
in what situation would you use one and not the other ?

Thanks for any feedback.

- Luigi

andrea agostini

Hi Luigi.

I'm pretty sure that your second example is incorrect, since you enclose your outlet calls within a critical region.

About the first example, it's ok if your linklist is readonly. If it's not, I'm not sure whether the called function runs inside a lock, but my guess is that it does (avoiding that would be quite cumbersome, and not 100% reliable) - which makes your outlet calls forbidden again!

The only safe method I see to accomplish what you want to do is copying the data you want to output to a temporary storage (probably with linklist_funall), and then output them from outside any lock.

hth
aa

Luigi Castelli

What Andrea says makes sense...

Vanille, even though I hope it is as you say, (writing operations are between mutexes while reading operations are not) I don't really see how the linklist API could know if I want to read or write in linklist_funall()
So my assumption is that linklist_funall() is ALWAYS between mutexes and therefore both my code constructs are incorrect, even though they might work.

So, for now, I agree with Andrea. The only solution is to copy the data to a buffer and then output from outside any mutex or critical region. I know it makes the code longer and less elegant, but I don't see any other approach...

- Luigi

Luigi Castelli

Yes, I would like to know Cycling opinion too... :)

- Luigi

Luigi Castelli

Hi Vanille,

no, I haven't found a good way either... except the copying strategy that Andrea suggested, but I would like to avoid that since it does make the algorithm longer and more complicated.

I'll let you know if I find something worth sharing...

- Luigi

Joshua Kit Clayton

1. linklist_funall and linklist_methodall do not hold the linklist lock around the callback, so that the callback can make other linklist calls (linklist doesn't use recursive locks since they are slower and linklists are used everywhere) So you should be safe to outlet in your callback.

2. You can also use next without critical regions, but the contents of the list can change (as they can in linklist_funall). It shouldn't crash however.

3. In general, if you know a bit about what you're doing, I would recommend avoiding using the global critical region. It's the easiest one to use, but it presents the greatest opportunity for thread contention.

Luigi Castelli

Thank you for your clear explanation, Joshua.

So that leaves me with 2 final questions:

1 - when you recommend avoiding the global critical region, do you mean we should avoid critical regions altogether and use mutexes instead such as in your examples:

systhread_mutex_lock(x->p_mutex);
// ... code to protect
systhread_mutex_unlock(x->p_mutex);

or could we still be using the ext_critical.h interface, such as:

critical_new(&x->c_critical);    // created in the new method, of course

critical_enter(x->c_critical);
//... code to protect
critical_exit(x->c_critical);

are the two examples pretty much equivalent ?

2 - ok, clear about linklist_funall() and linklist_methodall().
Now, even though I am safe to outlet in the callback, the content of the list
could still change.
How do I protect the linklist against changes, and still maintain safety outputting data ?
Do I need to copy the data to a temporary array like Andrea initially suggested or are there better ways ?

Thanks for the clarifications.

- Luigi

andrea agostini

Hi Luigi.

I think I can answer your first question:

Using the global critical region is not recommended because a lot of stuff (internal Max operations, other copies your own external, etc.) depend upon it, and by locking it in one thread you would possibly prevent unrelated things to happen in another, even when they safely could.

If you create and use your own critical region(s) you won't run into this situation, but anyway critical regions (which essentially are recursive mutexes - http://en.wikipedia.org/wiki/Reentrant_mutex ) are much heavier than standard mutexes, which for this reason should be preferred when possible.

Best
aa

Joshua Kit Clayton

Andrea's response for #1 is appropriate.

For #2, you could solve a number of ways, if it's actually important to you that the list doesn't change while outputting, yes, something like the previous suggestions for copying to a local list or array makes sense.

Hope this helps.

-Joshua