Re: [RFC PATCH 01/17] shm-signal: shared-memory signals

From: Gregory Haskins <ghaskins@novell.com>
To: Avi Kivity <avi@redhat.com>
Cc: linux-kernel@vger.kernel.org, agraf@suse.de,
	pmullaney@novell.com, pmorreale@novell.com,
	anthony@codemonkey.ws, rusty@rustcorp.com.au,
	netdev@vger.kernel.org, kvm@vger.kernel.org
Subject: Re: [RFC PATCH 01/17] shm-signal: shared-memory signals
Date: Tue, 31 Mar 2009 16:58:31 -0400	[thread overview]
Message-ID: <49D283F7.7030508@novell.com> (raw)
In-Reply-To: <49D280C0.8010802@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 4762 bytes --]

Avi Kivity wrote:
> Gregory Haskins wrote:
>> This interface provides a bidirectional shared-memory based signaling
>> mechanism.  It can be used by any entities which desire efficient
>> communication via shared memory.  The implementation details of the
>> signaling are abstracted so that they may transcend a wide variety
>> of locale boundaries (e.g. userspace/kernel, guest/host, etc).
>>
>> The shm_signal mechanism supports event masking as well as spurious
>> event delivery mitigation.
>> +
>> +/*
>> + *---------
>> + * The following structures represent data that is shared across
>> boundaries
>> + * which may be quite disparate from one another (e.g. Windows vs
>> Linux,
>> + * 32 vs 64 bit, etc).  Therefore, care has been taken to make sure
>> they
>> + * present data in a manner that is independent of the environment.
>> + *-----------
>> + */
>> +
>> +#define SHM_SIGNAL_MAGIC 0x58fa39df
>> +#define SHM_SIGNAL_VER   1
>> +
>> +struct shm_signal_irq {
>> +    __u8                  enabled;
>> +    __u8                  pending;
>> +    __u8                  dirty;
>> +};
>>   
>
> Some ABIs may choose to pad this, suggest explicit padding.

Yeah, good idea.  What is the official way to do this these days?  Are
GCC pragmas allowed?

>
>> +
>> +enum shm_signal_locality {
>> +    shm_locality_north,
>> +    shm_locality_south,
>> +};
>> +
>> +struct shm_signal_desc {
>> +    __u32                 magic;
>> +    __u32                 ver;
>> +    struct shm_signal_irq irq[2];
>> +};
>>   
>
> Similarly, this should be padded to 0 (mod 8).
Ack

>
> Instead of versions, I prefer feature flags which can be independently
> enabled or disabled.

Totally agreed.  If you look, most of the ABI has a type of "NEGCAP"
(negotiate capabilities) feature.  The version number is a contingency
plan in case I still have to break it for whatever reason.   I will
always opt for the feature bits over bumping the version when its
feasible (especially if/when this is actually in the field).

>
>> +
>> +/* --- END SHARED STRUCTURES --- */
>> +
>> +#ifdef __KERNEL__
>> +
>> +#include <linux/interrupt.h>
>> +
>> +struct shm_signal_notifier {
>> +    void (*signal)(struct shm_signal_notifier *);
>> +};
>>   
>
> This means "->inject() has been called from the other side"?

Yep
>
> (reading below I see this is so.  not used to reading well commented
> code... :)

:)

>
>> +
>> +struct shm_signal;
>> +
>> +struct shm_signal_ops {
>> +    int      (*inject)(struct shm_signal *s);
>> +    void     (*fault)(struct shm_signal *s, const char *fmt, ...);
>>   
>
> Eww.  Must we involve strings and printf formats?

This is still somewhat of a immature part of the design.  Its supposed
to be used so that by default, its a panic.  But on the host side, we
can do something like inject a machine-check.  That way malicious/broken
guests cannot (should not? ;) be able to take down the host.  Note today
I do not map this to anything other than the default panic, so this
needs some love.

But given the asynchronous nature of the fault, I want to be sure we
have decent accounting to avoid bug reports like "silent MCE kills the
guest" ;)  At least this way, we can log the fault string somewhere to
get a clue.

>
>> +    void     (*release)(struct shm_signal *s);
>> +};
>> +
>> +/*
>> + * signaling protocol:
>> + *
>> + * each side of the shm_signal has an "irq" structure with the
>> following
>> + * fields:
>> + *
>> + *    - enabled: controlled by shm_signal_enable/disable() to
>> mask/unmask
>> + *               the notification locally
>> + *    - dirty:   indicates if the shared-memory is dirty or clean. 
>> This
>> + *               is updated regardless of the enabled/pending state
>> so that
>> + *               the state is always accurately tracked.
>> + *    - pending: indicates if a signal is pending to the remote locale.
>> + *               This allows us to determine if a
>> remote-notification is
>> + *               already in flight to optimize spurious
>> notifications away.
>> + */
>>   
>
> When you overlay a ring on top of this, won't the ring indexes convey
> the same information as ->dirty?

I agree that the information may be redundant with components of the
broader shm state.  However, we need this state at this level of scope
in order to function optimally, so I dont think its a huge deal to have
this here as well.  Afterall, the shm_signal library can only assess its
internal state.  We would have to teach it how to glean the broader
state through some mechanism otherwise (callback, perhaps), but I don't
think its worth it.

-Greg

>
>

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]