From mboxrd@z Thu Jan  1 00:00:00 1970
From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Subject: Re: [PATCH v12 for-xen-4.5 11/20] x86/VPMU: Interface
 for setting PMU mode and flags
Date: Mon, 29 Sep 2014 11:34:58 -0400
Message-ID: <54297C22.6070909@oracle.com>
References: <1411673336-32736-1-git-send-email-boris.ostrovsky@oracle.com>
	<1411673336-32736-12-git-send-email-boris.ostrovsky@oracle.com>
	<5429938B020000780003AA28@mail.emea.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <5429938B020000780003AA28@mail.emea.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>
Cc: kevin.tian@intel.com, keir@xen.org, suravee.suthikulpanit@amd.com, andrew.cooper3@citrix.com, tim@xen.org, dietmar.hahn@ts.fujitsu.com, xen-devel@lists.xen.org, Aravind.Gopalakrishnan@amd.com, jun.nakajima@intel.com, dgdegra@tycho.nsa.gov
List-Id: xen-devel@lists.xenproject.org

On 09/29/2014 11:14 AM, Jan Beulich wrote:
>>>> On 25.09.14 at 21:28, <boris.ostrovsky@oracle.com> wrote:
>> +static int vpmu_force_context_switch(void)
>> +{
>> +    unsigned i, j, allbutself_num, mycpu;
>> +    static s_time_t start, now;
> I don't think "now" needs to be static. In fact it would perhaps better
> be moved into the more narrow scope below.
>
>> +    struct tasklet **sync_task;
>> +    struct vcpu *curr_vcpu = current;
>> +    int ret = 0;
>> +
>> +    allbutself_num = num_online_cpus() - 1;
>> +
>> +    sync_task = xzalloc_array(struct tasklet *, allbutself_num);
> So while the allocation further down got broken up properly, this
> till degenerates to an order-greater-0 allocation for beyond 512
> CPUs. I think you'd be better of (ab)using per-CPU data here.
>
>> +    if ( !sync_task )
>> +    {
>> +        printk(XENLOG_WARNING "vpmu_force_context_switch: out of memory\n");
>> +        return -ENOMEM;
>> +    }
>> +
>> +    for ( i = 0; i < allbutself_num; i++ )
>> +    {
>> +        sync_task[i] = xmalloc(struct tasklet);
>> +        if ( sync_task[i] == NULL )
>> +        {
>> +            printk(XENLOG_WARNING "vpmu_force_context_switch: out of memory\n");
>> +            ret = -ENOMEM;
>> +            goto out;
>> +        }
>> +        tasklet_init(sync_task[i], vpmu_sched_checkin, 0);
>> +    }
>> +
>> +    atomic_set(&vpmu_sched_counter, 0);
>> +
>> +    j = 0;
>> +    mycpu = smp_processor_id();
>> +    for_each_online_cpu( i )
>> +    {
>> +        if ( i != mycpu )
>> +            tasklet_schedule_on_cpu(sync_task[j++], i);
>> +    }
>> +
>> +    vpmu_save(curr_vcpu);
>> +
>> +    start = NOW();
>> +
>> +    /*
>> +     * Note that we may fail here if a CPU is hot-plugged while we are
>> +     * waiting. We will then time out.
>> +     */
>> +    while ( atomic_read(&vpmu_sched_counter) != allbutself_num )
>> +    {
>> +        cpu_relax();
>> +
>> +        now = NOW();
>> +
>> +        /* Give up after 5 seconds */
>> +        if ( now > start + SECONDS(5) )
>> +        {
>> +            printk(XENLOG_WARNING
>> +                   "vpmu_force_context_switch: failed to sync\n");
>> +            ret = -EBUSY;
>> +            break;
>> +        }
>> +
>> +        /* Or after 2 milliseconds if need to be preempted */
> /* Or after 2 ms (arbitrary value) if need to be preempted. */
>
>> +        if ( (now > start + MILLISECS(2)) && hypercall_preempt_check() )
>> +        {
>> +            ret = -EAGAIN;
>> +            break;
>> +        }
> And then this won't do what (I hope) you want on any continuation:
> "start" doesn't get updated again in that case.

Which is fine, 'start' doesn't need to be be updated, it's not (well, 
shouldn't be) a static any longer (just like 'now')

We don't have continuation any more, -EAGAIN will be passed directly to 
the user and the whole operation will need to be restarted by user. So 
'start' will get  reassigned as NOW().

-boris