[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Minios-devel] [UNIKRAFT PATCH v2 6/8] lib/uk9p: Add 9P fid abstraction
Hi Simon, I changed the signatures of the functions to return void. Thanks! Cristi On Fri, Jul 5, 2019 at 1:08 AM Simon Kuenzer <simon.kuenzer@xxxxxxxxx> wrote: > > > > On 05.07.19 00:01, Simon Kuenzer wrote: > > On 29.06.19 10:56, Cristian Banu wrote: > >> This patch adds the uk_9pfid struct, which abstracts the 4-byte fid > >> number used by the 9p protocol to associate numbers with filesystem > >> entries (files, directories, etc.) > >> > >> This patch also adds a fid cache for each 9p device, enabling reuse of > >> the fids and reducing the amount of memory allocations done. > >> > >> Signed-off-by: Cristian Banu <cristb@xxxxxxxxx> > >> --- > >> lib/uk9p/9pdev.c | 121 > >> ++++++++++++++++++++++++++++++++++++++- > >> lib/uk9p/9pfid.c | 71 +++++++++++++++++++++++ > >> lib/uk9p/Makefile.uk | 1 + > >> lib/uk9p/exportsyms.uk | 9 ++- > >> lib/uk9p/include/uk/9pdev.h | 23 ++++++++ > >> lib/uk9p/include/uk/9pdev_core.h | 22 ++++++- > >> lib/uk9p/include/uk/9pfid.h | 115 > >> +++++++++++++++++++++++++++++++++++++ > >> 7 files changed, 359 insertions(+), 3 deletions(-) > >> create mode 100644 lib/uk9p/9pfid.c > >> create mode 100644 lib/uk9p/include/uk/9pfid.h > >> > >> diff --git a/lib/uk9p/9pdev.c b/lib/uk9p/9pdev.c > >> index 05a581eafbdb..11ca4d965893 100644 > >> --- a/lib/uk9p/9pdev.c > >> +++ b/lib/uk9p/9pdev.c > >> @@ -45,11 +45,91 @@ > >> #include <uk/9pdev.h> > >> #include <uk/9pdev_trans.h> > >> #include <uk/9preq.h> > >> +#include <uk/9pfid.h> > >> #if CONFIG_LIBUKSCHED > >> #include <uk/sched.h> > >> #include <uk/wait.h> > >> #endif > >> +static int _fid_mgmt_init(struct uk_9pdev_fid_mgmt *fid_mgmt) > >> +{ > >> + ukarch_spin_lock_init(&fid_mgmt->spinlock); > >> + fid_mgmt->next_fid = 0; > >> + UK_INIT_LIST_HEAD(&fid_mgmt->fid_free_list); > >> + UK_INIT_LIST_HEAD(&fid_mgmt->fid_active_list); > >> + > >> + return 0; > >> +} > > > > Can be a void function, right? > > > >> + > >> +static int _fid_mgmt_next_fid_locked(struct uk_9pdev_fid_mgmt *fid_mgmt, > >> + struct uk_9pdev *dev, > >> + struct uk_9pfid **fid) > >> +{ > >> + struct uk_9pfid *result = NULL; > >> + > >> + if (!uk_list_empty(&fid_mgmt->fid_free_list)) { > >> + result = uk_list_first_entry(&fid_mgmt->fid_free_list, > >> + struct uk_9pfid, _list); > >> + uk_list_del(&result->_list); > >> + } else { > >> + result = uk_9pfid_alloc(dev); > >> + if (!result) > >> + return -ENOMEM; > >> + result->fid = fid_mgmt->next_fid++; > >> + } > >> + > >> + uk_refcount_init(&result->refcount, 1); > >> + result->was_removed = 0; > >> + *fid = result; > >> + > >> + return 0; > >> +} > >> + > >> +static int _fid_mgmt_add_fid_locked(struct uk_9pdev_fid_mgmt *fid_mgmt, > >> + struct uk_9pfid *fid) > >> +{ > >> + uk_list_add(&fid->_list, &fid_mgmt->fid_active_list); > >> + > >> + return 0; > >> +} > > > > > > void here, too? > > >> + > >> +static void _fid_mgmt_del_fid_locked(struct uk_9pdev_fid_mgmt *fid_mgmt, > >> + struct uk_9pfid *fid, > >> + int move_to_freelist) > >> +{ > >> + uk_list_del(&fid->_list); > >> + > >> + if (move_to_freelist) > >> + uk_list_add(&fid->_list, &fid_mgmt->fid_free_list); > >> + else { > >> + /* > >> + * Free the memory associated. This fid will never be used > >> + * again. > >> + */ > >> + uk_pr_warn("Could not move fid to freelist, freeing memory.\n"); > >> + uk_free(fid->_dev->a, fid); > >> + } > >> +} > >> + > >> +static void _fid_mgmt_cleanup(struct uk_9pdev_fid_mgmt *fid_mgmt) > >> +{ > >> + unsigned long flags; > >> + struct uk_9pfid *fid, *fidn; > >> + > >> + ukplat_spin_lock_irqsave(&fid_mgmt->spinlock, flags); > >> + /* > >> + * Every fid should have been clunked *before* destroying the > >> + * connection. > >> + */ > >> + UK_ASSERT(uk_list_empty(&fid_mgmt->fid_active_list)); > >> + uk_list_for_each_entry_safe(fid, fidn, &fid_mgmt->fid_free_list, > >> + _list) { > >> + uk_list_del(&fid->_list); > >> + uk_free(fid->_dev->a, fid); > >> + } > >> + ukplat_spin_unlock_irqrestore(&fid_mgmt->spinlock, flags); > >> +} > >> + > >> static int _req_mgmt_init(struct uk_9pdev_req_mgmt *req_mgmt) > >> { > >> ukarch_spin_lock_init(&req_mgmt->spinlock); > >> @@ -130,15 +210,21 @@ struct uk_9pdev *uk_9pdev_connect(const struct > >> uk_9pdev_trans *trans, > >> if (rc < 0) > >> goto free_dev; > >> - rc = dev->ops->connect(dev, device_identifier, mount_args); > >> + rc = _fid_mgmt_init(&dev->_fid_mgmt); > >> if (rc < 0) > >> goto free_req; > >> + rc = dev->ops->connect(dev, device_identifier, mount_args); > >> + if (rc < 0) > >> + goto free_fid; > >> + > >> UK_ASSERT(dev->msize != 0); > >> dev->state = UK_9PDEV_CONNECTED; > >> goto out; > >> +free_fid: > >> + _fid_mgmt_cleanup(&dev->_fid_mgmt); > >> free_req: > >> _req_mgmt_cleanup(&dev->_req_mgmt); > >> free_dev: > >> @@ -159,6 +245,7 @@ int uk_9pdev_disconnect(struct uk_9pdev *dev) > >> dev->state = UK_9PDEV_DISCONNECTING; > >> /* Clean up the requests before closing the channel. */ > >> + _fid_mgmt_cleanup(&dev->_fid_mgmt); > >> _req_mgmt_cleanup(&dev->_req_mgmt); > >> rc = dev->ops->disconnect(dev); > >> @@ -320,6 +407,38 @@ int uk_9pdev_req_remove(struct uk_9pdev *dev, > >> struct uk_9preq *req) > >> return uk_9preq_put(req); > >> } > >> +struct uk_9pfid *uk_9pdev_fid_create(struct uk_9pdev *dev) > >> +{ > >> + struct uk_9pfid *fid = NULL; > >> + int rc = 0; > >> + unsigned long flags; > >> + > >> + ukplat_spin_lock_irqsave(&dev->_fid_mgmt.spinlock, flags); > >> + rc = _fid_mgmt_next_fid_locked(&dev->_fid_mgmt, dev, &fid); > >> + if (rc < 0) > >> + goto out; > >> + > >> + rc = _fid_mgmt_add_fid_locked(&dev->_fid_mgmt, fid); > >> + if (rc < 0) > >> + goto out; > >> + > >> +out: > >> + ukplat_spin_unlock_irqrestore(&dev->_fid_mgmt.spinlock, flags); > >> + if (rc == 0) > >> + return fid; > >> + return ERR2PTR(rc); > >> +} > >> + > >> +void uk_9pdev_fid_release(struct uk_9pfid *fid) > >> +{ > >> + struct uk_9pdev *dev = fid->_dev; > >> + unsigned long flags; > >> + > >> + ukplat_spin_lock_irqsave(&dev->_fid_mgmt.spinlock, flags); > >> + _fid_mgmt_del_fid_locked(&dev->_fid_mgmt, fid, 1); > >> + ukplat_spin_unlock_irqrestore(&dev->_fid_mgmt.spinlock, flags); > >> +} > >> + > >> void uk_9pdev_adjust_msize(struct uk_9pdev *dev, uint32_t msize) > >> { > >> dev->msize = MIN(dev->msize, msize); > >> diff --git a/lib/uk9p/9pfid.c b/lib/uk9p/9pfid.c > >> new file mode 100644 > >> index 000000000000..85d14c66078a > >> --- /dev/null > >> +++ b/lib/uk9p/9pfid.c > >> @@ -0,0 +1,71 @@ > >> +/* SPDX-License-Identifier: BSD-3-Clause */ > >> +/* > >> + * Authors: Cristian Banu <cristb@xxxxxxxxx> > >> + * > >> + * Copyright (c) 2019, University Politehnica of Bucharest. All > >> rights reserved. > >> + * > >> + * Redistribution and use in source and binary forms, with or without > >> + * modification, are permitted provided that the following conditions > >> + * are met: > >> + * > >> + * 1. Redistributions of source code must retain the above copyright > >> + * notice, this list of conditions and the following disclaimer. > >> + * 2. Redistributions in binary form must reproduce the above copyright > >> + * notice, this list of conditions and the following disclaimer in > >> the > >> + * documentation and/or other materials provided with the > >> distribution. > >> + * 3. Neither the name of the copyright holder nor the names of its > >> + * contributors may be used to endorse or promote products derived > >> from > >> + * this software without specific prior written permission. > >> + * > >> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND > >> CONTRIBUTORS "AS IS" > >> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED > >> TO, THE > >> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR > >> PURPOSE > >> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR > >> CONTRIBUTORS BE > >> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR > >> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF > >> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR > >> BUSINESS > >> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, > >> WHETHER IN > >> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR > >> OTHERWISE) > >> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF > >> ADVISED OF THE > >> + * POSSIBILITY OF SUCH DAMAGE. > >> + * > >> + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY. > >> + */ > >> + > >> +#include <uk/config.h> > >> +#include <uk/refcount.h> > >> +#include <uk/alloc.h> > >> +#include <uk/9pdev.h> > >> +#include <uk/9pfid.h> > >> + > >> +struct uk_9pfid *uk_9pfid_alloc(struct uk_9pdev *dev) > >> +{ > >> + struct uk_9pfid *fid; > >> + > >> + fid = uk_calloc(dev->a, 1, sizeof(*fid)); > >> + if (fid == NULL) > >> + goto out; > >> + > >> + fid->_dev = dev; > >> + > >> + return fid; > >> + > >> +out: > >> + return NULL; > >> +} > >> + > >> +void uk_9pfid_get(struct uk_9pfid *fid) > >> +{ > >> + uk_refcount_acquire(&fid->refcount); > >> +} > >> + > >> +int uk_9pfid_put(struct uk_9pfid *fid) > >> +{ > >> + int last; > >> + > >> + last = uk_refcount_release(&fid->refcount); > >> + if (last) > >> + uk_9pdev_fid_release(fid); > >> + > >> + return last; > >> +} > >> diff --git a/lib/uk9p/Makefile.uk b/lib/uk9p/Makefile.uk > >> index 34cc987a2f9c..cd4bf4b8a033 100644 > >> --- a/lib/uk9p/Makefile.uk > >> +++ b/lib/uk9p/Makefile.uk > >> @@ -6,3 +6,4 @@ CXXINCLUDES-$(CONFIG_LIBUK9P) += > >> -I$(LIBUK9P_BASE)/include > >> LIBUK9P_SRCS-y += $(LIBUK9P_BASE)/9pdev_trans.c > >> LIBUK9P_SRCS-y += $(LIBUK9P_BASE)/9preq.c > >> LIBUK9P_SRCS-y += $(LIBUK9P_BASE)/9pdev.c > >> +LIBUK9P_SRCS-y += $(LIBUK9P_BASE)/9pfid.c > >> diff --git a/lib/uk9p/exportsyms.uk b/lib/uk9p/exportsyms.uk > >> index c373308fb0a2..5cd7817bf5c8 100644 > >> --- a/lib/uk9p/exportsyms.uk > >> +++ b/lib/uk9p/exportsyms.uk > >> @@ -1,6 +1,7 @@ > >> uk_9pdev_trans_register > >> uk_9pdev_trans_by_name > >> uk_9pdev_trans_default > >> + > > > > I guess splitting those entries in groups should probably part of > > earlier patches. But this is really minor... > > > > > >> uk_9preq_get > >> uk_9preq_put > >> uk_9preq_vserialize > >> @@ -12,12 +13,18 @@ uk_9preq_copy_from > >> uk_9preq_receive_cb > >> uk_9preq_waitreply > >> uk_9preq_error > >> + > >> uk_9pdev_connect > >> uk_9pdev_disconnect > >> uk_9pdev_request > >> uk_9pdev_xmit_ready > >> uk_9pdev_call > >> +uk_9pdev_adjust_msize > >> + > >> uk_9pdev_req_create > >> uk_9pdev_req_lookup > >> uk_9pdev_req_remove > >> -uk_9pdev_adjust_msize > >> + > >> +uk_9pdev_fid_create > >> +uk_9pfid_get > >> +uk_9pfid_put > >> diff --git a/lib/uk9p/include/uk/9pdev.h b/lib/uk9p/include/uk/9pdev.h > >> index f2210bbffc92..b8f400cea710 100644 > >> --- a/lib/uk9p/include/uk/9pdev.h > >> +++ b/lib/uk9p/include/uk/9pdev.h > >> @@ -172,6 +172,29 @@ struct uk_9preq *uk_9pdev_req_lookup(struct > >> uk_9pdev *dev, uint16_t tag); > >> int uk_9pdev_req_remove(struct uk_9pdev *dev, struct uk_9preq *req); > >> /** > >> + * Creates a FID associated with the given 9P device. > >> + * > >> + * @param dev > >> + * The Unikraft 9P Device. > >> + * @return > >> + * If not an error pointer, the created fid. > >> + * Otherwise, the error in creating the fid: > >> + * - ENOMEM: No memory for the request or no available tags. > >> + */ > >> +struct uk_9pfid *uk_9pdev_fid_create(struct uk_9pdev *dev); > >> + > >> +/** > >> + * @internal > >> + * Releases a FID when its reference count goes to 0. > >> + * > >> + * Should not be called directly, but rather via uk_9pfid_put(). > >> + * > >> + * @param fid > >> + * The FID to be released. > >> + */ > >> +void uk_9pdev_fid_release(struct uk_9pfid *fid); > >> + > >> +/** > >> * Adjusts the message size to the given maximum size. > >> * > >> * @param dev > >> diff --git a/lib/uk9p/include/uk/9pdev_core.h > >> b/lib/uk9p/include/uk/9pdev_core.h > >> index d7ef341e12d6..1c494732337a 100644 > >> --- a/lib/uk9p/include/uk/9pdev_core.h > >> +++ b/lib/uk9p/include/uk/9pdev_core.h > >> @@ -124,6 +124,24 @@ struct uk_9pdev_req_mgmt { > >> /** > >> * @internal > >> + * A structure used to describe the availability of 9P fids. > >> + */ > >> +struct uk_9pdev_fid_mgmt { > >> + /* Spinlock protecting fids. */ > >> + spinlock_t spinlock; > >> + /* Next available fid. */ > >> + uint32_t next_fid; > >> + /* Free-list of fids that can be reused. */ > >> + struct uk_list_head fid_free_list; > >> + /* > >> + * List of fids that are currently active, to be clunked at the > >> end of > >> + * a 9pfs session. > >> + */ > >> + struct uk_list_head fid_active_list; > >> +}; > >> + > >> +/** > >> + * @internal > >> * 9PDEV transport state > >> * > >> * - CONNECTED: Default state after initialization and during normal > >> operation. > >> @@ -150,7 +168,9 @@ struct uk_9pdev { > >> /* Maximum size of a message. */ > >> uint32_t msize; > >> /* Transport-allocated data. */ > >> - void *priv; > >> + void *priv; > > > > Be careful when your editor changes the whitespaces. Either keep spaces, > > or use tabs. Fixed. > > > >> + /* @internal Fid management. */ > >> + struct uk_9pdev_fid_mgmt _fid_mgmt; > >> /* @internal Request management. */ > >> struct uk_9pdev_req_mgmt _req_mgmt; > >> #if CONFIG_LIBUKSCHED > >> diff --git a/lib/uk9p/include/uk/9pfid.h b/lib/uk9p/include/uk/9pfid.h > >> new file mode 100644 > >> index 000000000000..7ec75bb4a508 > >> --- /dev/null > >> +++ b/lib/uk9p/include/uk/9pfid.h > >> @@ -0,0 +1,115 @@ > >> +/* SPDX-License-Identifier: BSD-3-Clause */ > >> +/* > >> + * Authors: Cristian Banu <cristb@xxxxxxxxx> > >> + * > >> + * Copyright (c) 2019, University Politehnica of Bucharest. All > >> rights reserved. > >> + * > >> + * Redistribution and use in source and binary forms, with or without > >> + * modification, are permitted provided that the following conditions > >> + * are met: > >> + * > >> + * 1. Redistributions of source code must retain the above copyright > >> + * notice, this list of conditions and the following disclaimer. > >> + * 2. Redistributions in binary form must reproduce the above copyright > >> + * notice, this list of conditions and the following disclaimer in > >> the > >> + * documentation and/or other materials provided with the > >> distribution. > >> + * 3. Neither the name of the copyright holder nor the names of its > >> + * contributors may be used to endorse or promote products derived > >> from > >> + * this software without specific prior written permission. > >> + * > >> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND > >> CONTRIBUTORS "AS IS" > >> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED > >> TO, THE > >> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR > >> PURPOSE > >> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR > >> CONTRIBUTORS BE > >> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR > >> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF > >> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR > >> BUSINESS > >> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, > >> WHETHER IN > >> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR > >> OTHERWISE) > >> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF > >> ADVISED OF THE > >> + * POSSIBILITY OF SUCH DAMAGE. > >> + * > >> + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY. > >> + */ > >> + > >> +#ifndef __UK_9PFID__ > >> +#define __UK_9PFID__ > >> + > >> +#include <stdbool.h> > >> +#include <inttypes.h> > >> +#include <uk/config.h> > >> +#include <uk/9p_core.h> > >> +#include <uk/alloc.h> > >> +#include <uk/essentials.h> > >> +#include <uk/list.h> > >> +#include <uk/refcount.h> > >> + > >> +#ifdef __cplusplus > >> +extern "C" { > >> +#endif > >> + > >> +/** > >> + * Structure describing a managed fid via reference counting. > >> + */ > >> +struct uk_9pfid { > >> + /* Fid number. */ > >> + uint32_t fid; > >> + /* Associated server qid. */ > >> + struct uk_9p_qid qid; > >> + /* I/O unit. */ > >> + uint32_t iounit; > >> + /* > >> + * If removed, no clunk is necessary, as the remove operation > >> + * implicitly clunks the fid. > >> + */ > >> + bool was_removed; > >> + /* Tracks the number of live references. */ > >> + __atomic refcount; > >> + /* @internal Associated 9P device. */ > >> + struct uk_9pdev *_dev; > >> + /* > >> + * @internal > >> + * List on which this fid currently is. See uk_9pdev_fid_mgmt for > >> + * details. > >> + */ > >> + struct uk_list_head _list; > >> +}; > >> + > >> +/** > >> + * @internal > >> + * Allocates a 9p fid. > >> + * Should not be used directly, use uk_9pdev_fid_create() instead. > >> + * > >> + * @param a > >> + * Allocator to use. > >> + * @return > >> + * - NULL: Out of memory. > >> + * - (!=NULL): Successful. > >> + */ > >> +struct uk_9pfid *uk_9pfid_alloc(struct uk_9pdev *dev); > > > > Do you need this function then be exported? it is not in exportsyms.uk, as it is only used from within the uk9p library: specifically, from uk_9pdev_fid_create via _fid_mgmt_next_fid_locked. > > > >> + > >> +/** > >> + * Gets the 9p fid, incrementing the reference count. > >> + * > >> + * @param fid > >> + * Reference to the 9p fid. > >> + */ > >> +void uk_9pfid_get(struct uk_9pfid *fid); > >> + > >> +/** > >> + * Puts the 9p fid, decrementing the reference count. > >> + * If this was the last live reference, the memory will be freed. > >> + * > >> + * @param fid > >> + * Reference to the 9p fid. > >> + * @return > >> + * - 0: This was not the last live reference. > >> + * - 1: This was the last live reference. > >> + */ > >> +int uk_9pfid_put(struct uk_9pfid *fid); > >> + > >> +#ifdef __cplusplus > >> +} > >> +#endif > >> + > >> +#endif /* __UK_9PFID__ */ > >> > > > > _______________________________________________ > > Minios-devel mailing list > > Minios-devel@xxxxxxxxxxxxxxxxxxxx > > https://lists.xenproject.org/mailman/listinfo/minios-devel _______________________________________________ Minios-devel mailing list Minios-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/minios-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |