[PATCH weston 2/8] shared: Add helpers to convert between protocol data and timespec
Pekka Paalanen
ppaalanen at gmail.com
Tue Dec 12 13:09:56 UTC 2017
On Tue, 12 Dec 2017 14:43:11 +0200
Alexandros Frantzis <alexandros.frantzis at collabora.com> wrote:
> On Tue, Dec 12, 2017 at 12:09:59PM +0200, Pekka Paalanen wrote:
> > On Mon, 4 Dec 2017 15:34:02 +0200
> > Alexandros Frantzis <alexandros.frantzis at collabora.com> wrote:
> >
> > > Add helpers to safely convert between struct timespec values and
> > > tv_sec_hi, tv_sec_lo, tv_nsec triplets used for sending high-resolution
> > > timestamp data over the wayland protocol. Replace existing conversion
> > > code with the helper functions.
> > >
> > > Signed-off-by: Alexandros Frantzis <alexandros.frantzis at collabora.com>
> > > ---
> > > clients/presentation-shm.c | 9 +------
> > > libweston/compositor.c | 9 ++++---
> > > shared/timespec-util.h | 39 +++++++++++++++++++++++++++
> > > tests/presentation-test.c | 9 +------
> > > tests/timespec-test.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++
> > > 5 files changed, 112 insertions(+), 20 deletions(-)
> >
> > Hi,
>
> Hi,
>
> >
> > I have questions below that will have implications to the protocol
> > extension spec as well. I would like to require the time values in
> > protocol to be normalized.
>
> Ack.
>
> > >
> > > diff --git a/clients/presentation-shm.c b/clients/presentation-shm.c
> > > index c9fb66cc..d6a939e5 100644
> > > --- a/clients/presentation-shm.c
> > > +++ b/clients/presentation-shm.c
> > > @@ -39,6 +39,7 @@
> > > #include <wayland-client.h>
> > > #include "shared/helpers.h"
> > > #include "shared/zalloc.h"
> > > +#include "shared/timespec-util.h"
> > > #include "shared/os-compatibility.h"
> > > #include "presentation-time-client-protocol.h"
> > >
> > > @@ -383,14 +384,6 @@ timespec_to_ms(const struct timespec *ts)
> > > return (uint32_t)ts->tv_sec * 1000 + ts->tv_nsec / 1000000;
> > > }
> > >
> > > -static void
> > > -timespec_from_proto(struct timespec *tm, uint32_t tv_sec_hi,
> > > - uint32_t tv_sec_lo, uint32_t tv_nsec)
> > > -{
> > > - tm->tv_sec = ((uint64_t)tv_sec_hi << 32) + tv_sec_lo;
> > > - tm->tv_nsec = tv_nsec;
> > > -}
> > > -
> > > static int
> > > timespec_diff_to_usec(const struct timespec *a, const struct timespec *b)
> > > {
> > > diff --git a/libweston/compositor.c b/libweston/compositor.c
> > > index 7d7a17ed..083664fd 100644
> > > --- a/libweston/compositor.c
> > > +++ b/libweston/compositor.c
> > > @@ -341,7 +341,9 @@ weston_presentation_feedback_present(
> > > {
> > > struct wl_client *client = wl_resource_get_client(feedback->resource);
> > > struct wl_resource *o;
> > > - uint64_t secs;
> > > + uint32_t tv_sec_hi;
> > > + uint32_t tv_sec_lo;
> > > + uint32_t tv_nsec;
> >
> > A suggestion: how about introducing
> >
> > struct timespec_proto {
> > uint32_t sec_hi;
> > uint32_t sec_lo;
> > uint32_t nsec;
> > };
> >
> > and using that in timespec_to_proto()?
> >
> > (Not useful for timespec_from_proto() because the three variables are
> > already declared.)
>
> I am not opposed to introducing a struct timespec_proto in general, but
> using it in only one of the two functions feels inconsistent.
Ok. I was thinking of saving in the number of local variables on every
call site of timespec_to_proto().
> > > wl_resource_for_each(o, &output->resource_list) {
> > > if (wl_resource_get_client(o) != client)
> > > @@ -350,10 +352,9 @@ weston_presentation_feedback_present(
> > > wp_presentation_feedback_send_sync_output(feedback->resource, o);
> > > }
> > >
> > > - secs = ts->tv_sec;
> > > + timespec_to_proto(ts, &tv_sec_hi, &tv_sec_lo, &tv_nsec);
> > > wp_presentation_feedback_send_presented(feedback->resource,
> > > - secs >> 32, secs & 0xffffffff,
> > > - ts->tv_nsec,
> > > + tv_sec_hi, tv_sec_lo, tv_nsec,
> > > refresh_nsec,
> > > seq >> 32, seq & 0xffffffff,
> > > flags | feedback->psf_flags);
> > > diff --git a/shared/timespec-util.h b/shared/timespec-util.h
> > > index a10edf5b..c734accd 100644
> > > --- a/shared/timespec-util.h
> > > +++ b/shared/timespec-util.h
> > > @@ -175,6 +175,30 @@ timespec_to_usec(const struct timespec *a)
> > > return (int64_t)a->tv_sec * 1000000 + a->tv_nsec / 1000;
> > > }
> > >
> > > +/* Convert timespec to protocol data
> > > + *
> > > + * \param a timespec
> > > + * \param tv_sec_hi[out] the high bytes of the seconds part
> > > + * \param tv_sec_lo[out] the low bytes of the seconds part
> > > + * \param tv_nsec[out] the nanoseconds part
> > > + *
> > > + * The timespec is normalized before being converted to protocol data.
> > > + */
> > > +static inline void
> > > +timespec_to_proto(const struct timespec *a, uint32_t *tv_sec_hi,
> > > + uint32_t *tv_sec_lo, uint32_t *tv_nsec)
> > > +{
> > > + struct timespec r;
> > > +
> > > + timespec_normalize(&r, a);
> > > +
> > > + /* We check the size of tv_sec, so that we shift only if the size
> > > + * is 64-bits, in order to avoid sign extension on 32-bit systems. */
> > > + *tv_sec_hi = sizeof(r.tv_sec) == 8 ? (int64_t)r.tv_sec >> 32 : 0;
> >
> > How about just casting tv_sec to uint64_t before shifting and masking
> > it to 32-bit pieces? I think that would be more obvious to read.
>
> We can only do this safely if tv_sec is non-negative (due to
> int32_t->uint64_t sign extension when time_t is 32 bits), which
> I agree is a sensible pre-condition/assertion to have (see below).
Why would the sign-extension be unwanted? I would expect it.
> > I think it would be better to assert() that the timespec is already:
> >
> > - normalized, because otherwise nsec might overflow in a computation,
> > and
>
> If we make normalized input a pre-condition of timespec_to_proto, we are
> essentially moving the burden to the caller, which in most cases should
> call timespec_normalize themselves before calling timespec_to_proto. I
> am not sure if we should burden the callers with this and introduce the
> possibility of runtime errors.
There would be no burden if we ensure our timestamps are always
normalized, which is a precondition for the timespec sub/add etc.
functions to operate without overflows. Therefore I'd push the
normalize calls to not just the callers but the sites where the
timestamps originate.
> Also, I like the idea that the callers don't need to care about protocol
> encoding details, and just let timespec_to_proto do the conversion
> properly.
I think there are two different "normalized" here. One is struct
timespec, another is what the protocol carries. Compared to struct
timespec, the protocol has an additional requirement of non-negativity.
I don't see that as leaking protocol encoding details - OTOH, we must
not even attempt to send values the protocol cannot carry.
> > - non-negative, because the protocol cannot carry negative
> > timestamps.
>
> Makes sense.
>
> >
> > > + *tv_sec_lo = r.tv_sec;
> > > + *tv_nsec = r.tv_nsec;
> > > +}
> > > +
> > > /* Convert nanoseconds to timespec
> > > *
> > > * \param a timespec
> > > @@ -209,6 +233,21 @@ timespec_from_msec(struct timespec *a, int64_t b)
> > > timespec_from_nsec(a, b * 1000000);
> > > }
> > >
> > > +/* Convert protocol data to timespec
> > > + *
> > > + * \param a[out] timespec
> > > + * \param tv_sec_hi the high bytes of seconds part
> > > + * \param tv_sec_lo the low bytes of seconds part
> > > + * \param tv_nsec the nanoseconds part
> > > + */
> > > +static inline void
> > > +timespec_from_proto(struct timespec *a, uint32_t tv_sec_hi,
> > > + uint32_t tv_sec_lo, uint32_t tv_nsec)
> > > +{
> > > + a->tv_sec = ((uint64_t)tv_sec_hi << 32) + tv_sec_lo;
> >
> > How to handle overflows in tv_sec would be a good question...
>
> A good question indeed. Overflow shouldn't be a problem for well
> behaving clients (if we are talking about real-time timestamps then not
> until 2038 for 32-bits and not ever for 64-bits). The question is how to
> deal with misbehaving and/or malicious clients.
How does client behaviour factor into it?
I wouldn't expect anyone to handle timespec wrapping around, but for input
events this can be fixed in the compositor by shifting the time base if
even necessary.
> There are two aspects to this. The first is 64-bit to 32-bit overflow on
> 32-bit systems, which is currently handled by implicitly dropping the
> high 32-bits. The second is unsigned to signed overflow, which currently
> produces negative timestamps. One way to deal with this is to clamp to
> the largest positive value if we ever get a negative value. Another way
> is to force the sign bit to 0, essentially making negative values cycle
> back to non-negative.
I wonder where time_t would still be 32-bit. I suppose we'll ignore the
issue for now.
> > > + a->tv_nsec = tv_nsec;
> > > +}
> >
> > Btw. refactoring code to introduce timespec_from_proto() with no
> > functional changes should probably be a patch of its own.
>
> Sure, I will split this in v2.
Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20171212/38a4a5b3/attachment-0001.sig>
More information about the wayland-devel
mailing list