Re: [PATCH] remove sysfs attr_list
On 03/01/2016 04:27 PM, Ferenc Wágner wrote: > Hannes Reinecke writes: > >> On 02/29/2016 10:58 PM, Bart Van Assche wrote: >> >>> On 02/28/16 23:59, Hannes Reinecke wrote: >>> libudev has a _massive_ static memory footprint (Kay doesn't believe in memory allocation). So when using libudev you might end up having a really large memory footprint due to all the on-stack allocations in there. Be especially careful when attempting to use pthreads; debugging stack overflow in pthreads is no fun whatsoever. >>> >>> That's exactly the reason why I never allocate large data structures on >>> the stack in applications I write myself and why I use dynamic memory >>> allocation for large data structures. To make sure that such large stack >>> allocations get detected I set the stack size to a low value: >>> >>> pthread_attr_t attr; >>> pthread_attr_init(&attr); >>> pthread_attr_setstacksize(&attr, PTHREAD_STACK_MIN); >>> pthread_create(..., &attr, ..., ...); >> >> Oh, I don't need to detect them. >> I know exactly where they are. But precisely that is a design goal of >> the libudev code, so any patches trying to fix that up will be discarded >> out-of-hand :-( > > Do you mean that it's a design goal to avoid dynamic allocations in the > libudev code? What's the rationale for that? Avoiding leaks at all costs? > (Sorry for the offtopic, but I wonder...) > Hey, don't ask me. _I_ didn't write that code. Kay Sievers appears to like it, so better ask him. (I would think the idea is that you don't need to do memory allocations, which might fail after all) Cheers, Hannes -- Dr. Hannes Reinecke zSeries & Storage h...@suse.de +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg) -- You received this message because you are subscribed to the Google Groups "open-iscsi" group. To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscr...@googlegroups.com. To post to this group, send email to open-iscsi@googlegroups.com. Visit this group at https://groups.google.com/group/open-iscsi. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH] remove sysfs attr_list
On 02/29/16 17:08, Hannes Reinecke wrote: On 02/29/2016 10:58 PM, Bart Van Assche wrote: On 02/28/16 23:59, Hannes Reinecke wrote: libudev has a _massive_ static memory footprint (Kay doesn't believe in memory allocation). So when using libudev you might end up having a really large memory footprint due to all the on-stack allocations in there. Be especially careful when attempting to use pthreads; debugging stack overflow in pthreads is no fun whatsoever. That's exactly the reason why I never allocate large data structures on the stack in applications I write myself and why I use dynamic memory allocation for large data structures. To make sure that such large stack allocations get detected I set the stack size to a low value: pthread_attr_t attr; pthread_attr_init(&attr); pthread_attr_setstacksize(&attr, PTHREAD_STACK_MIN); pthread_create(..., &attr, ..., ...); Oh, I don't need to detect them. I know exactly where they are. But precisely that is a design goal of the libudev code, so any patches trying to fix that up will be discarded out-of-hand :-( That could be an argument to fork libudev ... Bart. -- You received this message because you are subscribed to the Google Groups "open-iscsi" group. To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscr...@googlegroups.com. To post to this group, send email to open-iscsi@googlegroups.com. Visit this group at https://groups.google.com/group/open-iscsi. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH] remove sysfs attr_list
Hannes Reinecke writes: > On 02/29/2016 10:58 PM, Bart Van Assche wrote: > >> On 02/28/16 23:59, Hannes Reinecke wrote: >> >>> libudev has a _massive_ static memory footprint >>> (Kay doesn't believe in memory allocation). >>> So when using libudev you might end up having a really large memory >>> footprint due to all the on-stack allocations in there. >>> Be especially careful when attempting to use pthreads; debugging stack >>> overflow in pthreads is no fun whatsoever. >> >> That's exactly the reason why I never allocate large data structures on >> the stack in applications I write myself and why I use dynamic memory >> allocation for large data structures. To make sure that such large stack >> allocations get detected I set the stack size to a low value: >> >> pthread_attr_t attr; >> pthread_attr_init(&attr); >> pthread_attr_setstacksize(&attr, PTHREAD_STACK_MIN); >> pthread_create(..., &attr, ..., ...); > > Oh, I don't need to detect them. > I know exactly where they are. But precisely that is a design goal of > the libudev code, so any patches trying to fix that up will be discarded > out-of-hand :-( Do you mean that it's a design goal to avoid dynamic allocations in the libudev code? What's the rationale for that? Avoiding leaks at all costs? (Sorry for the offtopic, but I wonder...) -- Thanks, Feri. -- You received this message because you are subscribed to the Google Groups "open-iscsi" group. To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscr...@googlegroups.com. To post to this group, send email to open-iscsi@googlegroups.com. Visit this group at https://groups.google.com/group/open-iscsi. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH] remove sysfs attr_list
On 02/29/2016 10:58 PM, Bart Van Assche wrote: > On 02/28/16 23:59, Hannes Reinecke wrote: >> libudev has a _massive_ static memory footprint >> (Kay doesn't believe in memory allocation). >> So when using libudev you might end up having a really large memory >> footprint due to all the on-stack allocations in there. >> Be especially careful when attempting to use pthreads; debugging stack >> overflow in pthreads is no fun whatsoever. > > That's exactly the reason why I never allocate large data structures on > the stack in applications I write myself and why I use dynamic memory > allocation for large data structures. To make sure that such large stack > allocations get detected I set the stack size to a low value: > > pthread_attr_t attr; > pthread_attr_init(&attr); > pthread_attr_setstacksize(&attr, PTHREAD_STACK_MIN); > pthread_create(..., &attr, ..., ...); > Oh, I don't need to detect them. I know exactly where they are. But precisely that is a design goal of the libudev code, so any patches trying to fix that up will be discarded out-of-hand :-( Cheers, Hannes -- Dr. Hannes Reinecke zSeries & Storage h...@suse.de +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg) -- You received this message because you are subscribed to the Google Groups "open-iscsi" group. To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscr...@googlegroups.com. To post to this group, send email to open-iscsi@googlegroups.com. Visit this group at https://groups.google.com/group/open-iscsi. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH] remove sysfs attr_list
On Wed, Feb 17, 2016 at 03:01:36PM -0800, Chris Leech wrote: > The global cache is not well designed, it quickly can grow to the point > where lookups take much longer than just doing the sysfs read in the > first place. This patch has problems, the sysfs_attr_get_value changes are wrong when attempting to read an attribute that doesn't exist. It returns the result of calling strdup on random stack data, I'm not sure what I was thinking. Better version soon. - Chris -- You received this message because you are subscribed to the Google Groups "open-iscsi" group. To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscr...@googlegroups.com. To post to this group, send email to open-iscsi@googlegroups.com. Visit this group at https://groups.google.com/group/open-iscsi. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH] remove sysfs attr_list
On 02/28/16 23:59, Hannes Reinecke wrote: libudev has a _massive_ static memory footprint (Kay doesn't believe in memory allocation). So when using libudev you might end up having a really large memory footprint due to all the on-stack allocations in there. Be especially careful when attempting to use pthreads; debugging stack overflow in pthreads is no fun whatsoever. That's exactly the reason why I never allocate large data structures on the stack in applications I write myself and why I use dynamic memory allocation for large data structures. To make sure that such large stack allocations get detected I set the stack size to a low value: pthread_attr_t attr; pthread_attr_init(&attr); pthread_attr_setstacksize(&attr, PTHREAD_STACK_MIN); pthread_create(..., &attr, ..., ...); Bart. -- You received this message because you are subscribed to the Google Groups "open-iscsi" group. To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscr...@googlegroups.com. To post to this group, send email to open-iscsi@googlegroups.com. Visit this group at https://groups.google.com/group/open-iscsi. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH] remove sysfs attr_list
On 02/20/2016 03:41 AM, Chris Leech wrote: > On Thu, Feb 18, 2016 at 06:18:07PM -0600, Mike Christie wrote: >> On 02/17/2016 05:01 PM, Chris Leech wrote: >>> The global cache is not well designed, it quickly can grow to the point >>> where lookups take much longer than just doing the sysfs read in the >>> first place. >> >> Patch looks good. Thanks for working on this. >> >> Is the device cache not as a issue because there are a lot fewer >> devices/cache entries, or because we just to not go down that code path >> as often? > > More of the second I think. It doesn't pop up in a CPU profile, as > session login seems to be dominated by dominated by a mix of VFS > syscalls (mostly stat, lstat and open) now. > > There's a bit of a memory footprint hit for it, more significant to > iscsiadm than iscsid which has much bigger memory costs. With a bit of > testing to ensure it doesn't degrade anything it can probably be removed > as well. > Aah. That triggers bad memories. libudev has a _massive_ static memory footprint (Kay doesn't believe in memory allocation). So when using libudev you might end up having a really large memory footprint due to all the on-stack allocations in there. Be especially careful when attempting to use pthreads; debugging stack overflow in pthreads is no fun whatsoever. Cheers, Hannes -- Dr. Hannes Reinecke zSeries & Storage h...@suse.de +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg) -- You received this message because you are subscribed to the Google Groups "open-iscsi" group. To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscr...@googlegroups.com. To post to this group, send email to open-iscsi@googlegroups.com. Visit this group at https://groups.google.com/group/open-iscsi. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH] remove sysfs attr_list
On Thu, Feb 18, 2016 at 06:18:07PM -0600, Mike Christie wrote: > On 02/17/2016 05:01 PM, Chris Leech wrote: > > The global cache is not well designed, it quickly can grow to the point > > where lookups take much longer than just doing the sysfs read in the > > first place. > > Patch looks good. Thanks for working on this. > > Is the device cache not as a issue because there are a lot fewer > devices/cache entries, or because we just to not go down that code path > as often? More of the second I think. It doesn't pop up in a CPU profile, as session login seems to be dominated by dominated by a mix of VFS syscalls (mostly stat, lstat and open) now. There's a bit of a memory footprint hit for it, more significant to iscsiadm than iscsid which has much bigger memory costs. With a bit of testing to ensure it doesn't degrade anything it can probably be removed as well. - Chris -- You received this message because you are subscribed to the Google Groups "open-iscsi" group. To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscr...@googlegroups.com. To post to this group, send email to open-iscsi@googlegroups.com. Visit this group at https://groups.google.com/group/open-iscsi. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH] remove sysfs attr_list
On 02/17/2016 05:01 PM, Chris Leech wrote:
> The global cache is not well designed, it quickly can grow to the point
> where lookups take much longer than just doing the sysfs read in the
> first place.
> ---
> usr/host.c | 1 +
> usr/session_info.c | 1 +
> usr/sysfs.c| 62
> --
> 3 files changed, 16 insertions(+), 48 deletions(-)
>
> diff --git a/usr/host.c b/usr/host.c
> index f2052d3..6333490 100644
> --- a/usr/host.c
> +++ b/usr/host.c
> @@ -274,6 +274,7 @@ int host_info_print(int info_level, uint32_t host_no)
> printf("iSCSI Transport Class version %s\n",
> version);
> printf("version %s\n", ISCSI_VERSION_STR);
> + free(version);
> }
>
> flags |= SESSION_INFO_SCSI_DEVS;
> diff --git a/usr/session_info.c b/usr/session_info.c
> index 2f48e65..89422d8 100644
> --- a/usr/session_info.c
> +++ b/usr/session_info.c
> @@ -390,6 +390,7 @@ int session_info_print(int info_level, struct
> session_info *info, int do_show)
> printf("iSCSI Transport Class version %s\n",
> version);
> printf("version %s\n", ISCSI_VERSION_STR);
> + free(version);
> }
>
> flags |= (SESSION_INFO_SCSI_DEVS | SESSION_INFO_HOST_DEVS);
> diff --git a/usr/sysfs.c b/usr/sysfs.c
> index 6520bf6..efd4f74 100644
> --- a/usr/sysfs.c
> +++ b/usr/sysfs.c
> @@ -63,15 +63,6 @@ char sysfs_path[PATH_SIZE];
> /* device cache */
> static LIST_HEAD(dev_list);
>
> -/* attribute value cache */
> -static LIST_HEAD(attr_list);
> -
> -struct sysfs_attr {
> - struct list_head node;
> - char path[PATH_SIZE];
> - char *value;/* points to value_local if value is
> cached */
> - char value_local[NAME_SIZE];
> -};
> int sysfs_init(void)
> {
> const char *env;
> @@ -85,22 +76,14 @@ int sysfs_init(void)
> dbg("sysfs_path='%s'", sysfs_path);
>
> INIT_LIST_HEAD(&dev_list);
> - INIT_LIST_HEAD(&attr_list);
> return 0;
> }
>
> void sysfs_cleanup(void)
> {
> - struct sysfs_attr *attr_loop;
> - struct sysfs_attr *attr_temp;
> struct sysfs_device *dev_loop;
> struct sysfs_device *dev_temp;
>
> - list_for_each_entry_safe(attr_loop, attr_temp, &attr_list, node) {
> - list_del_init(&attr_loop->node);
> - free(attr_loop);
> - }
> -
Patch looks good. Thanks for working on this.
Is the device cache not as a issue because there are a lot fewer
devices/cache entries, or because we just to not go down that code path
as often?
--
You received this message because you are subscribed to the Google Groups
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.
