Re: [PATCH v2 2/2] staging: ion: chunk_heap: use %pad for printing dma_addr_t's
On Mon, May 4, 2015 at 1:22 AM, Dan Carpenter wrote:
> On Thu, Apr 09, 2015 at 06:10:04PM -0700, Mitchel Humpherys wrote:
>> We're currently using %lu and %ld to print some variables of type
>> dma_addr_t, which results in the following warning when dma_addr_t is
>> 64-bits wide:
>>
>> drivers/staging/android/ion/ion_chunk_heap.c: In function
>> 'ion_chunk_heap_create':
>> drivers/staging/android/ion/ion_chunk_heap.c:176:2: warning: format
>> '%lu' expects argument of type 'long unsigned int', but argument 3 has type
>> 'dma_addr_t' [-Wformat=]
>> pr_info("%s: base %lu size %zu align %ld\n", __func__,
>> chunk_heap->base,
>> ^
>> drivers/staging/android/ion/ion_chunk_heap.c:176:2: warning: format
>> '%ld' expects argument of type 'long int', but argument 5 has type
>> 'dma_addr_t' [-Wformat=]
>>
>> Fix this by using %pad as instructed in printk-formats.txt.
>>
>> Signed-off-by: Mitchel Humpherys
>
> This one was just merged and I was about to email you that it introduces
> some new Smatch warnings, but actually looking at it, it's just wrong.
>
> We want to print "chunk_heap->base" and not "&chunk_heap->base".
This would be correct if base was a dma_addr_t...
> And anyway "&chunk_heap->base" is a regular pointer, not a dma_addr_t.
But it is actually an ion_phys_addr_t, which is currently typedef'd to
unsigned long. Are you using a local patch that replaces
ion_phys_addr_t with dma_addr_t?
> So please send a new patch that removes the &.
Removing the & is not correct, lib/vsprintf.c will dereference the arg
for %pad or %pap. I think this patch should just be dropped, the old
%lu was correct for what is in Linus' tree.
> regards,
> dan carpenter
>
>> ---
>> drivers/staging/android/ion/ion_chunk_heap.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/android/ion/ion_chunk_heap.c
>> b/drivers/staging/android/ion/ion_chunk_heap.c
>> index 54746157d799..6b3e18aa1c64 100644
>> --- a/drivers/staging/android/ion/ion_chunk_heap.c
>> +++ b/drivers/staging/android/ion/ion_chunk_heap.c
>> @@ -173,8 +173,8 @@ struct ion_heap *ion_chunk_heap_create(struct
>> ion_platform_heap *heap_data)
>> chunk_heap->heap.ops = &chunk_heap_ops;
>> chunk_heap->heap.type = ION_HEAP_TYPE_CHUNK;
>> chunk_heap->heap.flags = ION_HEAP_FLAG_DEFER_FREE;
>> - pr_debug("%s: base %lu size %zu align %ld\n", __func__,
>> chunk_heap->base,
>> - heap_data->size, heap_data->align);
>> + pr_debug("%s: base %pad size %zu align %pad\n", __func__,
>> + &chunk_heap->base, heap_data->size, &heap_data->align);
>>
>> return &chunk_heap->heap;
>>
>> --
>> Qualcomm Innovation Center, Inc.
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>
>> ___
>> devel mailing list
>> de...@linuxdriverproject.org
>> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to kernel-team+unsubscr...@android.com.
+
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging : android : uapi : fix coding style
(resending without the html)
On Wed, Apr 16, 2014 at 9:36 AM, John Stultz wrote:
>
> On 04/16/2014 07:39 AM, Joe Perches wrote:
> > On Wed, 2014-04-16 at 23:27 +0900, Seunghun Lee wrote:
> >> This patch fix checkpatch.pl warnings and errors.
> > []
> >> diff --git a/drivers/staging/android/uapi/binder.h
> >> b/drivers/staging/android/uapi/binder.h
> > []
> >> @@ -169,7 +169,7 @@ struct binder_ptr_cookie {
> >> struct binder_handle_cookie {
> >> __u32 handle;
> >> binder_uintptr_t cookie;
> >> -} __attribute__((packed));
> >> +} __packed;
> > If this .h file is meant to be a user-space #include,
> > then it should not use the kernel specific __packed
> > but keep the __attribute__((packed))
>
> Agreed.
>
> > It does use __u32 though and that's generally
> > kernel specific.
>
> Hmm. Theres a ton of __u32 usage in include/uapi/* as well as typedefs
> for it too.
> include/uapi/asm-generic/int-l64.h:typedef unsigned int __u32;
> include/uapi/asm-generic/int-ll64.h:typedef unsigned int __u32;
>
> > John? Does any of these binder uapi files need a
> > bit more sorting out?
> I suspect this is ok, but Cc'ing Colin to give him a heads up, as it
> would probably cause trouble w/ their libc headers first.
__u32 is explicitly for use in userspace headers so it doesn't
conflict with userspace definitions of u32, uint32_t, etc. It gets
set through #include when a copy of include/uapi is in
the include path.
>From Documentation/CodingStyle:
(e) Types safe for use in userspace.
In certain structures which are visible to userspace, we cannot
require C99 types and cannot use the 'u32' form above. Thus, we
use __u32 and similar types in all structures which are shared
with userspace.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging/android: Remove ram_console.h
On Tue, Apr 22, 2014 at 2:22 AM, Bintian Wang wrote:
> ram_console is replaced by pstore and pstore_ram drivers,
> and there is no code to use this head file, so remove it.
>
> Signed-off-by: Bintian Wang
> ---
> drivers/staging/android/ram_console.h | 22 --
> 1 file changed, 22 deletions(-)
> delete mode 100644 drivers/staging/android/ram_console.h
>
> diff --git a/drivers/staging/android/ram_console.h
> b/drivers/staging/android/ram_console.h
> deleted file mode 100644
> index 9f1125c..000
> --- a/drivers/staging/android/ram_console.h
> +++ /dev/null
> @@ -1,22 +0,0 @@
> -/*
> - * Copyright (C) 2010 Google, Inc.
> - *
> - * This software is licensed under the terms of the GNU General Public
> - * License version 2, as published by the Free Software Foundation, and
> - * may be copied, distributed, and modified under those terms.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> - * GNU General Public License for more details.
> - *
> - */
> -
> -#ifndef _INCLUDE_LINUX_PLATFORM_DATA_RAM_CONSOLE_H_
> -#define _INCLUDE_LINUX_PLATFORM_DATA_RAM_CONSOLE_H_
> -
> -struct ram_console_platform_data {
> - const char *bootinfo;
> -};
> -
> -#endif /* _INCLUDE_LINUX_PLATFORM_DATA_RAM_CONSOLE_H_ */
> --
> 1.7.9.5
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
Acked-by: Colin Cross
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Wed, Dec 4, 2013 at 10:35 AM, Greg KH wrote: > And finally, is this all really needed? Why not just fix the structures > to be "correct", and then fix userspace to use the correct structures as > well, thereby not needing a compat layer at all? Some of the binder ioctls take userspace pointers. Are you suggesting storing those pointers in a __u64 to avoid having to have a compat_ioctl? ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Wed, Dec 4, 2013 at 1:43 PM, Greg KH wrote: > On Wed, Dec 04, 2013 at 12:46:42PM -0800, Colin Cross wrote: >> On Wed, Dec 4, 2013 at 10:35 AM, Greg KH wrote: >> >> >> > And finally, is this all really needed? Why not just fix the structures >> > to be "correct", and then fix userspace to use the correct structures as >> > well, thereby not needing a compat layer at all? >> >> Some of the binder ioctls take userspace pointers. Are you suggesting >> storing those pointers in a __u64 to avoid having to have a >> compat_ioctl? > > Yes, that's the best way to solve the issue, right? It's the least code, but in exchange you lose all the type safety and warnings when copying in and out of the pointers, as well as sparse checking on the __user attribute. That doesn't seem like a good tradeoff to me. In addition it requires modifying the existing heavily used 32 bit api, which means a mostly-equivalent compat layer added in libbinder to support old kernels. I would suggest fixing the 32-bit structures to use fixed sizes where appropriate (__u32 instead of unsigned long) while maintaining compatibility, and then using compat_ioctl where necessary to handle pointers. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Wed, Dec 4, 2013 at 2:02 PM, Greg KH wrote: > On Wed, Dec 04, 2013 at 01:55:34PM -0800, Colin Cross wrote: >> On Wed, Dec 4, 2013 at 1:43 PM, Greg KH wrote: >> > On Wed, Dec 04, 2013 at 12:46:42PM -0800, Colin Cross wrote: >> >> On Wed, Dec 4, 2013 at 10:35 AM, Greg KH >> >> wrote: >> >> >> >> >> >> > And finally, is this all really needed? Why not just fix the structures >> >> > to be "correct", and then fix userspace to use the correct structures as >> >> > well, thereby not needing a compat layer at all? >> >> >> >> Some of the binder ioctls take userspace pointers. Are you suggesting >> >> storing those pointers in a __u64 to avoid having to have a >> >> compat_ioctl? >> > >> > Yes, that's the best way to solve the issue, right? >> >> It's the least code, but in exchange you lose all the type safety and >> warnings when copying in and out of the pointers, as well as sparse >> checking on the __user attribute. > > Not if you make the cast right at the beginning, when you first "touch" > the data, but yes, it does take some of the type saftey away, at the > expense of simpler code to mess up :) > >> That doesn't seem like a good tradeoff to me. In addition it requires >> modifying the existing heavily used 32 bit api, which means a >> mostly-equivalent compat layer added in libbinder to support old >> kernels. > > Wait, I thought that libbinder would have to be changed anyway here, to > handle 64bit kernels (in both 32 and 64bit userspace). Since you are > already changing it, why not just "do it correctly"? libbinder will need changes to support 64-bit userspace and especially a mixed 64-bit and 32-bit userspace, but this patch series is only addressing a pure 32-bit userspace on a 64-bit kernel. Support for a 64-bit userspace in Android is obviously going to require a future version of Android including, among other things, libbinder changes. As far as I know, those changes won't need to change the ioctl api, only the layout of the buffers that are passed through the ioctl api. > Or does this patch series mean that no userspace code is changed? Is > that a "requirement" here? Since this series only addresses 32-bit userspace on 64-bit kernel support there are no associated userspace changes. Changing the 32-bit api here means that combining the KitKat branch from http://android.googlesource.com with a newer kernel version will not work. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Wed, Dec 4, 2013 at 3:21 PM, One Thousand Gnomes
wrote:
> On Wed, 4 Dec 2013 10:35:54 -0800
> Greg KH wrote:
>
>> On Wed, Dec 04, 2013 at 06:09:41PM +, Serban Constantinescu wrote:
>> > +#define size_helper(x) ({ \
>> > + size_t __size; \
>> > + if (!is_compat_task()) \
>> > + __size = sizeof(x); \
>> > + else if (sizeof(x) == sizeof(struct flat_binder_object))\
>> > + __size = sizeof(struct compat_flat_binder_object); \
>> > + else if (sizeof(x) == sizeof(struct binder_transaction_data)) \
>> > + __size = sizeof(struct compat_binder_transaction_data); \
>> > + else if (sizeof(x) == sizeof(size_t)) \
>> > + __size = sizeof(compat_size_t); \
>> > + else\
>> > +BUG(); \
>> > + __size; \
>> > + })
>>
>> Ick.
>>
>> First off, no driver should ever be able to crash the kernel, which you
>> just did.
>
> And which would appear to mean that this code hasn't actually been
> tested - at least not properly with error cases ?
>
> You talk about type safety too but your code is already full of
> "put_user(node->ptr, (void * __user *)ptr))"
None of this (the patch series or the original code) is mine. My
question was more of a general one on designing ioctls, as well as
concerns with changing the existing 32-bit api.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Wed, Dec 4, 2013 at 4:02 PM, Greg KH wrote: > On Wed, Dec 04, 2013 at 02:22:13PM -0800, Colin Cross wrote: >> On Wed, Dec 4, 2013 at 2:02 PM, Greg KH wrote: >> > On Wed, Dec 04, 2013 at 01:55:34PM -0800, Colin Cross wrote: >> >> On Wed, Dec 4, 2013 at 1:43 PM, Greg KH >> >> wrote: >> >> > On Wed, Dec 04, 2013 at 12:46:42PM -0800, Colin Cross wrote: >> >> >> On Wed, Dec 4, 2013 at 10:35 AM, Greg KH >> >> >> wrote: >> >> >> >> >> >> >> >> >> > And finally, is this all really needed? Why not just fix the >> >> >> > structures >> >> >> > to be "correct", and then fix userspace to use the correct >> >> >> > structures as >> >> >> > well, thereby not needing a compat layer at all? >> >> >> >> >> >> Some of the binder ioctls take userspace pointers. Are you suggesting >> >> >> storing those pointers in a __u64 to avoid having to have a >> >> >> compat_ioctl? >> >> > >> >> > Yes, that's the best way to solve the issue, right? >> >> >> >> It's the least code, but in exchange you lose all the type safety and >> >> warnings when copying in and out of the pointers, as well as sparse >> >> checking on the __user attribute. >> > >> > Not if you make the cast right at the beginning, when you first "touch" >> > the data, but yes, it does take some of the type saftey away, at the >> > expense of simpler code to mess up :) >> > >> >> That doesn't seem like a good tradeoff to me. In addition it requires >> >> modifying the existing heavily used 32 bit api, which means a >> >> mostly-equivalent compat layer added in libbinder to support old >> >> kernels. >> > >> > Wait, I thought that libbinder would have to be changed anyway here, to >> > handle 64bit kernels (in both 32 and 64bit userspace). Since you are >> > already changing it, why not just "do it correctly"? >> >> libbinder will need changes to support 64-bit userspace and especially >> a mixed 64-bit and 32-bit userspace, but this patch series is only >> addressing a pure 32-bit userspace on a 64-bit kernel. Support for a >> 64-bit userspace in Android is obviously going to require a future >> version of Android including, among other things, libbinder changes. >> As far as I know, those changes won't need to change the ioctl api, >> only the layout of the buffers that are passed through the ioctl api. > > "only" means you can rearrange things at that point in time, as you will > have to be doing that anyway :) > >> > Or does this patch series mean that no userspace code is changed? Is >> > that a "requirement" here? >> >> Since this series only addresses 32-bit userspace on 64-bit kernel >> support there are no associated userspace changes. Changing the >> 32-bit api here means that combining the KitKat branch from >> http://android.googlesource.com with a newer kernel version will not >> work. > > Is that something that anyone has said would work in the past? It seems > that other parts of the Android userspace are pretty tied to specific > kernel features / versions, is this anything new if the binder code had > to change? We have explicitly said that Android userspace does not require a specific kernel version. I expect to see KitKat devices running at least 3.4, 3.10, and whatever is the next long term stable version. Sometimes new kernels need userspace support, but we try to avoid that as much as possible. The last major one I can remember was removing early suspend in 3.4, but in that case I provided a compat 3.4 branch to allow using 3.4 with an older userspace. Changing the binder api is not completely unsupportable for old versions, my guess is some people would just ship with a new kernel with the binder changes reverted. > Anyway, the code as submitted has problems, see my response to the > second patch, so it's not ready yet anyway :( > > thanks, > > greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Re: [PATCH 1/1] [ion]: system-heap use PAGE_ALLOC_COSTLY_ORDER for high order
On Mon, Oct 6, 2014 at 9:26 AM, PINTU KUMAR wrote: > > Hi, > > > > From: Laura Abbott > >To: Heesub Shin ; Pintu Kumar > >; a...@linux-foundation.org; > >gre...@linuxfoundation.org; john.stu...@linaro.org; rebe...@android.com; > >ccr...@android.com; de...@driverdev.osuosl.org; linux-ker...@vger.kernel.org > >Cc: iqbal@samsung.com; pintu_agar...@yahoo.com; vishnu...@samsung.com > >Sent: Monday, 6 October 2014 7:37 PM > >Subject: Re: [PATCH 1/1] [ion]: system-heap use PAGE_ALLOC_COSTLY_ORDER for > >high order > > > > > >On 10/6/2014 3:27 AM, Heesub Shin wrote: > > > > > > > > > >> Hello Kumar, > >> > >> On 10/06/2014 05:31 PM, Pintu Kumar wrote: > >>> The Android ion_system_heap uses allocation fallback mechanism > >>> based on 8,4,0 order pages available in the system. > >>> It changes gfp flags based on higher order allocation request. > >>> This higher order value is hard-coded as 4, instead of using > >>> the system defined higher order value. > >>> Thus replacing this hard-coded value with PAGE_ALLOC_COSTLY_ORDER > >>> which is defined as 3. > >>> This will help mapping the higher order request in system heap with > >>> the actual allocation request. > >> > >> Quite reasonable. > >> > >> Reviewed-by: Heesub Shin > >> > >> BTW, Anyone knows how the allocation order (8,4 and 0) was decided? I > >> think only Google guys might know the answer. > >> > >> regards, > >> heesub > >> > > > >My understanding was this was completely unrelated to the costly order > >and was related to the page sizes corresponding to IOMMU page sizes > >(1MB, 64K, 4K). This won't make a difference for the uncached page > >pool case but for the not page pool case, I'm not sure if there would > >be a benefit for trying to get 32K pages with some effort vs. just > >going back to 4K pages. > > No, it is not just related to IOMMU case. It comes into picture also for > normal system-heap allocation (without iommu cases). > Also, it is applicable for both uncached and page_pool cases. > Please also check the changes under ion_system_heap_create. > Here the gfp_flags are set under the pool structure. > This value is used in ion_page_pool_alloc_pages. > In both the cases, it internally calls alloc_pages, with this gfp_flags. > Now, during memory pressure scenario, when alloc_pages moves to slowpath > this gfp_flags will be used to decide allocation retry. > In the current code, the higher-order flag is set only when order is greater > than 4. > But, in MM, the order 4 is also considered as higher-order request. > This higher-order is decided based on PAGE_ALLOC_COSTLY_ORDER (3) value. > Hence, I think this value should be in sync with the MM code. > > > >Do you have any data/metrics that show a benefit from this patch? > I think it is not related to any data or metrics. > It is about replacing the hard-coded higher-order check to be in sync with > the MM code. > The selection of the orders used for allocation (8, then 4, then 0) is designed to match with the sizes often found in IOMMUs, but this isn't changing the order of the allocation, it is changing the GFP flags used for the order 4 allocation. Right now we are using the low_order_gfp_flags for order 4, this patch would change it to use high_order_gfp_flags. We originally used low_order_gfp_flags here because the MM subsystem can usually satisfy these allocations, and the additional load placed on the MM subsystem to kick off kswapd to free up more order 4 chunks is generally worth it. Using order 4 pages instead of order 0 pages can significantly improve the performance of many IOMMUs by reducing TLB pressure and time spent updating page tables. Unless you have data showing that this improves something, and doesn't just cause all allocations to be order 0 when under memory pressure, I don't suggest merging this. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Re: [PATCH 1/1] [ion]: system-heap use PAGE_ALLOC_COSTLY_ORDER for high order
On Tue, Oct 7, 2014 at 9:07 AM, PINTU KUMAR wrote: > - Original Message - >> From: Colin Cross >> To: pint...@samsung.com >> Cc: Laura Abbott ; Heesub Shin >> ; "a...@linux-foundation.org" >> ; "gre...@linuxfoundation.org" >> ; "john.stu...@linaro.org" >> ; "rebe...@android.com" ; >> "de...@driverdev.osuosl.org" ; >> "linux-ker...@vger.kernel.org" ; IQBAL SHAREEF >> ; "pintu_agar...@yahoo.com" >> ; Vishnu Pratap Singh ; >> "c...@samsung.com" >> Sent: Monday, 6 October 2014 11:01 PM >> Subject: Re: Re: [PATCH 1/1] [ion]: system-heap use PAGE_ALLOC_COSTLY_ORDER >> for high order >> >> On Mon, Oct 6, 2014 at 9:26 AM, PINTU KUMAR wrote: >> >>> >>> Hi, >>> > >>> > From: Laura Abbott >>> >To: Heesub Shin ; Pintu Kumar >> ; a...@linux-foundation.org; >> gre...@linuxfoundation.org; john.stu...@linaro.org; rebe...@android.com; >> ccr...@android.com; de...@driverdev.osuosl.org; linux-ker...@vger.kernel.org >>> >Cc: iqbal@samsung.com; pintu_agar...@yahoo.com; >> vishnu...@samsung.com >>> >Sent: Monday, 6 October 2014 7:37 PM >>> >Subject: Re: [PATCH 1/1] [ion]: system-heap use PAGE_ALLOC_COSTLY_ORDER >> for high order >>> > >>> > >>> >On 10/6/2014 3:27 AM, Heesub Shin wrote: >>> > >>> > >>> > >>> > >>> >> Hello Kumar, >>> >> >>> >> On 10/06/2014 05:31 PM, Pintu Kumar wrote: >>> >>> The Android ion_system_heap uses allocation fallback mechanism >>> >>> based on 8,4,0 order pages available in the system. >>> >>> It changes gfp flags based on higher order allocation request. >>> >>> This higher order value is hard-coded as 4, instead of using >>> >>> the system defined higher order value. >>> >>> Thus replacing this hard-coded value with >> PAGE_ALLOC_COSTLY_ORDER >>> >>> which is defined as 3. >>> >>> This will help mapping the higher order request in system heap >> with >>> >>> the actual allocation request. >>> >> >>> >> Quite reasonable. >>> >> >>> >> Reviewed-by: Heesub Shin >>> >> >>> >> BTW, Anyone knows how the allocation order (8,4 and 0) was >> decided? I >>> >> think only Google guys might know the answer. >>> >> >>> >> regards, >>> >> heesub >>> >> >>> > >>> >My understanding was this was completely unrelated to the costly order >>> >and was related to the page sizes corresponding to IOMMU page sizes >>> >(1MB, 64K, 4K). This won't make a difference for the uncached page >>> >pool case but for the not page pool case, I'm not sure if there >> would >>> >be a benefit for trying to get 32K pages with some effort vs. just >>> >going back to 4K pages. >>> >>> No, it is not just related to IOMMU case. It comes into picture also for >>> normal system-heap allocation (without iommu cases). >>> Also, it is applicable for both uncached and page_pool cases. >>> Please also check the changes under ion_system_heap_create. >>> Here the gfp_flags are set under the pool structure. >>> This value is used in ion_page_pool_alloc_pages. >>> In both the cases, it internally calls alloc_pages, with this gfp_flags. >>> Now, during memory pressure scenario, when alloc_pages moves to slowpath >>> this gfp_flags will be used to decide allocation retry. >>> In the current code, the higher-order flag is set only when order is >> greater than 4. >>> But, in MM, the order 4 is also considered as higher-order request. >>> This higher-order is decided based on PAGE_ALLOC_COSTLY_ORDER (3) value. >>> Hence, I think this value should be in sync with the MM code. >>> > >>> >Do you have any data/metrics that show a benefit from this patch? >>> I think it is not related to any data or metrics. >>> It is about replacing the hard-coded higher-order check to be in sync with >>> the MM code. >>> >> >> The selection of the orders used for allocation (8, then 4, then 0) is >> designed to match with the sizes often found in IOMMUs, but this isn't >> changing the order of the allocation, it is changing the G
Re: [PATCH] staging: ion: fix corruption of ion_import_dma_buf
On Wed, Sep 9, 2015 at 10:19 AM, Laura Abbott wrote:
> (adding Colin and John)
>
>
> On 09/09/2015 12:41 AM, Shawn Lin wrote:
>>
>> we found this issue but still exit in lastest kernel. Simply
>> keep ion_handle_create under mutex_lock to avoid this race.
>>
>> WARNING: CPU: 2 PID: 2648 at drivers/staging/android/ion/ion.c:512
>> ion_handle_add+0xb4/0xc0()
>> ion_handle_add: buffer already found.
>> Modules linked in: iwlmvm iwlwifi mac80211 cfg80211 compat
>> CPU: 2 PID: 2648 Comm: TimedEventQueue Tainted: GW3.14.0 #7
>> 9a3efd2c 80faf273 9a3efd6c 9a3efd5c 80935dc9 811d7fd3
>> 9a3efd88 0a58 812208a0 0200 80e128d4 80e128d4 8d4ae00c a8cd8600
>> a8cd8094 9a3efd74 80935e0e 0009 9a3efd6c 811d7fd3 9a3efd88 9a3efd9c
>> Call Trace:
>>[<80faf273>] dump_stack+0x48/0x69
>>[<80935dc9>] warn_slowpath_common+0x79/0x90
>>[<80e128d4>] ? ion_handle_add+0xb4/0xc0
>>[<80e128d4>] ? ion_handle_add+0xb4/0xc0
>>[<80935e0e>] warn_slowpath_fmt+0x2e/0x30
>>[<80e128d4>] ion_handle_add+0xb4/0xc0
>>[<80e144cc>] ion_import_dma_buf+0x8c/0x110
>>[<80c517c4>] reg_init+0x364/0x7d0
>>[<80993363>] ? futex_wait+0x123/0x210
>>[<80992e0e>] ? get_futex_key+0x16e/0x1e0
>>[<8099308f>] ? futex_wake+0x5f/0x120
>>[<80c51e19>] vpu_service_ioctl+0x1e9/0x500
>>[<80994aec>] ? do_futex+0xec/0x8e0
>>[<80971080>] ? prepare_to_wait_event+0xc0/0xc0
>>[<80c51c30>] ? reg_init+0x7d0/0x7d0
>>[<80a22562>] do_vfs_ioctl+0x2d2/0x4c0
>>[<80b198ad>] ? inode_has_perm.isra.41+0x2d/0x40
>>[<80b199cf>] ? file_has_perm+0x7f/0x90
>>[<80b1a5f7>] ? selinux_file_ioctl+0x47/0xf0
>>[<80a227a8>] SyS_ioctl+0x58/0x80
>>[<80fb45e8>] syscall_call+0x7/0x7
>>[<80fb>] ? mmc_do_calc_max_discard+0xab/0xe4
>>
>> Fixes: 83271f626 ("ion: hold reference to handle...")
>> Signed-off-by: Shawn Lin
>> ---
>>
>> drivers/staging/android/ion/ion.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/staging/android/ion/ion.c
>> b/drivers/staging/android/ion/ion.c
>> index eec878e..32e7b5c 100644
>> --- a/drivers/staging/android/ion/ion.c
>> +++ b/drivers/staging/android/ion/ion.c
>> @@ -1179,13 +1179,13 @@ struct ion_handle *ion_import_dma_buf(struct
>> ion_client *client, int fd)
>> mutex_unlock(&client->lock);
>> goto end;
>> }
>> - mutex_unlock(&client->lock);
>>
>> handle = ion_handle_create(client, buffer);
>> - if (IS_ERR(handle))
>> + if (IS_ERR(handle)) {
>> + mutex_unlock(&client->lock);
>> goto end;
>> + }
>>
>> - mutex_lock(&client->lock);
>> ret = ion_handle_add(client, handle);
>> mutex_unlock(&client->lock);
>> if (ret) {
>>
>
> So the patch looks correct but the locking change there seems like it was
> added
> deliberately. Colin/John, do you remember why the locking for
> ion_import_dma_buf
> changed? Was there a deadlock condition somewhere?
>
> Thanks,
> Laura
I can't see any reason to not hold the mutex across ion_handle_create.
The patch that introduced the bug
(83271f6262c91a49df325c52bec8f00f4de294ca, ion: hold reference to
handle after ion_uhandle_get) required that the mutex not be held
around the call to ion_handle_put, but didn't affect
ion_handle_create.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] drivers: staging: android: ion: Kconfig: Let it also depend on HAS_DMA
On Mon, Jul 7, 2014 at 1:49 AM, Chen Gang wrote: > ION need HAS_DMA (e.g. need DMA_SHARED_BUFFER), so it has to depend on > HAS_DMA, or can not pass compiling with allmodconfig under score which > NO_DMA. And the related error: > > CC drivers/staging/android/ion/ion_cma_heap.o > drivers/staging/android/ion/ion_cma_heap.c: In function 'ion_cma_mmap': > drivers/staging/android/ion/ion_cma_heap.c:168:2: error: implicit > declaration of function 'dma_mmap_coherent' > [-Werror=implicit-function-declaration] > return dma_mmap_coherent(dev, vma, info->cpu_addr, info->handle, > ^ > cc1: some warnings being treated as errors > make[4]: *** [drivers/staging/android/ion/ion_cma_heap.o] Error 1 > make[3]: *** [drivers/staging/android/ion] Error 2 > make[2]: *** [drivers/staging/android] Error 2 > make[1]: *** [drivers/staging] Error 2 > make: *** [drivers] Error 2 > > > Signed-off-by: Chen Gang > --- > drivers/staging/android/ion/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/android/ion/Kconfig > b/drivers/staging/android/ion/Kconfig > index 0f8fec1..0a6e4d0 100644 > --- a/drivers/staging/android/ion/Kconfig > +++ b/drivers/staging/android/ion/Kconfig > @@ -1,6 +1,6 @@ > menuconfig ION > bool "Ion Memory Manager" > - depends on HAVE_MEMBLOCK > + depends on HAVE_MEMBLOCK && HAS_DMA > select GENERIC_ALLOCATOR > select DMA_SHARED_BUFFER > ---help--- > -- > 1.9.2.459.g68773ac Acked-by: Colin Cross ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: staging: Unwritten function for ion_carveout_heap.c
On Wed, Jul 23, 2014 at 1:04 PM, Nick Krause wrote: > Hey Greg and others. > Sorry for another email but it seems the function, > ion_carveout_heap_unmap_dma is > just returning and not doing anything useful. Furthermore I am new so > I don't known how > to write this function but this may be causing some rather serious > bugs as if the dma heap > is not unmaped and we call this function a lot this will make the > kernel not able to handle dma requests > for this driver and other drivers that need this and in turn lead to > a oops or even a kernel panic due to leaked > dma allocated memory. I would recommend writing this function or > helping me write it in > other to avoid some rather serious bugs without a proper dma unmapping > function for this driver :). > Nick Look at ion_carveout_heap_map_dma - it doesn't do anything, it just returns a pre-existing virtual address. That means there is nothing to do in unmap. map_dma is actually a bit of a misnomer here, as the actual mapping is done in ion_map_dma_buf. All ion_carveout_heap_map_dma does is return the sg table for ion_map_dma_buf to pass to dma_map_sg. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] LMK: Optimize lowmem_shrink
On Mon, Aug 19, 2013 at 7:23 PM, Colin Cross wrote:
> On Mon, Aug 19, 2013 at 6:16 PM, Leon Ma wrote:
>> From: Leon Ma
>> Date: Mon, 19 Aug 2013 14:22:38 +0800
>> Subject: [PATCH] LMK: Optimize lowmem_shrink.
>>
>> By comparing with selected_oom_score_adj instead of min_score_adj,
>> we may do less calculation.
>>
>> Signed-off-by: Leon Ma
>> ---
>> drivers/staging/android/lowmemorykiller.c | 12
>> 1 files changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/staging/android/lowmemorykiller.c
>> b/drivers/staging/android/lowmemorykiller.c
>> index f6c05c9..cb944c5 100644
>> --- a/drivers/staging/android/lowmemorykiller.c
>> +++ b/drivers/staging/android/lowmemorykiller.c
>> @@ -126,7 +126,7 @@ static int lowmem_shrink(struct shrinker *s, struct
>> shrink_control *sc)
>> return 0;
>> }
>> oom_score_adj = p->signal->oom_score_adj;
>> - if (oom_score_adj < min_score_adj) {
>> + if (oom_score_adj < selected_oom_score_adj) {
>
> This needs to be oom_score_adj <= selected_oom_score_adj.
Sorry, got the logic inverted, yours is correct.
>> task_unlock(p);
>> continue;
>> }
>> @@ -134,13 +134,9 @@ static int lowmem_shrink(struct shrinker *s, struct
>> shrink_control *sc)
>> task_unlock(p);
>> if (tasksize <= 0)
>> continue;
>> - if (selected) {
>> - if (oom_score_adj < selected_oom_score_adj)
>> - continue;
>> - if (oom_score_adj == selected_oom_score_adj &&
>> - tasksize <= selected_tasksize)
>> - continue;
>> - }
>> + if (selected && oom_score_adj == selected_oom_score_adj &&
>> + tasksize <= selected_tasksize)
>> + continue;
>> selected = p;
>> selected_tasksize = tasksize;
>> selected_oom_score_adj = oom_score_adj;
>> --
>> 1.7.4.1
>>
>>
>>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] LMK: Optimize lowmem_shrink
On Mon, Aug 19, 2013 at 6:16 PM, Leon Ma wrote:
> From: Leon Ma
> Date: Mon, 19 Aug 2013 14:22:38 +0800
> Subject: [PATCH] LMK: Optimize lowmem_shrink.
>
> By comparing with selected_oom_score_adj instead of min_score_adj,
> we may do less calculation.
>
> Signed-off-by: Leon Ma
> ---
> drivers/staging/android/lowmemorykiller.c | 12
> 1 files changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/android/lowmemorykiller.c
> b/drivers/staging/android/lowmemorykiller.c
> index f6c05c9..cb944c5 100644
> --- a/drivers/staging/android/lowmemorykiller.c
> +++ b/drivers/staging/android/lowmemorykiller.c
> @@ -126,7 +126,7 @@ static int lowmem_shrink(struct shrinker *s, struct
> shrink_control *sc)
> return 0;
> }
> oom_score_adj = p->signal->oom_score_adj;
> - if (oom_score_adj < min_score_adj) {
> + if (oom_score_adj < selected_oom_score_adj) {
This needs to be oom_score_adj <= selected_oom_score_adj.
> task_unlock(p);
> continue;
> }
> @@ -134,13 +134,9 @@ static int lowmem_shrink(struct shrinker *s, struct
> shrink_control *sc)
> task_unlock(p);
> if (tasksize <= 0)
> continue;
> - if (selected) {
> - if (oom_score_adj < selected_oom_score_adj)
> - continue;
> - if (oom_score_adj == selected_oom_score_adj &&
> - tasksize <= selected_tasksize)
> - continue;
> - }
> + if (selected && oom_score_adj == selected_oom_score_adj &&
> + tasksize <= selected_tasksize)
> + continue;
> selected = p;
> selected_tasksize = tasksize;
> selected_oom_score_adj = oom_score_adj;
> --
> 1.7.4.1
>
>
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
