lkml.org 
[lkml]   [2023]   [Oct]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v5 16/18] x86/sgx: Limit process EPC usage with misc cgroup controller
    Date

    > +
    > +static inline struct sgx_epc_cgroup *sgx_epc_cgroup_from_misc_cg(struct misc_cg *cg)
    > +{
    > + if (cg)
    > + return (struct sgx_epc_cgroup *)(cg->res[MISC_CG_RES_SGX_EPC].priv);
    > +
    > + return NULL;
    > +}
    > +
    >

    Is it good idea to allow passing a NULL @cg to this basic function?

    Why not only call this function when @cg is valid?

    > +
    > +static int __sgx_epc_cgroup_try_charge(struct sgx_epc_cgroup *epc_cg,
    > + bool reclaim)
    > +{
    > + struct sgx_epc_reclaim_control rc;
    > + unsigned int nr_empty = 0;
    > +
    > + sgx_epc_reclaim_control_init(&rc, epc_cg);
    > +
    > + for (;;) {
    > + if (!misc_cg_try_charge(MISC_CG_RES_SGX_EPC, epc_cg->cg,
    > + PAGE_SIZE))
    > + break;
    > +
    > + if (sgx_epc_cgroup_lru_empty(epc_cg))
    > + return -ENOMEM;
    > +
    > + if (signal_pending(current))
    > + return -ERESTARTSYS;
    > +
    > + if (!reclaim) {
    > + queue_work(sgx_epc_cg_wq, &rc.epc_cg->reclaim_work);
    > + return -EBUSY;
    > + }
    > +
    > + if (!sgx_epc_cgroup_reclaim_pages(1, &rc)) {
    > + if (sgx_epc_cgroup_reclaim_failed(&rc)) {
    > + if (++nr_empty > SGX_EPC_RECLAIM_OOM_THRESHOLD)
    > + return -ENOMEM;
    > + schedule();
    > + }
    > + }
    > + }
    > + if (epc_cg->cg != misc_cg_root())
    > + css_get(&epc_cg->cg->css);

    I don't quite understand why root is treated specially.

    And I thought get_current_misc_cg() in sgx_epc_cgroup_try_charge() already grabs
    the reference before calling this function? Why do it again?

    > +
    > + return 0;
    > +}
    > +
    > +/**
    > + * sgx_epc_cgroup_try_charge() - hierarchically try to charge a single EPC page
    > + * @mm: the mm_struct of the process to charge
    > + * @reclaim: whether or not synchronous reclaim is allowed
    > + *
    > + * Returns EPC cgroup or NULL on success, -errno on failure.
    > + */
    > +struct sgx_epc_cgroup *sgx_epc_cgroup_try_charge(bool reclaim)
    > +{
    > + struct sgx_epc_cgroup *epc_cg;
    > + int ret;
    > +
    > + if (sgx_epc_cgroup_disabled())
    > + return NULL;
    > +
    > + epc_cg = sgx_epc_cgroup_from_misc_cg(get_current_misc_cg());
    > + ret = __sgx_epc_cgroup_try_charge(epc_cg, reclaim);
    > + put_misc_cg(epc_cg->cg);
    > +
    > + if (ret)
    > + return ERR_PTR(ret);
    > +
    > + return epc_cg;
    > +}
    > +
    > +/**
    > + * sgx_epc_cgroup_uncharge() - hierarchically uncharge EPC pages
    > + * @epc_cg: the charged epc cgroup
    > + */
    > +void sgx_epc_cgroup_uncharge(struct sgx_epc_cgroup *epc_cg)
    > +{
    > + if (sgx_epc_cgroup_disabled())
    > + return;
    > +
    > + misc_cg_uncharge(MISC_CG_RES_SGX_EPC, epc_cg->cg, PAGE_SIZE);
    > +
    > + if (epc_cg->cg != misc_cg_root())
    > + put_misc_cg(epc_cg->cg);

    Again why root is special? And where is the get_misc_cg()?

    Oh is it the

    if (epc_cg->cg != misc_cg_root())
    css_get(&epc_cg->cg->css);

    in __sgx_epc_cgroup_try_charge()?

    That's horrible to follow. Can this be explicitly done in
    sgx_epc_cgroup_try_charge() and sgx_epc_cgroup_uncharge(), that is, grab the
    reference in the former and release the reference in the latter?

    \
     
     \ /
      Last update: 2023-10-10 11:20    [W:4.292 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site