From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_2 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0F148C2D0DB for ; Fri, 31 Jan 2020 14:49:59 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id DF85F217BA for ; Fri, 31 Jan 2020 14:49:58 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728939AbgAaOt6 (ORCPT ); Fri, 31 Jan 2020 09:49:58 -0500 Received: from mail.kernel.org ([198.145.29.99]:54750 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728825AbgAaOt6 (ORCPT ); Fri, 31 Jan 2020 09:49:58 -0500 Received: from gandalf.local.home (cpe-66-24-58-225.stny.res.rr.com [66.24.58.225]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id D64AD206D5; Fri, 31 Jan 2020 14:49:56 +0000 (UTC) Date: Fri, 31 Jan 2020 09:49:54 -0500 From: Steven Rostedt To: Tzvetomir Stoyanov Cc: "Tzvetomir Stoyanov (VMware)" , Linux Trace Devel Subject: Re: [PATCH v18 16/18] trace-cmd: Basic infrastructure for host - guest timestamp synchronization Message-ID: <20200131094954.7c553e85@gandalf.local.home> In-Reply-To: References: <20191213153029.133570-1-tz.stoyanov@gmail.com> <20191213153029.133570-17-tz.stoyanov@gmail.com> <20191220234829.55000e54@rorschach.local.home> X-Mailer: Claws Mail 3.17.3 (GTK+ 2.24.32; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-trace-devel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-trace-devel@vger.kernel.org On Fri, 31 Jan 2020 09:53:41 +0000 Tzvetomir Stoyanov wrote: > On Sat, Dec 21, 2019 at 6:48 AM Steven Rostedt wrote: > > > [ .. ] > > > +#define PROTO_MASK_SIZE (sizeof(char)) > > > > I'm thinking that declaring a mask the size of 1 is a bit overkill. > > > This define is a leftover from the previous implementation of this > bitmask, where > the size was 4 bytes. It was very easy to switch to 1 byte using the > define, that's > why I would prefer to keep it - in case we decide to change the size again. > The code below is written with the assumption that the size could be > more than 1 byte. OK, that makes more sense. > > [ .. ] > > > + */ > > > +void tracecmd_tsync_free(struct tracecmd_time_sync *tsync) > > > +{ > > > + struct clock_sync_context *tsync_context; > > > + struct tsync_proto *proto; > > > + > > > + if (!tsync->context) > > > + return; > > > + tsync_context = (struct clock_sync_context *)tsync->context; > > > + > > > + proto = tsync_proto_find(tsync->sync_proto); > > > + if (proto && proto->clock_sync_free) > > > + proto->clock_sync_free(tsync); > > > + > > > + clock_synch_delete_instance(tsync_context->vinst); > > > + tsync_context->vinst = NULL; > > > + > > > + free(tsync_context->sync_ts); > > > + free(tsync_context->sync_offsets); > > > + tsync_context->sync_ts = NULL; > > > + tsync_context->sync_offsets = NULL; > > > + tsync_context->sync_count = 0; > > > + tsync_context->sync_size = 0; > > > + pthread_mutex_destroy(&tsync->lock); > > > + pthread_cond_destroy(&tsync->cond); > > > + free(tsync->clock_str); > > > > I would think we would want a free(tsync) here. As the name of the > > function suggests. > > > There is no API to allocate it, that's why this function does not free > the memory. There is one use case where this memory is not allocated, > and free(tsync) will not work for it. Then perhaps we should call it: tracecmd_tsync_cleanup(), as that's what it is doing. _free() usually means you are actually freeing what you pass in. > > [ .. ] > > > + > > > +unsigned int tracecmd_guest_tsync(char *tsync_protos, > > > + unsigned int tsync_protos_size, char *clock, > > > + unsigned int *tsync_port, pthread_t *thr_id) > > > +{ > > > + struct tracecmd_time_sync *tsync = NULL; > > > + cpu_set_t *pin_mask = NULL; > > > + pthread_attr_t attrib; > > > + size_t mask_size = 0; > > > + unsigned int proto; > > > + int ret; > > > + int fd; > > > + > > > + fd = -1; > > > + proto = tracecmd_tsync_proto_select(tsync_protos, tsync_protos_size); > > > + if (!proto) > > > + return 0; > > > +#ifdef VSOCK > > > + fd = trace_make_vsock(VMADDR_PORT_ANY); > > > + if (fd < 0) > > > + goto error; > > > + > > > + ret = trace_get_vsock_port(fd, tsync_port); > > > + if (ret < 0) > > > + goto error; > > > +#else > > > + return 0; > > > > If we have no synchronization support, shouldn't this return an error? > > This function returns the id of negotiated time sync protocol. 0 means the > negotiation was not successful, no protocol is selected. When the caller > receives 0, it means we have no synchronization with the peer. OK, so this function definitely needs a "kernel doc" header explaining this. -- Steve