<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div>Hi Glenn,</div><div><br></div><div>thanks a lot for your patches! I'm very glad to see more "infrastructure improvements" patches that make the system easier to use or more robust. Comments inline.</div><br><div><div>On Mar 1, 2013, at 5:35 AM, Glenn Elliott <<a href="mailto:gelliott@cs.unc.edu">gelliott@cs.unc.edu</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">#1: prop/cpu-affinity<br>The first branch is prop/cpu-affinity. Litmus requires that when a thread becomes a real-time task that this thread is executing on a CPU within the thread's desired cluster/partition. This precondition can be achieved by properly setting up the thread's CPU affinity mask. Unfortunately, liblitmus's be_migrate_to() is troublesome to work with because it takes a single CPU parameter. It is entirely possible that the caller thread only knows what cluster/partition it should run on, not a particular CPU within that cluster/partition. Things are further complicated if you add release master into the mix---tasks shouldn't include the release master (which can have an arbitrary value) in their affinity mask. In the end, figuring out what CPU affinity mask to set can be complicated.<br><br>(8d65a8e4db) introduces the following functions (and other related functions):<br><br>* migrate_to_cpu(): Has the same functionality as be_migrate_to().<br><br>* migrate_to_partition(): Migrate a task to specified partition. CPUs are indexed 0..m (or 0..m-1, if release master is enabled). The release master can be an arbitrary CPU. The partition-to-CPU mapping will just skip over the release master CPU.<br><br>* migrate_to_cluster(): Migrate a task to specified cluster. The caller just gives the cluster index and cluster size (small changes to C-EDF should allow us to remove the cluster size parameter in the future). If release master is used, then the cluster size parameter should be the size of the largest cluster. The release master is still excluded from the affinity mask (unless forced by __migrate_to_cluster()). If cluster size is 1, then migrate_to_partition() is called instead.<br><br>* num_online_cpus(): Returns the number of online CPUs.<br><br>* release_master(): Returns the CPU that is the current release master. -1 if no release master is in use.<br></blockquote><div><br></div><div>These changes look ok to me (though I liked the be_ in be_migrate_to() to indicate that only best-effort tasks may call it). I left a few low-level comments in Github.</div><div><br></div><blockquote type="cite">#2: prop/pthreadification<br>This branch introduces a new pthread-styled API to setting up real-time task parameters. The sporadic_* functions and macros are taking ever more parameters. Worse, these parameters do not always apply to the active scheduler (ex. fixed priority). This API should hopefully simply things with a familiar interface and make code more expressive.<br></blockquote><div><br></div><div>Mh, this looks less straightforward to me. To be honest, it looks a bit too complicated for my taste.</div><div><br></div><div>First, the sporadic_* macros were never meant to be the primary interface. The fill-in-a-struct method is the primary method for configuring task parameters. The macros were intended to be convenience wrappers for common cases. If they seem more trouble than benefit now, I'm happy removing them entirely.</div><div><br></div><div>Second, what's wrong with simply populating a struct and passing it to a syscall? I don't really see what we gain from introducing a getter/setter API, especially not with verbose litmus_attr_… prefixes. Pthreads was standardized to be portable and needs all this indirection; we're not portable anyway so I'd prefer to keep it simple. That said, if there's broad consensus that this API is better I'm ok merging it. It just seems to make things more complicated to me.</div><div><br></div><blockquote type="cite">Yes, this is more code than even a very long function call or macro. However, consider: (1) that existing macros can wrap this new API; </blockquote><div><br></div><div>Not sure why we need it then. I'd prefer removing the macros altogether if they are considered to not be helpful.</div><br><blockquote type="cite">(2) this approach allows thread attributes to be copied around;</blockquote><div><br></div><div>Same with structs.</div><br><blockquote type="cite"> (3) time units are made explicit (no more guessing!);</blockquote><div><br></div><div>Let's standardize on nanoseconds everywhere, with the exception of APIs that operate on doubles. Unit conversion should be limited to parameter parsing code.</div><br><blockquote type="cite"> (4) very clear and expressive; </blockquote><div><br></div><div>I disagree;</div><div><br></div><div><pre class="line" style="margin-top: 0px; margin-bottom: 0px; padding: 0px 0px 0px 10px; border: 0px; font-size: 12px; font-family: Consolas, 'Liberation Mono', Courier, monospace; line-height: 16px; ">litmus_attr_set_period(&attr, p, TIME_NS);</pre><div><br></div><div>is no more clear or expressive than</div><div><br></div><div><pre class="line" style="margin-top: 0px; margin-bottom: 0px; padding: 0px 0px 0px 10px; border: 0px; font-size: 12px; font-family: Consolas, 'Liberation Mono', Courier, monospace; line-height: 16px; ">param.period = p;</pre><div><br></div></div><div>The former could invoke all kind of magic; the latter is a straightforward plain-old-C assignment without any potential for surprises.</div><div><br></div></div><blockquote type="cite">(5) caller only needs to specify the parameters they care about (no need to deal with budgets or fixed priorities); </blockquote><div><br></div><div>The same is true with the param struct approach. Just zero the struct first and then fill in what you want.</div><br><blockquote type="cite">(6) uses prop/cpu-affinity to make cluster/partition assignment easy. We can also consider combining some of these setters into reasonable groups, such as: litmus_attr_set_params(&attr, exe, period, time_units).<br><br>I believe that there has also been some discussion about moving task configuration parameters to control pages or /proc. I understand the motivation for this, but I thought a familiar API might be easier to work with. Also, there is no reason why the parameters set in litmus_attr_t could not be directed to control pages or /proc instead of set_rt_task_params(). Should the user care where these parameters go (syscall, control page, or /proc)?<br></blockquote><div><br></div><div>No, the user should not care. I don't think we'll change the API any time soon, though; we lack volunteers with time to do this.</div><br><blockquote type="cite"><br>(fd5a0c8163) This patch builds on (d9daa24e24) to make a litmus_rt_task_create() function. It's basically pthread_create() with an additional litmus_attr_t parameter. I'm not entirely sold that this patch is needed, but I am interested in feedback. It currently has two limitations/drawbacks: (1) all binaries must now include -pthread when compiling and linking; (2) errors in setting Litmus parameters are not reported back to the caller---not sure how to get around this one yet.<br></blockquote><div><br></div><div>Personally, I don't like the pthreads API, so this isn't really a huge improvement to me. I would prefer not having to link against pthreads for "regular" tasks. Error reporting is important.</div><div><br></div><div>I would suggest to merge a simplified and rebased version of the migration API changes, and to wait for consensus regarding the attr patch set.</div><div><br></div><div>Thanks,</div><div>Björn</div><div><br></div><div><br></div><div><br></div></div></body></html>