Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1231333 > unrolled thread
| Started by | Anton Altaparmakov <anton@tuxera.com> |
|---|---|
| First post | 2015-09-23 12:40 +0200 |
| Last post | 2015-09-24 11:30 +0200 |
| Articles | 4 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH 22/31] fs/ntfs: use kmemdup rather than duplicating its implementation Anton Altaparmakov <anton@tuxera.com> - 2015-09-23 12:40 +0200
Re: [PATCH 22/31] fs/ntfs: use kmemdup rather than duplicating its implementation Andrzej Hajda <a.hajda@samsung.com> - 2015-09-24 10:40 +0200
Re: [PATCH 22/31] fs/ntfs: use kmemdup rather than duplicating its implementation Anton Altaparmakov <anton@tuxera.com> - 2015-09-24 11:30 +0200
Re: [PATCH 22/31] fs/ntfs: use kmemdup rather than duplicating its implementation Anton Altaparmakov <anton@tuxera.com> - 2015-09-24 11:30 +0200
| From | Anton Altaparmakov <anton@tuxera.com> |
|---|---|
| Date | 2015-09-23 12:40 +0200 |
| Subject | Re: [PATCH 22/31] fs/ntfs: use kmemdup rather than duplicating its implementation |
| Message-ID | <qbPCX-1wv-31@gated-at.bofh.it> |
Hi Andrzej,
Thanks for your patch. It looks fine though I don't quite see the point of it to be honest.
It actually adds an additional function call (kmemdup() is not inline) just to save 1 line of source code in the driver and I don't think it improves readability or anything so why bother? What does it gain?
Best regards,
Anton
> On 7 Aug 2015, at 08:59, Andrzej Hajda <a.hajda@samsung.com> wrote:
>
> The patch was generated using fixed coccinelle semantic patch
> scripts/coccinelle/api/memdup.cocci [1].
>
> [1]: http://permalink.gmane.org/gmane.linux.kernel/2014320
>
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
> fs/ntfs/dir.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ntfs/dir.c b/fs/ntfs/dir.c
> index 9e38daf..2b7fef0 100644
> --- a/fs/ntfs/dir.c
> +++ b/fs/ntfs/dir.c
> @@ -1172,14 +1172,13 @@ static int ntfs_readdir(struct file *file, struct dir_context *actor)
> * map the mft record without deadlocking.
> */
> rc = le32_to_cpu(ctx->attr->data.resident.value_length);
> - ir = kmalloc(rc, GFP_NOFS);
> + /* Copy the index root value (it has been verified in read_inode). */
> + ir = kmemdup((u8 *)ctx->attr + le16_to_cpu(ctx->attr->data.resident.value_offset),
> + rc, GFP_NOFS);
> if (unlikely(!ir)) {
> err = -ENOMEM;
> goto err_out;
> }
> - /* Copy the index root value (it has been verified in read_inode). */
> - memcpy(ir, (u8*)ctx->attr +
> - le16_to_cpu(ctx->attr->data.resident.value_offset), rc);
> ntfs_attr_put_search_ctx(ctx);
> unmap_mft_record(ndir);
> ctx = NULL;
> --
> 1.9.1
--
Anton Altaparmakov <anton at tuxera.com> (replace at with @)
Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
Linux NTFS maintainer
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Andrzej Hajda <a.hajda@samsung.com> |
|---|---|
| Date | 2015-09-24 10:40 +0200 |
| Subject | Re: [PATCH 22/31] fs/ntfs: use kmemdup rather than duplicating its implementation |
| Message-ID | <qcael-6bA-7@gated-at.bofh.it> |
| In reply to | #1231333 |
On 09/23/2015 12:21 PM, Anton Altaparmakov wrote:
> Hi Andrzej,
>
> Thanks for your patch. It looks fine though I don't quite see the point of it to be honest.
>
> It actually adds an additional function call (kmemdup() is not inline) just to save 1 line of source code in the driver and I don't think it improves readability or anything so why bother? What does it gain?
kmemdup replaces combo (kmalloc + memdup) with one call.
The patch follows quite common practice of abstracting out common patterns.
Regards
Andrzej
>
> Best regards,
>
> Anton
>
>> On 7 Aug 2015, at 08:59, Andrzej Hajda <a.hajda@samsung.com> wrote:
>>
>> The patch was generated using fixed coccinelle semantic patch
>> scripts/coccinelle/api/memdup.cocci [1].
>>
>> [1]: http://permalink.gmane.org/gmane.linux.kernel/2014320
>>
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>> ---
>> fs/ntfs/dir.c | 7 +++----
>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/ntfs/dir.c b/fs/ntfs/dir.c
>> index 9e38daf..2b7fef0 100644
>> --- a/fs/ntfs/dir.c
>> +++ b/fs/ntfs/dir.c
>> @@ -1172,14 +1172,13 @@ static int ntfs_readdir(struct file *file, struct dir_context *actor)
>> * map the mft record without deadlocking.
>> */
>> rc = le32_to_cpu(ctx->attr->data.resident.value_length);
>> - ir = kmalloc(rc, GFP_NOFS);
>> + /* Copy the index root value (it has been verified in read_inode). */
>> + ir = kmemdup((u8 *)ctx->attr + le16_to_cpu(ctx->attr->data.resident.value_offset),
>> + rc, GFP_NOFS);
>> if (unlikely(!ir)) {
>> err = -ENOMEM;
>> goto err_out;
>> }
>> - /* Copy the index root value (it has been verified in read_inode). */
>> - memcpy(ir, (u8*)ctx->attr +
>> - le16_to_cpu(ctx->attr->data.resident.value_offset), rc);
>> ntfs_attr_put_search_ctx(ctx);
>> unmap_mft_record(ndir);
>> ctx = NULL;
>> --
>> 1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Anton Altaparmakov <anton@tuxera.com> |
|---|---|
| Date | 2015-09-24 11:30 +0200 |
| Message-ID | <qcb0K-7l5-17@gated-at.bofh.it> |
| In reply to | #1231333 |
> On 24 Sep 2015, at 10:20, Anton Altaparmakov <anton@tuxera.com> wrote:
>
> Hi Andrzej,
>
>> On 24 Sep 2015, at 09:34, Andrzej Hajda <a.hajda@samsung.com> wrote:
>>
>> On 09/23/2015 12:21 PM, Anton Altaparmakov wrote:
>>> Hi Andrzej,
>>>
>>> Thanks for your patch. It looks fine though I don't quite see the point of it to be honest.
>>>
>>> It actually adds an additional function call (kmemdup() is not inline) just to save 1 line of source code in the driver and I don't think it improves readability or anything so why bother? What does it gain?
>>
>> kmemdup replaces combo (kmalloc + memdup) with one call.
>> The patch follows quite common practice of abstracting out common patterns.
>
> Sure it does I am just questioning the sanity of the practice... (-;
>
> Such changes reduce the size of the kernel binary by a few bytes at the cost of adding CPU cycles to the execution time. How is that good thing? Unless you are on an embedded system desperate for RAM throwing away CPU cycles on pointless abstractions makes no sense to me...
>
> But as I said patch is fine. Feel free to send it onto Andrew to get it into mainline. You can add my Acked-by: Anton Altaparmakoc <anton@tuxera.com>
Altaparmakov even. Can't even spell my own surname... )-;
> line to it when sending it. I am just saying that I think patches like that don't make much sense to me...
>
> Best regards,
>
> Anton
>
>> Regards
>> Andrzej
>>
>>>
>>> Best regards,
>>>
>>> Anton
>>>
>>>> On 7 Aug 2015, at 08:59, Andrzej Hajda <a.hajda@samsung.com> wrote:
>>>>
>>>> The patch was generated using fixed coccinelle semantic patch
>>>> scripts/coccinelle/api/memdup.cocci [1].
>>>>
>>>> [1]: http://permalink.gmane.org/gmane.linux.kernel/2014320
>>>>
>>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>>> ---
>>>> fs/ntfs/dir.c | 7 +++----
>>>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/fs/ntfs/dir.c b/fs/ntfs/dir.c
>>>> index 9e38daf..2b7fef0 100644
>>>> --- a/fs/ntfs/dir.c
>>>> +++ b/fs/ntfs/dir.c
>>>> @@ -1172,14 +1172,13 @@ static int ntfs_readdir(struct file *file, struct dir_context *actor)
>>>> * map the mft record without deadlocking.
>>>> */
>>>> rc = le32_to_cpu(ctx->attr->data.resident.value_length);
>>>> - ir = kmalloc(rc, GFP_NOFS);
>>>> + /* Copy the index root value (it has been verified in read_inode). */
>>>> + ir = kmemdup((u8 *)ctx->attr + le16_to_cpu(ctx->attr->data.resident.value_offset),
>>>> + rc, GFP_NOFS);
>>>> if (unlikely(!ir)) {
>>>> err = -ENOMEM;
>>>> goto err_out;
>>>> }
>>>> - /* Copy the index root value (it has been verified in read_inode). */
>>>> - memcpy(ir, (u8*)ctx->attr +
>>>> - le16_to_cpu(ctx->attr->data.resident.value_offset), rc);
>>>> ntfs_attr_put_search_ctx(ctx);
>>>> unmap_mft_record(ndir);
>>>> ctx = NULL;
>>>> --
>>>> 1.9.1
>
> --
> Anton Altaparmakov <anton at tuxera.com> (replace at with @)
> Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
> Linux NTFS maintainer
Best regards,
Anton
--
Anton Altaparmakov <anton at tuxera.com> (replace at with @)
Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
Linux NTFS maintainer
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Anton Altaparmakov <anton@tuxera.com> |
|---|---|
| Date | 2015-09-24 11:30 +0200 |
| Message-ID | <qcb0K-7l5-21@gated-at.bofh.it> |
| In reply to | #1231333 |
Hi Andrzej,
> On 24 Sep 2015, at 09:34, Andrzej Hajda <a.hajda@samsung.com> wrote:
>
> On 09/23/2015 12:21 PM, Anton Altaparmakov wrote:
>> Hi Andrzej,
>>
>> Thanks for your patch. It looks fine though I don't quite see the point of it to be honest.
>>
>> It actually adds an additional function call (kmemdup() is not inline) just to save 1 line of source code in the driver and I don't think it improves readability or anything so why bother? What does it gain?
>
> kmemdup replaces combo (kmalloc + memdup) with one call.
> The patch follows quite common practice of abstracting out common patterns.
Sure it does I am just questioning the sanity of the practice... (-;
Such changes reduce the size of the kernel binary by a few bytes at the cost of adding CPU cycles to the execution time. How is that good thing? Unless you are on an embedded system desperate for RAM throwing away CPU cycles on pointless abstractions makes no sense to me...
But as I said patch is fine. Feel free to send it onto Andrew to get it into mainline. You can add my Acked-by: Anton Altaparmakoc <anton@tuxera.com> line to it when sending it. I am just saying that I think patches like that don't make much sense to me...
Best regards,
Anton
> Regards
> Andrzej
>
>>
>> Best regards,
>>
>> Anton
>>
>>> On 7 Aug 2015, at 08:59, Andrzej Hajda <a.hajda@samsung.com> wrote:
>>>
>>> The patch was generated using fixed coccinelle semantic patch
>>> scripts/coccinelle/api/memdup.cocci [1].
>>>
>>> [1]: http://permalink.gmane.org/gmane.linux.kernel/2014320
>>>
>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>> ---
>>> fs/ntfs/dir.c | 7 +++----
>>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/fs/ntfs/dir.c b/fs/ntfs/dir.c
>>> index 9e38daf..2b7fef0 100644
>>> --- a/fs/ntfs/dir.c
>>> +++ b/fs/ntfs/dir.c
>>> @@ -1172,14 +1172,13 @@ static int ntfs_readdir(struct file *file, struct dir_context *actor)
>>> * map the mft record without deadlocking.
>>> */
>>> rc = le32_to_cpu(ctx->attr->data.resident.value_length);
>>> - ir = kmalloc(rc, GFP_NOFS);
>>> + /* Copy the index root value (it has been verified in read_inode). */
>>> + ir = kmemdup((u8 *)ctx->attr + le16_to_cpu(ctx->attr->data.resident.value_offset),
>>> + rc, GFP_NOFS);
>>> if (unlikely(!ir)) {
>>> err = -ENOMEM;
>>> goto err_out;
>>> }
>>> - /* Copy the index root value (it has been verified in read_inode). */
>>> - memcpy(ir, (u8*)ctx->attr +
>>> - le16_to_cpu(ctx->attr->data.resident.value_offset), rc);
>>> ntfs_attr_put_search_ctx(ctx);
>>> unmap_mft_record(ndir);
>>> ctx = NULL;
>>> --
>>> 1.9.1
--
Anton Altaparmakov <anton at tuxera.com> (replace at with @)
Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
Linux NTFS maintainer
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web