Re: [PATCH 00/11] ALSA: PCM state reference optimization

[Takashi Iwai <tiwai@suse.de>]

On Tue, 27 Sep 2022 03:22:07 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Mon, Sep 26, 2022 at 03:55:47PM +0200, Takashi Iwai wrote:
> > Hi,
> > 
> > this is a patch set for simplifying the reference to the current PCM
> > state by having the local copy in runtime instead of relying on
> > runtime->status indirection.  This also hardens against the attack by
> > modifying the mmapped status record.
>  
> The overall patches looks good to me and I have no objections, while I
> have some slight opinions to them in a place of sound driver developer.
> 
> > The first patch does the basic job in the core PCM side,
> 
> The main concern is indirect accessing to state field via some pointer
> hops. I think addition of helper macro at first step eases centre of your
> work, like:
> 
> ```
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index 8c48a5bce88c..f6a160cb8135 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -669,6 +669,20 @@ void snd_pcm_stream_unlock_irqrestore(struct snd_pcm_substream *substream,
>              stream <= SNDRV_PCM_STREAM_LAST;           \
>              stream++)
>  
> +/**
> + * snd_pcm_stream_state - Return state in runtime of the PCM substream.
> + * @substream: substream to check. runtime should be attached.
> + *
> + * Return state in runtime of the PCM substream. The substream should exists and
> + * runtime should be attached to it.
> + */
> +static inline snd_pcm_state_t snd_pcm_stream_state(const snd_pcm_substream *substream)
> +{
> +       snd_BUG_ON(!(sub) || !(sub)->runtime);
> +
> +       return substream->runtime->status->state;
> +}
> ```
> 
> As we can see, sound driver programmer sometimes checks state of runtime
> in their code, thus the macro could helps them as well as centre of your
> change.

This might help if we may need any change in future again.

But the NULL check is superfluous in most places, and the state check
is done often in a hot path, it's better to rip off.  So it's only
about the read of runtime->state, and then it's doubtful whether it
brings much value at this moment; that's the reason I didn't introduce
such a macro for now.


Takashi