linux-kernel - Re: [PATCH v1 1/3] interconnect: Add generic interconnect controller API

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 30 Jun 2017 15:52:13 +0300
From:   Georgi Djakov <georgi.djakov@...aro.org>
To:     Vincent Guittot <vincent.guittot@...aro.org>
Cc:     "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
        "rjw@...ysocki.net" <rjw@...ysocki.net>,
        Rob Herring <robh+dt@...nel.org>,
        Kevin Hilman <khilman@...libre.com>,
        mturquette <mturquette@...libre.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        Saravana Kannan <skannan@...eaurora.org>,
        Stephen Boyd <sboyd@...eaurora.org>,
        Andy Gross <andy.gross@...aro.org>,
        "Sweeney, Sean" <seansw@....qualcomm.com>,
        "Dai, David" <davidai@...cinc.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        LAK <linux-arm-kernel@...ts.infradead.org>,
        linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v1 1/3] interconnect: Add generic interconnect controller
 API

Hi Vincent,

On 06/28/2017 08:45 PM, Vincent Guittot wrote:
> Hi Georgi,
> 
> On 27 June 2017 at 19:49, Georgi Djakov <georgi.djakov@...aro.org> wrote:
> 
> [snip]
> 
>> +
>> +static int interconnect_aggregate(struct interconnect_node *node,
>> +                                 struct interconnect_creq *creq)
>> +{
>> +       int ret = 0;
>> +
>> +       mutex_lock(&node->icp->lock);
>> +
>> +       if (node->icp->ops->aggregate) {
>> +               ret = node->icp->ops->aggregate(node, creq);
>> +               if (ret) {
>> +                       pr_info("%s: error (%d)\n", __func__, ret);
>> +                       goto out;
>> +               }
>> +       } else {
>> +               /* do not aggregate by default */
>> +               struct icp *icp = node->icp;
>> +
>> +               icp->creq.avg_bw = creq->avg_bw;
>> +               icp->creq.peak_bw = creq->peak_bw;
> 
> Does it means that by default the last caller defines the bandwidth
> for everybody ?
> IMHO, having a default aggregation policy that sums the avg_bw of all
> request of the node
> and that gets the max of peak_bw of all request of a node is better
> 

Yes, i had this in one of the previous versions, but then i removed the
aggregation by default. Will put it back. Thanks!

>> +       }
>> +
>> +out:
>> +       mutex_unlock(&node->icp->lock);
>> +       return ret;
>> +}
>> +
>> +/**
>> + * interconnect_set() - set constraints on a path between two endpoints
>> + * @path: reference to the path returned by interconnect_get()
>> + * @creq: request from the consumer, containing its requirements
>> + *
>> + * This function is used by an interconnect consumer to express its own needs
>> + * in term of bandwidth and QoS for a previously requested path between two
>> + * endpoints. The requests are aggregated and each node is updated accordingly.
>> + *
>> + * Returns 0 on success, or an approproate error code otherwise.
>> + */
>> +int interconnect_set(struct interconnect_path *path,
>> +                    struct interconnect_creq *creq)
>> +{
>> +       struct interconnect_node *next, *prev = NULL;
>> +       size_t i;
>> +       int ret = 0;
>> +
>> +       for (i = 0; i < path->num_nodes; i++, prev = next) {
>> +               next = path->reqs[i].node;
>> +
>> +               if (!next || !prev)
> 
> This needs a comment with an explanation about why you don't do
> anything in this case

Ok.

> 
>> +                       continue;
>> +
>> +               if (next->icp != prev->icp)
> 
> This needs a comment with an explanation about why you don't do
> anything in this case

Ok.

> 
>> +                       continue;
>> +
>> +               /* aggregate requests from consumers */
> 
> you should update the path->reqs[i].avg_bw and path->reqs[i].peak_bw
> with creq values
> before aggregating the requests from the different consumer of a node ?
> 
>               path->reqs[i].avg_bw = creq->avg_bw
>               path->reqs[i].peak_bw = creq->peak_bw

I am updating them currently in the vendor implementation of the
aggregate() function, but this was probably not the right place,
so i will move it here instead. Thanks for the comments!

BR,
Georgi