Re: Wording proposal for request_safe_update

John,

as said in my original mail on this subject, mixing
request_update/async_request_update is not the real problem.

  To me, async_request_update has a single purpose: protect the kernel's
internal structures of the update handling, when an update is requested
from the outside. This covers races between concurrent request_update
calls to different channels, that are now possible due to the presence
of external callers.

  Anything else is NOT handled by this mechanism. Am I missing some
magic synchronisation definition in the proposal?

I'll try to explain my point with the following, simple example (untested):

struct async_double_signal
  : virtual sc_signal_inout_if<double>, sc_prim_channel
{
  double m_cur_val, m_new_val;

  void write( double d ){
    async_request_update();
    m_new_val = d;
  }

  void update()
  {
    // ...
    m_cur_val = m_new_val;
  }
};

  So let's look at this channel. Is everything fine, if we _only_ call
'write' from a single process outside of SystemC? Are we done? No. We
still have races on the shared data members during update() and write().
 m_new_val might contain a "new" value, an "old" value, or complete
garbage (if the write 'm_new_val = d' is not atomic).

  Additional calls to write() from SystemC processes might of course add
further races _within_ the write() function. But this again affects the
internal integrity of the channel, not the interaction with the kernel.
 Even with the above 'write' function, that always calls
async_request_update, these races can still be an issue. So using
request_update/async_request_update is the least to worry about, here.

The simple rule could be:
  - use async_request_update in functions, that can be called from
    the outside of SystemC
  - use request_update otherwise
This requires a safe implementation of the update phase, of course.

  We should add a recommendation, that the shared data needs to be
properly synchronised. But the details are really out of the scope of
the LRM and very application-specific, I think.

That said, I'm still fine with disallowing both (async_)request_update
calls from a primitive channel. It just feels to me like barking up
some rather small tree... ;-)

Greetings from Oldenburg,
  Philipp

On 28/11/10 10:13, john.aynsley@doulos.com wrote:
> David, Philipp,
>
> Wait a minute. Maybe I'm just being slow here. update() has to copy data from some memory used by the external thread to some memory used by the SystemC kernel. Both areas would typically be members of the primitive channel itself, as in sc_signal::m_new_val and sc_signal::m_cur_val. We cannot allow SystemC and non-SystemC threads to race each other to write to the equivalent of sc_signal::m_new_val. If we allowed both request_update and async_request_update, the update() method would typically have to know (using an application-defined flag) which was being serviced so it copy data from the appropriate domain (or something to that effect).
>
> In other words, I think I am agreeing with David that an application shall not call both for a given channel. Also, should we give the LRM reader some guidelines as the expected usage, i.e. (in the absence of explicit application-defined sync mechanisms) update() should copy from members of the channel that are only written to by the external thread?
>
> John A
>
>
> -----David C Black <dcblack@xtreme-eda.com> wrote: -----
> To: john.aynsley@doulos.com
> From: David C Black <dcblack@xtreme-eda.com>
> Date: 11/25/2010 01:43PM
> Cc: "Philipp A. Hartmann" <philipp.hartmann@offis.de>, systemc-p1666-technical@eda.org
> Subject: Re: Wording proposal for request_safe_update
>
> I personally don't think the idea of a channel calling both request mechanisms in the same cycle is a good idea. The idea is to create special channels for the async_request_update. The async version will always have a performance penalty to ensure safety.
>
> When I looked at the code and considered the issue, I realized it would be bad if that happened with the implementation I had; however, it was easy to make it resilient. I added a separate pointer for the safe vs normal sc_prim_channel mechanisms. This adds a small space overhead with no performance penalty (unless you count construction time during elaboration). So you can call both mechanisms with the net result that update will be called twice. Since I think this should be discouraged, I don't think it is a bad thing. At least the code is now "safe" from that scenario.
>
> Of course an sc_prim_channel derived class could have an internal flag that it uses to determine which of the two it will use (for its duration).
>
> BOTTOM LINE: I suggest adding text to the effect:
>
>
> An sc_prim_channel object shall not call both request_update and async_request_update. The results of violating this are undefined.
>
> On Thu, Nov 25, 2010 at 4:06 AM, <john.aynsley@doulos.com> wrote:
> Philipp, David,
>
> Philip wrote "I think, we can allow both as long as they result in a _single_ call to 'update' in the corresponding update phase as it is for multiple, regular 'request_update' calls, too. (although I haven't found any explicit wording covering this?)."
>
> In my mind I was assuming the regular request_update had this behavior, because I remember writing that the update request queue is a "set". I need to tweak the LRM to make this unambiguous.
>
> Given that, we have two choices:
>
> * Allow request_update and aync_request_update to be mixed for the same primitive channel instance. This means that the implementation has to be able to handle this case.
>
> * Disallow it, which may simplify the implementation a little in the sense that async_request_update can assume there are no regular update requests in the queue for that primitive channel.
>
> What does the current implementation do, David? Which way should we go?
>
> John A
>
>
>
> From: "Philipp A. Hartmann" <philipp.hartmann@offis.de> To: David C Black <dcblack@xtreme-eda.com>, john.aynsley@doulos.com Cc: systemc-p1666-technical@eda.org Date: 24/11/2010 16:09 Subject: Re: Wording proposal for request_safe_update
>
>
>
>
> John, David,
>
> simultaneous calls to '(async_)request_thread_safe_update' and
> 'request_update' and are not really the key issue.
>
> I think, we can allow both as long as they result in a _single_ call
> to 'update' in the corresponding update phase as it is for multiple,
> regular 'request_update' calls, too. (although I haven't found any
> explicit wording covering this?).
>
> The real issue is here, that the user can't arbitrarily access the
> state of the current channel that actually provides this asynchronous
> access (and therefore calls request_thread_safe_update internally).
> Such concurrent accesses from inside the SystemC model and from other
> parts of the application might need to be protected by additional,
> application defined locking techniques. But this is out of the scope of
> the standard. It could warrant a note, though.
>
> Greetings from Oldenburg,
> Philipp
>
> On 24/11/10 15:08, David C Black wrote:
> >
> > On Wed, Nov 24, 2010 at 4:31 AM, <john.aynsley@doulos.com> wrote:
> [snip]
> >> 2nd question: Are there any dependencies or interactions between
> >> request_update and request_thread_safe_update? Can you have both for the
> >> same primitive channel instance? What happens?
> >
> > Good point, I think it should be stated that it is undefined if both are
> > called. Choose one or the other.

-- 
Philipp A. Hartmann
Hardware/Software Design Methodology Group
OFFIS Institute for Information Technology
R&D Division Transportation · FuE-Bereich Verkehr
Escherweg 2 · 26121 Oldenburg · Germany · http://www.offis.de/
Phone/Fax: +49-441-9722-420/282 · PGP: 0x9161A5C0 · Skype: phi.har
-- 
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.