On Tue, 2012-05-22 at 11:02 -0400, J. Bruce Fields wrote:
> On Tue, May 15, 2012 at 09:12:29AM -0400, Simo Sorce wrote:
> > +/* numbers somewhat arbitrary but large enough for current needs */
> > +#define GSSX_MAX_OUT_HANDLE 128
> > +#define GSSX_MAX_MECH_OID 16
> > +#define GSSX_MAX_SRC_PRINC 256
> > +#define GSSX_KMEMBUF (GSSX_max_output_handle_sz + \
> > + GSSX_max_oid_sz + \
> > + GSSX_max_princ_sz + \
> > + sizeof(struct svc_cred))
> > +
> ...
> > + data->kmembuf = kmalloc(GSSX_KMEMBUF, GFP_KERNEL);
> ...
> > + rctxh.exported_context_token.data = data->kmembuf;
> ...
> > + rctxh.mech.data = data->kmembuf + GSSX_max_output_handle_sz;
> ...
> > + rctxh.src_name.display_name.data = data->kmembuf +
> > + GSSX_max_output_handle_sz +
> > + GSSX_max_oid_sz;
> ...
> > + data->creds = data->kmembuf +
> > + GSSX_max_output_handle_sz +
> > + GSSX_max_oid_sz +
> > + GSSX_max_princ_sz;
>
> Sorry, is this did I complaining about too many kmalloc()'s?
Yes, you complained about kmallocs, so I did this instead :)
> This seems
> likely to break in subtle ways if we ever change one of those constants
> to not be a multiple of a large enough power of 2. And makes the memory
> handling a little more obscure. I'd rather just allocate those
> separately if that's the choice.
I do not see why it would break, the only limit we have is the total
size of the kmembuf.
> But why not just include this in gssp_upcall_data?:
>
> struct gssp_upcall_data {
> - void *kmembuf;
> struct xdr_netobj in_handle;
> struct gssp_in_token in_token;
> struct xdr_netobj out_handle;
> + char out_handle_data[GSSX_MAX_OUT_HANDLE];
> struct xdr_netobj out_token;
> struct xdr_netobj mech_oid;
> + char mech_oid_data[GSSX_MAX_MECH_OID];
> struct xdr_netobj client_name;
> + char client_name_data[GSSX_MAX_SRC_PRINC];
> - struct svc_cred *creds;
> + struct svc_cred creds;
> int major_status;
> int minor_status;
> };
>
> As long as that still comes to under 4k that should be OK.
I did not want to do this to avoid users of the struct starting to rely
on those sizes, exactly to avoid having subtle issues later on if we
need to change them (unlikely).
> Oh, I see, and you'd have to alloc/free this in svcauth_gss_proxy_init
> instead of here, to avoid putting it on the stack there.
Right, plus it increases the stack size, and I really did not want to do
that.
> Whatever, I don't really care how the various xdr_netobj->data's are
> allocated, honestly there's no crusade to eliminate kmalloc()'s, I'll
> only object in a case (like the struct svc_cred field above) where it
> seems obviously unnecessary.
Ok, so what should I do ?
I can remove the static allocation and let the code allocate the data
with kmalloc, in the xdr unmarshalling code.
Whatever you like best.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
