Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1212189 > unrolled thread

Re: [PATCH] mm: mmap: Check all failures before set values

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.

Contents

  Re: [PATCH] mm: mmap: Check all failures before set values Chen Gang <xili_gchen_5257@hotmail.com> - 2015-08-24 15:40 +0200
    Re: [PATCH] mm: mmap: Check all failures before set values Michal Hocko <mhocko@kernel.org> - 2015-08-24 16:00 +0200
      Re: [PATCH] mm: mmap: Check all failures before set values Michal Hocko <mhocko@kernel.org> - 2015-08-25 13:40 +0200
      Re: [PATCH] mm: mmap: Check all failures before set values Chen Gang <xili_gchen_5257@hotmail.com> - 2015-08-25 23:40 +0200

#1212189 — Re: [PATCH] mm: mmap: Check all failures before set values

T24gOC8yNC8xNSAxOTozMiwgTWljaGFsIEhvY2tvIHdyb3RlOgo+IE9uIE1vbiAyNC0wOC0xNSAw
MDo1OTozOSwgZ2FuZy5jaGVuLjVpNWpAcXEuY29tIHdyb3RlOgo+Pj4gRnJvbTogQ2hlbiBHYW5n
IDxnYW5nLmNoZW4uNWk1akBnbWFpbC5jb20+Cj4+Pgo+Pj4gV2hlbiBmYWlsdXJlIG9jY3VycyBh
bmQgcmV0dXJuLCB2bWEtPnZtX3Bnb2ZmIGlzIGFscmVhZHkgc2V0LCB3aGljaCBpcwo+Pj4gbm90
IGEgZ29vZCBpZGVhLgo+IFdoeT8gVGhlIHZtYSBpcyBub3QgaW5zZXJ0ZWQgYW55d2hlcmUgYW5k
IHRoZSBmYWlsdXJlIHBhdGggaXMgc3VwcG9zZWQKPiB0byBzaW1wbHkgZnJlZSB0aGUgdm1hLgo+
CgpJdCBjYW4gc2F2ZSBzZXZlcmFsIGluc25zIHdoZW4gZmFpbHVyZSBvY2N1cnMuCgpJdCBpcyBh
bHdheXMgYSBsaXR0bGUgYmV0dGVyIHRvIGxldCB0aGUgZXh0ZXJuYWwgZnVuY3Rpb24gc3VwcG9z
ZSBmZXdlcgpjYWxsZXJzJyBiZWhhbGYuCgpJdCBjYW4gc2F2ZSB0aGUgY29kZSByZWFkZXJzJyAo
ZXNwZWNpYWxseSBuZXcgcmVhZGVycycpIHRpbWUgcmVzb3VyY2UKdG8gYXZvaWQgdG8gYW5hbHl6
ZSB3aHkgc2V0ICd2bWEtPnZtX3Bnb2ZmJyBiZWZvcmUgY2hlY2tpbmcgJy1FTk9NRU0nCihtYXkg
aXQgY2F1c2UgaXNzdWU/IG9yIGlzICd2bV9wZ29mZicgcmVsYXRlZCB3aXRoIHRoZSBuZXh0IGNo
ZWNraW5nPykuCgoKVGhhbmtzLgotLQpDaGVuIEdhbmcKCk9wZW4sIHNoYXJlLCBhbmQgYXR0aXR1
ZGUgbGlrZSBhaXIsIHdhdGVyLCBhbmQgbGlmZSB3aGljaCBHb2QgYmxlc3NlZAogCQkgCSAgIAkJ
ICA=
--
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]

#1212209

On Mon 24-08-15 21:34:25, Chen Gang wrote:
> On 8/24/15 19:32, Michal Hocko wrote:
> > On Mon 24-08-15 00:59:39, gang.chen.5i5j@qq.com wrote:
> >>> From: Chen Gang <gang.chen.5i5j@gmail.com>
> >>>
> >>> When failure occurs and return, vma->vm_pgoff is already set, which is
> >>> not a good idea.
> > Why? The vma is not inserted anywhere and the failure path is supposed
> > to simply free the vma.
> >
> 
> It can save several insns when failure occurs.

The failure is quite unlikely, though.

> It is always a little better to let the external function suppose fewer
> callers' behalf.

I am sorry but I do not understand what you are saying here.

> It can save the code readers' (especially new readers') time resource
> to avoid to analyze why set 'vma->vm_pgoff' before checking '-ENOMEM'
> (may it cause issue? or is 'vm_pgoff' related with the next checking?).

Then your changelog should be specific about these reasons. "not a good
idea" is definitely not a good justification for a patch. I am not
saying the patch is incorrect I just do not sure it is worth it. The
code is marginally better. But others might think otherwise. The
changelog needs some more work for sure.
-- 
Michal Hocko
SUSE Labs
--
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]

#1212977

On Tue 25-08-15 05:54:00, Chen Gang wrote:
> On 8/24/15 21:57, Michal Hocko wrote:
> > On Mon 24-08-15 21:34:25, Chen Gang wrote:
> 
> [...]
> 
> 
> >> It is always a little better to let the external function suppose fewer
> >> callers' behalf.
> > 
> > I am sorry but I do not understand what you are saying here.
> > 
> 
> Execuse me, my English maybe be still not quite well, my meaning is:
> 
>  - For the external functions (e.g. insert_vm_struct in our case), as a
>    callee, it may have to supose something from the caller.
> 
>  - If we can keep callee's functional contents no touch, a little fewer
>    supposing will let callee a little more independent from caller.
> 
>  - If can keep functional contens no touch, the lower dependency between
>    caller and callee is always better.

OK, I guess I understand what you mean. You are certainly right that a
partial initialization for the failure case is not nice in general. I
was just objecting that the callers are supposed to free the vma in
the failure case so any partial initialization doesn't matter in this
particular case.

Your patch would be more sensible if the failure case was more
likely. But this function is used for special mappings (vdso, temporary
vdso stack) which are created early in the process life time so both
failure paths are highly unlikely. If this was a part of a larger
changes where the function would be used elsewhere I wouldn't object at
all.

The reason I am skeptical about such changes in general is that
the effect is very marginal while it increases chances of the code
conflicts.

But as I've said, if others feel this is worthwhile I will not object.

-- 
Michal Hocko
SUSE Labs
--
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]

#1213375

T24gOC8yNS8xNSAxOTozNSwgTWljaGFsIEhvY2tvIHdyb3RlOgo+Cj4gT0ssIEkgZ3Vlc3MgSSB1
bmRlcnN0YW5kIHdoYXQgeW91IG1lYW4uIFlvdSBhcmUgY2VydGFpbmx5IHJpZ2h0IHRoYXQgYQo+
IHBhcnRpYWwgaW5pdGlhbGl6YXRpb24gZm9yIHRoZSBmYWlsdXJlIGNhc2UgaXMgbm90IG5pY2Ug
aW4gZ2VuZXJhbC4gSQo+IHdhcyBqdXN0IG9iamVjdGluZyB0aGF0IHRoZSBjYWxsZXJzIGFyZSBz
dXBwb3NlZCB0byBmcmVlIHRoZSB2bWEgaW4KPiB0aGUgZmFpbHVyZSBjYXNlIHNvIGFueSBwYXJ0
aWFsIGluaXRpYWxpemF0aW9uIGRvZXNuJ3QgbWF0dGVyIGluIHRoaXMKPiBwYXJ0aWN1bGFyIGNh
c2UuCj4KPiBZb3VyIHBhdGNoIHdvdWxkIGJlIG1vcmUgc2Vuc2libGUgaWYgdGhlIGZhaWx1cmUg
Y2FzZSB3YXMgbW9yZQo+IGxpa2VseS4gQnV0IHRoaXMgZnVuY3Rpb24gaXMgdXNlZCBmb3Igc3Bl
Y2lhbCBtYXBwaW5ncyAodmRzbywgdGVtcG9yYXJ5Cj4gdmRzbyBzdGFjaykgd2hpY2ggYXJlIGNy
ZWF0ZWQgZWFybHkgaW4gdGhlIHByb2Nlc3MgbGlmZSB0aW1lIHNvIGJvdGgKPiBmYWlsdXJlIHBh
dGhzIGFyZSBoaWdobHkgdW5saWtlbHkuIElmIHRoaXMgd2FzIGEgcGFydCBvZiBhIGxhcmdlcgo+
IGNoYW5nZXMgd2hlcmUgdGhlIGZ1bmN0aW9uIHdvdWxkIGJlIHVzZWQgZWxzZXdoZXJlIEkgd291
bGRuJ3Qgb2JqZWN0IGF0Cj4gYWxsLgo+CgpPSy4KCj4gVGhlIHJlYXNvbiBJIGFtIHNrZXB0aWNh
bCBhYm91dCBzdWNoIGNoYW5nZXMgaW4gZ2VuZXJhbCBpcyB0aGF0Cj4gdGhlIGVmZmVjdCBpcyB2
ZXJ5IG1hcmdpbmFsIHdoaWxlIGl0IGluY3JlYXNlcyBjaGFuY2VzIG9mIHRoZSBjb2RlCj4gY29u
ZmxpY3RzLgo+Cj4gQnV0IGFzIEkndmUgc2FpZCwgaWYgb3RoZXJzIGZlZWwgdGhpcyBpcyB3b3J0
aHdoaWxlIEkgd2lsbCBub3Qgb2JqZWN0Lgo+CgpPSywgSSBjYW4gdW5kZXJzdGFuZC4KCgpUaGFu
a3MuCi0tCkNoZW4gR2FuZwoKT3Blbiwgc2hhcmUsIGFuZCBhdHRpdHVkZSBsaWtlIGFpciwgd2F0
ZXIsIGFuZCBsaWZlIHdoaWNoIEdvZCBibGVzc2VkCiAJCSAJICAgCQkgIA==
--
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