RE: [PATCH v7 1/8] Talitos: Support for async_tx XOR offload
On Fri, 31 Aug 2012 06:08:05 +0300, Liu Qiang-B32616 b32...@freescale.com wrote: -Original Message- From: Geanta Neag Horia Ioan-B05471 Sent: Thursday, August 30, 2012 10:23 PM To: Liu Qiang-B32616; linux-cry...@vger.kernel.org; dan.j.willi...@gmail.com; herb...@gondor.hengli.com.au; da...@davemloft.net; linux-ker...@vger.kernel.org; linuxppc- d...@lists.ozlabs.org Cc: Li Yang-R58472; Phillips Kim-R1AAHA; vinod.k...@intel.com; dan.j.willi...@intel.com; a...@arndb.de; gre...@linuxfoundation.org; Liu Qiang-B32616 Subject: RE: [PATCH v7 1/8] Talitos: Support for async_tx XOR offload On Thu, 9 Aug 2012 11:20:48 +0300, qiang@freescale.com wrote: From: Qiang Liu qiang@freescale.com Expose Talitos's XOR functionality to be used for RAID parity calculation via the Async_tx layer. Cc: Herbert Xu herb...@gondor.apana.org.au Cc: David S. Miller da...@davemloft.net Signed-off-by: Dipen Dudhat dipen.dud...@freescale.com Signed-off-by: Maneesh Gupta maneesh.gu...@freescale.com Signed-off-by: Kim Phillips kim.phill...@freescale.com Signed-off-by: Vishnu Suresh vis...@freescale.com Signed-off-by: Qiang Liu qiang@freescale.com --- drivers/crypto/Kconfig |9 + drivers/crypto/talitos.c | 413 ++ drivers/crypto/talitos.h | 53 ++ 3 files changed, 475 insertions(+), 0 deletions(-) +static int talitos_alloc_chan_resources(struct dma_chan *chan) +{ + struct talitos_xor_chan *xor_chan; + struct talitos_xor_desc *desc; + LIST_HEAD(tmp_list); + int i; + + xor_chan = container_of(chan, struct talitos_xor_chan, common); + + if (!list_empty(xor_chan-free_desc)) + return xor_chan-total_desc; + + for (i = 0; i TALITOS_MAX_DESCRIPTOR_NR; i++) { + desc = talitos_xor_alloc_descriptor(xor_chan, + GFP_KERNEL | GFP_DMA); talitos_xor_alloc_descriptor() is called here without holding the xor_chan-desc_lock and it increments xor_chan-total_desc. Isn't this an issue ? No, please refer to the code as below, + list_add_tail(desc-node, tmp_list); The list is temporary list, it will be merged to xor_chan-free_desc in next step, here is protected by lock, + spin_lock_bh(xor_chan-desc_lock); + list_splice_init(tmp_list, xor_chan-free_desc); + spin_unlock_bh(xor_chan-desc_lock); I was not referring to the list, but to xor_chan-total_desc variable. The following access: talitos_alloc_chan_resources()-talitos_xor_alloc_descriptor()-total_desc++ is not protected by the xor_chan-desc_lock. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH v7 1/8] Talitos: Support for async_tx XOR offload
-Original Message- From: Geanta Neag Horia Ioan-B05471 Sent: Friday, August 31, 2012 6:39 PM To: Liu Qiang-B32616; linux-cry...@vger.kernel.org; dan.j.willi...@gmail.com; herb...@gondor.hengli.com.au; da...@davemloft.net; linux-ker...@vger.kernel.org; linuxppc- d...@lists.ozlabs.org Cc: Li Yang-R58472; Phillips Kim-R1AAHA; vinod.k...@intel.com; Dan Williams; a...@arndb.de; gre...@linuxfoundation.org Subject: RE: [PATCH v7 1/8] Talitos: Support for async_tx XOR offload On Fri, 31 Aug 2012 06:08:05 +0300, Liu Qiang-B32616 b32...@freescale.com wrote: -Original Message- From: Geanta Neag Horia Ioan-B05471 Sent: Thursday, August 30, 2012 10:23 PM To: Liu Qiang-B32616; linux-cry...@vger.kernel.org; dan.j.willi...@gmail.com; herb...@gondor.hengli.com.au; da...@davemloft.net; linux-ker...@vger.kernel.org; linuxppc- d...@lists.ozlabs.org Cc: Li Yang-R58472; Phillips Kim-R1AAHA; vinod.k...@intel.com; dan.j.willi...@intel.com; a...@arndb.de; gre...@linuxfoundation.org; Liu Qiang-B32616 Subject: RE: [PATCH v7 1/8] Talitos: Support for async_tx XOR offload On Thu, 9 Aug 2012 11:20:48 +0300, qiang@freescale.com wrote: From: Qiang Liu qiang@freescale.com Expose Talitos's XOR functionality to be used for RAID parity calculation via the Async_tx layer. Cc: Herbert Xu herb...@gondor.apana.org.au Cc: David S. Miller da...@davemloft.net Signed-off-by: Dipen Dudhat dipen.dud...@freescale.com Signed-off-by: Maneesh Gupta maneesh.gu...@freescale.com Signed-off-by: Kim Phillips kim.phill...@freescale.com Signed-off-by: Vishnu Suresh vis...@freescale.com Signed-off-by: Qiang Liu qiang@freescale.com --- drivers/crypto/Kconfig |9 + drivers/crypto/talitos.c | 413 ++ drivers/crypto/talitos.h | 53 ++ 3 files changed, 475 insertions(+), 0 deletions(-) +static int talitos_alloc_chan_resources(struct dma_chan *chan) +{ + struct talitos_xor_chan *xor_chan; + struct talitos_xor_desc *desc; + LIST_HEAD(tmp_list); + int i; + + xor_chan = container_of(chan, struct talitos_xor_chan, common); + + if (!list_empty(xor_chan-free_desc)) + return xor_chan-total_desc; + + for (i = 0; i TALITOS_MAX_DESCRIPTOR_NR; i++) { + desc = talitos_xor_alloc_descriptor(xor_chan, + GFP_KERNEL | GFP_DMA); talitos_xor_alloc_descriptor() is called here without holding the xor_chan-desc_lock and it increments xor_chan-total_desc. Isn't this an issue ? No, please refer to the code as below, + list_add_tail(desc-node, tmp_list); The list is temporary list, it will be merged to xor_chan-free_desc in next step, here is protected by lock, + spin_lock_bh(xor_chan-desc_lock); + list_splice_init(tmp_list, xor_chan-free_desc); + spin_unlock_bh(xor_chan-desc_lock); I was not referring to the list, but to xor_chan-total_desc variable. The following access: talitos_alloc_chan_resources()-talitos_xor_alloc_descriptor()- total_desc++ is not protected by the xor_chan-desc_lock. Ok, I will correct it in next series. Thanks. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC v8 PATCH 00/20] memory-hotplug: hot-remove physical memory
On Tue, 28 Aug 2012 18:00:07 +0800 we...@cn.fujitsu.com wrote: This patch series aims to support physical memory hot-remove. Have you had much review and testing feedback yet? The patches can free/remove the following things: - acpi_memory_info : [RFC PATCH 4/19] - /sys/firmware/memmap/X/{end, start, type} : [RFC PATCH 8/19] - iomem_resource: [RFC PATCH 9/19] - mem_section and related sysfs files : [RFC PATCH 10-11, 13-16/19] - page table of removed memory : [RFC PATCH 12/19] - node and related sysfs files : [RFC PATCH 18-19/19] If you find lack of function for physical memory hot-remove, please let me know. I doubt if many people have hardware which permits physical memory removal? How would you suggest that people with regular hardware can test these chagnes? Known problems: 1. memory can't be offlined when CONFIG_MEMCG is selected. That's quite a problem! Do you have a description of why this is the case, and a plan for fixing it? ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC v8 PATCH 04/20] memory-hotplug: offline and remove memory when removing the memory device
On Tue, 28 Aug 2012 18:00:11 +0800 we...@cn.fujitsu.com wrote: +int remove_memory(int nid, u64 start, u64 size) +{ + int ret = -EBUSY; + lock_memory_hotplug(); + /* + * The memory might become online by other task, even if you offine it. + * So we check whether the cpu has been onlined or not. I think you meant memory, not cpu. Actually, check whether any part of this memory range has been onlined would be better. If that is accurate ;) + */ + if (!is_memblk_offline(start, size)) { + pr_warn(memory removing [mem %#010llx-%#010llx] failed, + because the memmory range is online\n, + start, start + size); + ret = -EAGAIN; + } + + unlock_memory_hotplug(); + return ret; + +} +EXPORT_SYMBOL_GPL(remove_memory); ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC v8 PATCH 08/20] memory-hotplug: remove /sys/firmware/memmap/X sysfs
On Tue, 28 Aug 2012 18:00:15 +0800 we...@cn.fujitsu.com wrote: From: Yasuaki Ishimatsu isimatu.yasu...@jp.fujitsu.com When (hot)adding memory into system, /sys/firmware/memmap/X/{end, start, type} sysfs files are created. But there is no code to remove these files. The patch implements the function to remove them. Note : The code does not free firmware_map_entry since there is no way to free memory which is allocated by bootmem. +#define to_memmap_entry(obj) container_of(obj, struct firmware_map_entry, kobj) It would be better to implement this as an inlined C function. That has improved type safety and improved readability. +static void release_firmware_map_entry(struct kobject *kobj) +{ + struct firmware_map_entry *entry = to_memmap_entry(kobj); + struct page *page; + + page = virt_to_page(entry); + if (PageSlab(page) || PageCompound(page)) That PageCompound() test looks rather odd. Why is this done? + kfree(entry); + + /* There is no way to free memory allocated from bootmem*/ +} This function is a bit ugly - poking around in page flags to determine whether or not the memory came from bootmem. It would be cleaner to use a separate boolean. Although I guess we can live with it as you have it here. static struct kobj_type memmap_ktype = { + .release= release_firmware_map_entry, .sysfs_ops = memmap_attr_ops, .default_attrs = def_attrs, }; @@ -123,6 +139,16 @@ static int firmware_map_add_entry(u64 start, u64 end, return 0; } +/** + * firmware_map_remove_entry() - Does the real work to remove a firmware + * memmap entry. + * @entry: removed entry. + **/ +static inline void firmware_map_remove_entry(struct firmware_map_entry *entry) +{ + list_del(entry-list); +} Is there no locking to protect that list? /* * Add memmap entry on sysfs */ @@ -144,6 +170,31 @@ static int add_sysfs_fw_map_entry(struct firmware_map_entry *entry) return 0; } +/* + * Remove memmap entry on sysfs + */ +static inline void remove_sysfs_fw_map_entry(struct firmware_map_entry *entry) +{ + kobject_put(entry-kobj); +} + +/* + * Search memmap entry + */ + +struct firmware_map_entry * __meminit +find_firmware_map_entry(u64 start, u64 end, const char *type) A better name would be firmware_map_find_entry(). To retain the (good) convention that symbols exported from here all start with firmware_map_. +{ + struct firmware_map_entry *entry; + + list_for_each_entry(entry, map_entries, list) + if ((entry-start == start) (entry-end == end) + (!strcmp(entry-type, type))) + return entry; + + return NULL; +} + /** * firmware_map_add_hotplug() - Adds a firmware mapping entry when we do * memory hotplug. @@ -196,6 +247,32 @@ int __init firmware_map_add_early(u64 start, u64 end, const char *type) return firmware_map_add_entry(start, end, type, entry); } +/** + * firmware_map_remove() - remove a firmware mapping entry + * @start: Start of the memory range. + * @end: End of the memory range. + * @type: Type of the memory range. + * + * removes a firmware mapping entry. + * + * Returns 0 on success, or -EINVAL if no entry. + **/ +int __meminit firmware_map_remove(u64 start, u64 end, const char *type) +{ + struct firmware_map_entry *entry; + + entry = find_firmware_map_entry(start, end - 1, type); + if (!entry) + return -EINVAL; + + firmware_map_remove_entry(entry); + + /* remove the memmap entry */ + remove_sysfs_fw_map_entry(entry); + + return 0; +} Again, the lack of locking looks bad. ... --- a/mm/memory_hotplug.c +++ b/mm/memory_hotplug.c @@ -1052,9 +1052,9 @@ int offline_memory(u64 start, u64 size) return 0; } -int remove_memory(int nid, u64 start, u64 size) +int __ref remove_memory(int nid, u64 start, u64 size) Why was __ref added? { - int ret = -EBUSY; + int ret = 0; lock_memory_hotplug(); /* * The memory might become online by other task, even if you offine it. ... ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC v8 PATCH 13/20] memory-hotplug: check page type in get_page_bootmem
On Tue, 28 Aug 2012 18:00:20 +0800 we...@cn.fujitsu.com wrote: From: Yasuaki Ishimatsu isimatu.yasu...@jp.fujitsu.com There is a possibility that get_page_bootmem() is called to the same page many times. So when get_page_bootmem is called to the same page, the function only increments page-_count. I really don't understand this explanation, even after having looked at the code. Can you please have another attempt at the changelog? --- a/mm/memory_hotplug.c +++ b/mm/memory_hotplug.c @@ -95,10 +95,17 @@ static void release_memory_resource(struct resource *res) static void get_page_bootmem(unsigned long info, struct page *page, unsigned long type) { - page-lru.next = (struct list_head *) type; - SetPagePrivate(page); - set_page_private(page, info); - atomic_inc(page-_count); + unsigned long page_type; + + page_type = (unsigned long) page-lru.next; + if (page_type MEMORY_HOTPLUG_MIN_BOOTMEM_TYPE || + page_type MEMORY_HOTPLUG_MAX_BOOTMEM_TYPE){ + page-lru.next = (struct list_head *) type; + SetPagePrivate(page); + set_page_private(page, info); + atomic_inc(page-_count); + } else + atomic_inc(page-_count); } And a code comment which explains what is going on would be good. As is always the case ;) ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev