On Thu, Apr 23, 2009 at 07:33:40PM -0400, Chuck Lever wrote:
> Recently, commit ad5b365c fixed a data type alignment issue in
> nsm_init_private() by introducing put_unaligned(). We don't actually
> _require_ an unaligned access to nsm_private here.
>
> Instead, we should always use memcpy/memcmp to access the contents of
> RPC opaque data types. This permits us to continue to define these as
> simple character arrays, as most legacy RPC code does, and to rely on
> gcc to pack the fields in in-core structures optimally without breaking
> on platforms that require strict alignment.
OK, I'm confused. What structures will get packed differently?
--b.
>
> Signed-off-by: Chuck Lever <[email protected]>
> ---
>
> fs/lockd/mon.c | 12 +++++-------
> 1 files changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
> index 5017d50..763509a 100644
> --- a/fs/lockd/mon.c
> +++ b/fs/lockd/mon.c
> @@ -16,8 +16,6 @@
> #include <linux/sunrpc/svc.h>
> #include <linux/lockd/lockd.h>
>
> -#include <asm/unaligned.h>
> -
> #define NLMDBG_FACILITY NLMDBG_MONITOR
> #define NSM_PROGRAM 100024
> #define NSM_VERSION 1
> @@ -278,14 +276,14 @@ static struct nsm_handle *nsm_lookup_priv(const struct nsm_private *priv)
> */
> static void nsm_init_private(struct nsm_handle *nsm)
> {
> - u64 *p = (u64 *)&nsm->sm_priv.data;
> struct timespec ts;
> - s64 ns;
> + u64 buf[2];
>
> ktime_get_ts(&ts);
> - ns = timespec_to_ns(&ts);
> - put_unaligned(ns, p);
> - put_unaligned((unsigned long)nsm, p + 1);
> + buf[0] = timespec_to_ns(&ts);
> + buf[1] = (unsigned long)nsm;
> +
> + memcpy(nsm->sm_priv.data, buf, SM_PRIV_SIZE);
> }
>
> static struct nsm_handle *nsm_create_handle(const struct sockaddr *sap,
>
On Apr 28, 2009, at 12:31 PM, J. Bruce Fields wrote:
> On Thu, Apr 23, 2009 at 07:33:40PM -0400, Chuck Lever wrote:
>> Recently, commit ad5b365c fixed a data type alignment issue in
>> nsm_init_private() by introducing put_unaligned(). We don't actually
>> _require_ an unaligned access to nsm_private here.
>>
>> Instead, we should always use memcpy/memcmp to access the contents of
>> RPC opaque data types. This permits us to continue to define these
>> as
>> simple character arrays, as most legacy RPC code does, and to rely on
>> gcc to pack the fields in in-core structures optimally without
>> breaking
>> on platforms that require strict alignment.
>
> OK, I'm confused. What structures will get packed differently?
Any struct that has an nsm_private embedded in it, such as struct
nlm_reboot.
> --b.
>
>>
>> Signed-off-by: Chuck Lever <[email protected]>
>> ---
>>
>> fs/lockd/mon.c | 12 +++++-------
>> 1 files changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
>> index 5017d50..763509a 100644
>> --- a/fs/lockd/mon.c
>> +++ b/fs/lockd/mon.c
>> @@ -16,8 +16,6 @@
>> #include <linux/sunrpc/svc.h>
>> #include <linux/lockd/lockd.h>
>>
>> -#include <asm/unaligned.h>
>> -
>> #define NLMDBG_FACILITY NLMDBG_MONITOR
>> #define NSM_PROGRAM 100024
>> #define NSM_VERSION 1
>> @@ -278,14 +276,14 @@ static struct nsm_handle
>> *nsm_lookup_priv(const struct nsm_private *priv)
>> */
>> static void nsm_init_private(struct nsm_handle *nsm)
>> {
>> - u64 *p = (u64 *)&nsm->sm_priv.data;
>> struct timespec ts;
>> - s64 ns;
>> + u64 buf[2];
>>
>> ktime_get_ts(&ts);
>> - ns = timespec_to_ns(&ts);
>> - put_unaligned(ns, p);
>> - put_unaligned((unsigned long)nsm, p + 1);
>> + buf[0] = timespec_to_ns(&ts);
>> + buf[1] = (unsigned long)nsm;
>> +
>> + memcpy(nsm->sm_priv.data, buf, SM_PRIV_SIZE);
>> }
>>
>> static struct nsm_handle *nsm_create_handle(const struct sockaddr
>> *sap,
>>
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
On Tue, Apr 28, 2009 at 12:35:50PM -0400, Chuck Lever wrote:
> On Apr 28, 2009, at 12:31 PM, J. Bruce Fields wrote:
>> On Thu, Apr 23, 2009 at 07:33:40PM -0400, Chuck Lever wrote:
>>> Recently, commit ad5b365c fixed a data type alignment issue in
>>> nsm_init_private() by introducing put_unaligned(). We don't actually
>>> _require_ an unaligned access to nsm_private here.
>>>
>>> Instead, we should always use memcpy/memcmp to access the contents of
>>> RPC opaque data types. This permits us to continue to define these
>>> as
>>> simple character arrays, as most legacy RPC code does, and to rely on
>>> gcc to pack the fields in in-core structures optimally without
>>> breaking
>>> on platforms that require strict alignment.
>>
>> OK, I'm confused. What structures will get packed differently?
>
> Any struct that has an nsm_private embedded in it, such as struct
> nlm_reboot.
I don't see how that or any structure is changed by this patch.
--b.
>
>> --b.
>>
>>>
>>> Signed-off-by: Chuck Lever <[email protected]>
>>> ---
>>>
>>> fs/lockd/mon.c | 12 +++++-------
>>> 1 files changed, 5 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
>>> index 5017d50..763509a 100644
>>> --- a/fs/lockd/mon.c
>>> +++ b/fs/lockd/mon.c
>>> @@ -16,8 +16,6 @@
>>> #include <linux/sunrpc/svc.h>
>>> #include <linux/lockd/lockd.h>
>>>
>>> -#include <asm/unaligned.h>
>>> -
>>> #define NLMDBG_FACILITY NLMDBG_MONITOR
>>> #define NSM_PROGRAM 100024
>>> #define NSM_VERSION 1
>>> @@ -278,14 +276,14 @@ static struct nsm_handle
>>> *nsm_lookup_priv(const struct nsm_private *priv)
>>> */
>>> static void nsm_init_private(struct nsm_handle *nsm)
>>> {
>>> - u64 *p = (u64 *)&nsm->sm_priv.data;
>>> struct timespec ts;
>>> - s64 ns;
>>> + u64 buf[2];
>>>
>>> ktime_get_ts(&ts);
>>> - ns = timespec_to_ns(&ts);
>>> - put_unaligned(ns, p);
>>> - put_unaligned((unsigned long)nsm, p + 1);
>>> + buf[0] = timespec_to_ns(&ts);
>>> + buf[1] = (unsigned long)nsm;
>>> +
>>> + memcpy(nsm->sm_priv.data, buf, SM_PRIV_SIZE);
>>> }
>>>
>>> static struct nsm_handle *nsm_create_handle(const struct sockaddr
>>> *sap,
>>>
>
> --
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
On Apr 28, 2009, at 12:40 PM, J. Bruce Fields wrote:
> On Tue, Apr 28, 2009 at 12:35:50PM -0400, Chuck Lever wrote:
>> On Apr 28, 2009, at 12:31 PM, J. Bruce Fields wrote:
>>> On Thu, Apr 23, 2009 at 07:33:40PM -0400, Chuck Lever wrote:
>>>> Recently, commit ad5b365c fixed a data type alignment issue in
>>>> nsm_init_private() by introducing put_unaligned(). We don't
>>>> actually
>>>> _require_ an unaligned access to nsm_private here.
>>>>
>>>> Instead, we should always use memcpy/memcmp to access the
>>>> contents of
>>>> RPC opaque data types. This permits us to continue to define these
>>>> as
>>>> simple character arrays, as most legacy RPC code does, and to
>>>> rely on
>>>> gcc to pack the fields in in-core structures optimally without
>>>> breaking
>>>> on platforms that require strict alignment.
>>>
>>> OK, I'm confused. What structures will get packed differently?
>>
>> Any struct that has an nsm_private embedded in it, such as struct
>> nlm_reboot.
>
> I don't see how that or any structure is changed by this patch.
It's not. Note the phrase above in the description: "permits us to
_continue_ to define these" -- meaning, I'm not changing the structures.
It has been suggested that we use a union or __aligned attribute for
RPC opaques. The problem with that is that it would cause structs
like nlm_reboot to balloon in size; char x[] is aligned on bytes, but
a union of u8, u32, and u64 would be aligned on a double-word
boundary. That would make such structures larger.
Legacy RPC code, and any code generated by rpcgen, generally defines
an opaque as a character array. So again, using a union would be
inconsistent with other uses of RPC opaques.
The point is we want to define and access RPC opaque's consistently,
using memset() and memcpy(), since these are nothing more than byte
arrays. The code in nsm_init_private() was an exception to this, for
no reason. We don't really need special alignment macros in there, as
long as we access the RPC opaque field with the byte sets and copies.
Would it help if I described this patch as a "clean up" ?
> --b.
>
>>
>>> --b.
>>>
>>>>
>>>> Signed-off-by: Chuck Lever <[email protected]>
>>>> ---
>>>>
>>>> fs/lockd/mon.c | 12 +++++-------
>>>> 1 files changed, 5 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
>>>> index 5017d50..763509a 100644
>>>> --- a/fs/lockd/mon.c
>>>> +++ b/fs/lockd/mon.c
>>>> @@ -16,8 +16,6 @@
>>>> #include <linux/sunrpc/svc.h>
>>>> #include <linux/lockd/lockd.h>
>>>>
>>>> -#include <asm/unaligned.h>
>>>> -
>>>> #define NLMDBG_FACILITY NLMDBG_MONITOR
>>>> #define NSM_PROGRAM 100024
>>>> #define NSM_VERSION 1
>>>> @@ -278,14 +276,14 @@ static struct nsm_handle
>>>> *nsm_lookup_priv(const struct nsm_private *priv)
>>>> */
>>>> static void nsm_init_private(struct nsm_handle *nsm)
>>>> {
>>>> - u64 *p = (u64 *)&nsm->sm_priv.data;
>>>> struct timespec ts;
>>>> - s64 ns;
>>>> + u64 buf[2];
>>>>
>>>> ktime_get_ts(&ts);
>>>> - ns = timespec_to_ns(&ts);
>>>> - put_unaligned(ns, p);
>>>> - put_unaligned((unsigned long)nsm, p + 1);
>>>> + buf[0] = timespec_to_ns(&ts);
>>>> + buf[1] = (unsigned long)nsm;
>>>> +
>>>> + memcpy(nsm->sm_priv.data, buf, SM_PRIV_SIZE);
>>>> }
>>>>
>>>> static struct nsm_handle *nsm_create_handle(const struct sockaddr
>>>> *sap,
>>>>
>>
>> --
>> Chuck Lever
>> chuck[dot]lever[at]oracle[dot]com
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
On Tue, Apr 28, 2009 at 01:24:24PM -0400, Chuck Lever wrote:
> On Apr 28, 2009, at 12:40 PM, J. Bruce Fields wrote:
>> On Tue, Apr 28, 2009 at 12:35:50PM -0400, Chuck Lever wrote:
>>> On Apr 28, 2009, at 12:31 PM, J. Bruce Fields wrote:
>>>> On Thu, Apr 23, 2009 at 07:33:40PM -0400, Chuck Lever wrote:
>>>>> Recently, commit ad5b365c fixed a data type alignment issue in
>>>>> nsm_init_private() by introducing put_unaligned(). We don't
>>>>> actually
>>>>> _require_ an unaligned access to nsm_private here.
>>>>>
>>>>> Instead, we should always use memcpy/memcmp to access the
>>>>> contents of
>>>>> RPC opaque data types. This permits us to continue to define these
>>>>> as
>>>>> simple character arrays, as most legacy RPC code does, and to
>>>>> rely on
>>>>> gcc to pack the fields in in-core structures optimally without
>>>>> breaking
>>>>> on platforms that require strict alignment.
>>>>
>>>> OK, I'm confused. What structures will get packed differently?
>>>
>>> Any struct that has an nsm_private embedded in it, such as struct
>>> nlm_reboot.
>>
>> I don't see how that or any structure is changed by this patch.
>
> It's not. Note the phrase above in the description: "permits us to
> _continue_ to define these" -- meaning, I'm not changing the structures.
Err, but that's not right either, is it?: We don't need to apply this
patch in order to continue to define the structures as they're currently
defined.
Help! I'm confused!
> It has been suggested that we use a union or __aligned attribute for RPC
> opaques. The problem with that is that it would cause structs like
> nlm_reboot to balloon in size; char x[] is aligned on bytes, but a union
> of u8, u32, and u64 would be aligned on a double-word boundary. That
> would make such structures larger.
OK, I agree, so let's not do that.
--b.
> Legacy RPC code, and any code generated by rpcgen, generally defines an
> opaque as a character array. So again, using a union would be
> inconsistent with other uses of RPC opaques.
>
> The point is we want to define and access RPC opaque's consistently,
> using memset() and memcpy(), since these are nothing more than byte
> arrays. The code in nsm_init_private() was an exception to this, for no
> reason. We don't really need special alignment macros in there, as long
> as we access the RPC opaque field with the byte sets and copies.
>
> Would it help if I described this patch as a "clean up" ?
On Apr 28, 2009, at 5:36 PM, J. Bruce Fields wrote:
> On Tue, Apr 28, 2009 at 01:24:24PM -0400, Chuck Lever wrote:
>> On Apr 28, 2009, at 12:40 PM, J. Bruce Fields wrote:
>>> On Tue, Apr 28, 2009 at 12:35:50PM -0400, Chuck Lever wrote:
>>>> On Apr 28, 2009, at 12:31 PM, J. Bruce Fields wrote:
>>>>> On Thu, Apr 23, 2009 at 07:33:40PM -0400, Chuck Lever wrote:
>>>>>> Recently, commit ad5b365c fixed a data type alignment issue in
>>>>>> nsm_init_private() by introducing put_unaligned(). We don't
>>>>>> actually
>>>>>> _require_ an unaligned access to nsm_private here.
>>>>>>
>>>>>> Instead, we should always use memcpy/memcmp to access the
>>>>>> contents of
>>>>>> RPC opaque data types. This permits us to continue to define
>>>>>> these
>>>>>> as
>>>>>> simple character arrays, as most legacy RPC code does, and to
>>>>>> rely on
>>>>>> gcc to pack the fields in in-core structures optimally without
>>>>>> breaking
>>>>>> on platforms that require strict alignment.
>>>>>
>>>>> OK, I'm confused. What structures will get packed differently?
>>>>
>>>> Any struct that has an nsm_private embedded in it, such as struct
>>>> nlm_reboot.
>>>
>>> I don't see how that or any structure is changed by this patch.
>>
>> It's not. Note the phrase above in the description: "permits us to
>> _continue_ to define these" -- meaning, I'm not changing the
>> structures.
>
> Err, but that's not right either, is it?: We don't need to apply this
> patch in order to continue to define the structures as they're
> currently
> defined.
>
> Help! I'm confused!
This patch is simply a clean up. We don't need to use put_unaligned
in nsm_init_private. There is absolutely nothing special about the
nsm_private data type that would require this. It should be accessed
and modified the way all other RPC opaques are, via memset/memcpy.
Take a look at http://thread.gmane.org/gmane.linux.nfs/25607 and
commit ad5b365c.
The controversy is over how to define opaques so they are accessible
on both 32- and 64-bit hardware platforms. My first pass at
nsm_init_private worked on 32-bit systems, but broke on 64-bit
systems. An expedient fix for this was to add the put_unaligned in
there so 64-bit systems could access the field no matter how it was
aligned. I argue this is unneeded complexity, and inconsistent with
the way most other RPC opaques are treated in the kernel.
Andrew Morton proposed making RPC opaques a union of u8, u32 (or
__be32), and u64 -- the u8 would allow us to treat an opaque as a byte
array when needed, the u32 would allow access via XDR quads, and the
u64 would force 64-bit alignment. The issues with this are:
1. Defined this way, opaque fields in data structures will force the
encompassing structures to be large enough to honor the alignment
requirements of the fields, and
2. Most other RPC opaques are already defined as character arrays, so
we would have to visit all of them to see if there were issues.
If we insist on accessing opaques only via memset() and memcpy()
problem 1 goes away and we remain compatible with the traditional
definition of an RPC opaque as an array of bytes, on both 64- and 32-
bit systems.
The description for this patch can be rewritten this way:
"Clean up: There is nothing special about the nsm_private data type
that requires the use of put_unaligned() to access it. Rewrite
nsm_init_private so it accesses nsm_private the way other code
accesses other RPC opaques.
See kernel bugzilla 12995."
>> It has been suggested that we use a union or __aligned attribute
>> for RPC
>> opaques. The problem with that is that it would cause structs like
>> nlm_reboot to balloon in size; char x[] is aligned on bytes, but a
>> union
>> of u8, u32, and u64 would be aligned on a double-word boundary. That
>> would make such structures larger.
>
> OK, I agree, so let's not do that.
>
>
> --b.
>
>> Legacy RPC code, and any code generated by rpcgen, generally
>> defines an
>> opaque as a character array. So again, using a union would be
>> inconsistent with other uses of RPC opaques.
>>
>> The point is we want to define and access RPC opaque's consistently,
>> using memset() and memcpy(), since these are nothing more than byte
>> arrays. The code in nsm_init_private() was an exception to this,
>> for no
>> reason. We don't really need special alignment macros in there, as
>> long
>> as we access the RPC opaque field with the byte sets and copies.
>>
>> Would it help if I described this patch as a "clean up" ?
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs"
> in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
On Tue, Apr 28, 2009 at 06:11:06PM -0400, Chuck Lever wrote:
> This patch is simply a clean up. We don't need to use put_unaligned in
> nsm_init_private. There is absolutely nothing special about the
> nsm_private data type that would require this. It should be accessed
> and modified the way all other RPC opaques are, via memset/memcpy.
>
> Take a look at http://thread.gmane.org/gmane.linux.nfs/25607 and commit
> ad5b365c.
>
> The controversy is over how to define opaques so they are accessible on
> both 32- and 64-bit hardware platforms. My first pass at
> nsm_init_private worked on 32-bit systems, but broke on 64-bit systems.
> An expedient fix for this was to add the put_unaligned in there so 64-bit
> systems could access the field no matter how it was aligned. I argue
> this is unneeded complexity, and inconsistent with the way most other RPC
> opaques are treated in the kernel.
>
> Andrew Morton proposed making RPC opaques a union of u8, u32 (or
> __be32), and u64 -- the u8 would allow us to treat an opaque as a byte
> array when needed, the u32 would allow access via XDR quads, and the u64
> would force 64-bit alignment. The issues with this are:
>
> 1. Defined this way, opaque fields in data structures will force the
> encompassing structures to be large enough to honor the alignment
> requirements of the fields, and
>
> 2. Most other RPC opaques are already defined as character arrays, so
> we would have to visit all of them to see if there were issues.
>
> If we insist on accessing opaques only via memset() and memcpy() problem
> 1 goes away and we remain compatible with the traditional definition of
> an RPC opaque as an array of bytes, on both 64- and 32-bit systems.
>
> The description for this patch can be rewritten this way:
>
> "Clean up: There is nothing special about the nsm_private data type that
> requires the use of put_unaligned() to access it. Rewrite
> nsm_init_private so it accesses nsm_private the way other code accesses
> other RPC opaques.
>
> See kernel bugzilla 12995."
OK.--b.
Chuck Lever <[email protected]> writes:
> On Apr 28, 2009, at 5:36 PM, J. Bruce Fields wrote:
>
>> On Tue, Apr 28, 2009 at 01:24:24PM -0400, Chuck Lever wrote:
>>> On Apr 28, 2009, at 12:40 PM, J. Bruce Fields wrote:
>>>> On Tue, Apr 28, 2009 at 12:35:50PM -0400, Chuck Lever wrote:
>>>>> On Apr 28, 2009, at 12:31 PM, J. Bruce Fields wrote:
>>>>>> On Thu, Apr 23, 2009 at 07:33:40PM -0400, Chuck Lever wrote:
>>>>>>> Recently, commit ad5b365c fixed a data type alignment issue in
>>>>>>> nsm_init_private() by introducing put_unaligned(). We don't
>>>>>>> actually _require_ an unaligned access to nsm_private here.
>>>>>>>
>>>>>>> Instead, we should always use memcpy/memcmp to access the
>>>>>>> contents of RPC opaque data types. This permits us to
>>>>>>> continue to define these as simple character arrays, as most
>>>>>>> legacy RPC code does, and to rely on gcc to pack the fields in
>>>>>>> in-core structures optimally without breaking on platforms
>>>>>>> that require strict alignment.
>>>>>>
>>>>>> OK, I'm confused. What structures will get packed differently?
>>>>>
>>>>> Any struct that has an nsm_private embedded in it, such as struct
>>>>> nlm_reboot.
>>>>
>>>> I don't see how that or any structure is changed by this patch.
>>>
>>> It's not. Note the phrase above in the description: "permits us to
>>> _continue_ to define these" -- meaning, I'm not changing the
>>> structures.
>>
>> Err, but that's not right either, is it?: We don't need to apply th=
is
>> patch in order to continue to define the structures as they're
>> currently
>> defined.
>>
>> Help! I'm confused!
>
> This patch is simply a clean up. We don't need to use put_unaligned
> in nsm_init_private. There is absolutely nothing special about the
> nsm_private data type that would require this. It should be accessed
The "special" thing is has not guaranteed alignment. Hence, any
access to it must use unaligned-safe methods.
> and modified the way all other RPC opaques are, via memset/memcpy.
What is so special about put_unaligned() that you insist on replacing i=
t?
> Take a look at http://thread.gmane.org/gmane.linux.nfs/25607 and
> commit ad5b365c.
>
> The controversy is over how to define opaques so they are accessible
> on both 32- and 64-bit hardware platforms. My first pass at
> nsm_init_private worked on 32-bit systems, but broke on 64-bit
> systems. An expedient fix for this was to add the put_unaligned in
> there so 64-bit systems could access the field no matter how it was
> aligned. I argue this is unneeded complexity, and inconsistent with
> the way most other RPC opaques are treated in the kernel.
>
> Andrew Morton proposed making RPC opaques a union of u8, u32 (or
> __be32), and u64 -- the u8 would allow us to treat an opaque as a byt=
e
> array when needed, the u32 would allow access via XDR quads, and the
> u64 would force 64-bit alignment. The issues with this are:
>
> 1. Defined this way, opaque fields in data structures will force the
> encompassing structures to be large enough to honor the alignment
> requirements of the fields, and
>
> 2. Most other RPC opaques are already defined as character arrays, s=
o
> we would have to visit all of them to see if there were issues.
>
> If we insist on accessing opaques only via memset() and memcpy()
> problem 1 goes away and we remain compatible with the traditional
> definition of an RPC opaque as an array of bytes, on both 64- and 32-
> bit systems.
I still don't see what problem put_unaligned() poses. Think of it as
a more efficient memcpy(). We don't want the code to be larger and
slower than necessary, do we?
--=20
M=E5ns Rullg=E5rd
[email protected]