Date: Wed, 14 Jun 2017 15:10:40 +0200
From: Petr Mladek <pmladek@suse.com>
To: Joe Lawrence <joe.lawrence@redhat.com>
Cc: live-patching@vger.kernel.org, linux-kernel@vger.kernel.org,
        Josh Poimboeuf <jpoimboe@redhat.com>, Jessica Yu <jeyu@redhat.com>,
        Jiri Kosina <jikos@kernel.org>, Miroslav Benes <mbenes@suse.cz>
Subject: Re: [PATCH 2/3] livepatch: add shadow variable documentation
Message-ID: <20170614131040.GD15013@pathway.suse.cz>
References: <1496341526-19061-1-git-send-email-joe.lawrence@redhat.com>
 <1496341526-19061-3-git-send-email-joe.lawrence@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1496341526-19061-3-git-send-email-joe.lawrence@redhat.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2842
Lines: 79

On Thu 2017-06-01 14:25:25, Joe Lawrence wrote:
> Document the new shadow variable API, including a few common use cases.
> 
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> ---
>  Documentation/livepatch/shadow-vars.txt | 175 ++++++++++++++++++++++++++++++++
>  1 file changed, 175 insertions(+)
>  create mode 100644 Documentation/livepatch/shadow-vars.txt
> 
> diff --git a/Documentation/livepatch/shadow-vars.txt b/Documentation/livepatch/shadow-vars.txt
> new file mode 100644
> index 000000000000..7df99ade4615
> --- /dev/null
> +++ b/Documentation/livepatch/shadow-vars.txt
> +API
> +---
> +
> +void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data);
> +
> +  Description: Allocate and attach a new shadow variable.
> +  Parameters:
> +
> +    void *obj  - pointer to original data
> +    char *var  - string key describing new data
> +    gfp_t gfp  - GFP flags used to allocate shadow variable metadata
> +    void *data - pointer to new data
> +
> +  Returns: the shadow variable data element, otherwise NULL on failure.
> +
> +
> +void klp_shadow_detach(void *obj, char *var);
> +
> +  Description: Detach and free a shadow variable.
> +  Parameters:
> +
> +      void *obj  - pointer to original data
> +      char *var  - string key describing new data
> +
> +
> +void *klp_shadow_get(void *obj, char *var);
> +
> +  Description: Retrieve a shadow variable data pointer.
> +  Parameters:
> +
> +      void *obj  - pointer to original data
> +      char *var  - string key describing new data
> +
> +  Returns: the shadow variable data element, otherwise NULL if the
> +  <obj, var> combination is not found.

I would convert all the above into comments of the function definitions
in shadow.c. It is really helpful when you try to undestand the code
using cscope. Also it is much easier to keep the documentation
and code in sync when they are in the same file.

> +
> +Concurrency notes:
> +
> +* The shadow variable API simply provides a relationship between an
> +<obj, var> pair and a pointer value.  It is the responsibility of the
> +caller to provide any mutual exclusion required of the shadow data.
> +
> +* Once klp_shadow_attach() adds a shadow variable to the
> +klp_shadow_hash, it is considered live and klp_shadow_get() may
> +return the shadow variable's data pointer.  Therefore, initialization of
> +shadow data should be completed before attaching the shadow variable.
> +
> +* If the API is called under a special context (like spinlocks),
> +set the GFP flags passed to klp_shadow_attach() accordingly.
> +
> +* The klp_shadow_hash is an RCU-enabled hashtable and should be safe
> +against concurrent klp_shadow_detach() and klp_shadow_get() operations.

I would prefer to have this in the source file as well.

Otherwise, I like the descriptions and the examples.

Best Regards,
Petr