RE: SLAB for IDL stack allocation (ITS#2183)

[jongchoi@us.ibm.com]

>This sounds like a great idea. I've briefly read this diff and have some
>issues...
>
>This should be a config file option, not a commandline switch. Moving the
>option into back-bdb/config.c would also eliminate the need to modify
slapd's
>main.c/slap.h/mimic.c etc. This is the kind of feature that one would
>typically turn on and leave on, command line switches are best used for
>things that you only want to try once in a while (like debug options).
(Also
>my intent is to make config file options for other command-line switches
>like -h, because again, this tends to be something you want set every time
>slapd runs.) Adding the bdb_idl_slab_on variable into slapd itself
violates
>the abstraction layer, I think.

I agree.

>Also the patch to back-bdb/init.c looks like it has a bug. The chunk from
>@@ -415,6 +429,20 @@ never assigns a value to "slab" but tries to
reference
>"slab->next" as well as free slab->buf and slab.

Yes. It should be change to sth like the following : (thanks !)

in bdb_db_close()

+#ifdef SLAP_IDL_SLAB
+            for ( i = 0; i < MAX_IDL_SLAB_BUCKETS; i++ ) {
+                        bdb_idl_slab_t *slab;
+                        ldap_pvt_thread_rdwr_wlock (
&bdb->bi_idl_slab_rwlock[i] );
+                        while ( bdb->bi_idl_slab_buckets[i] != NULL ) {
*****                         slab = bdb->bi_idl_slab_buckets[i];
+                                    bdb->bi_idl_slab_buckets[i] =
slab->next;
+                                    free ( slab->buf );
+                                    free ( slab );
+                        }
+                        ldap_pvt_thread_rdwr_wunlock (
&bdb->bi_idl_slab_rwlock[i] );
+
+            }
+#endif

>The bdb_idl_slab_on variable probably should be available both as a
backend
>option and as a database option. I would also use it as an int instead of
a
>boolean:
>idl_slab = 0, idl_slab = 16 <-- configure the max depth.
>(Not so sure about this just yet, 16 is probably safe but you never know
when
>you might run across a legitimate sufficiently complex filter...)

Currently, SLAB cache does not store IDL stack whose depth is larger than
16.
Therefore, if depth > 16, the memory will be freed immediately after use.
Although it seems good for common cases, we may consider a more flexible
approach -
i.e. sth like LRU-style SLAB cache.

The patch in the current form was mainly for the most common cases, for the
proof of concept first, and
more importantly, for the community review...
After finding out more issues and after appropriate changes, I'll try to
include it in the CVS repository.

- Jong

------------------------
Jong Hyuk Choi
IBM Thomas J. Watson Research Center - Enterprise Linux Group
P. O. Box 218, Yorktown Heights, NY 10598
email: jongchoi@us.ibm.com
(phone) 914-945-3979    (fax) 914-945-4425   TL: 862-3979