From: "Daniel W. S. Almeida" <dwlsalmeida@gmail.com> To: Geert Uytterhoeven <geert@linux-m68k.org> Cc: "mchehab+huawei@kernel.org" <mchehab+huawei@kernel.org>, "r.verdejo@samsung.com" <r.verdejo@samsung.com>, "nicolas@ndufresne.ca" <nicolas@ndufresne.ca>, "linux-media@vger.kernel.org" <linux-media@vger.kernel.org>, "skhan@linuxfoundation.org" <skhan@linuxfoundation.org>, "linux-kernel-mentees@lists.linuxfoundation.org" <linux-kernel-mentees@lists.linuxfoundation.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org> Subject: Re: [v10 3/4] media: vidtv: add a bridge driver Date: Tue, 15 Sep 2020 10:26:17 -0300 [thread overview] Message-ID: <EA89ED5D-E477-4464-B523-D729796CE0D0@getmailspring.com> (raw) In-Reply-To: <alpine.DEB.2.21.2009151345001.31296@ramsan.of.borg> Hi Geert, Thanks for bringing this to my attention. >> + u32 nbytes = 0; /* the number of bytes written by this function */ >> + >> + u64 nbytes_expected; /* the number of bytes we should have written */ >> + u64 nbytes_streamed; /* the number of bytes we actually wrote */ >> + u32 num_null_pkts; /* number of null packets to bridge the gap */ >> + >> + u64 elapsed_time_msecs = jiffies_to_usecs(m->timing.current_jiffies - >> + m->timing.past_jiffies); >> + >> + elapsed_time_msecs = min(elapsed_time_msecs, >> (u64)VIDTV_MAX_SLEEP_USECS / 1000); >> + nbytes_expected = div64_u64(m->mux_rate_kbytes_sec * 1000, MSEC_PER_SEC); > > Seriously?!? > > You multiply by 1000 first, followed by a division by 1000 using an > expensive 64-by-64 division? This entire function is broken and needs a do-over :) > using an expensive 64-by-64 division? I am new to kernel development. I wasn't even aware that this was expensive, to be honest. >> + if (nbytes_streamed < nbytes_expected) { >> + /* can't write half a packet: roundup to a 188 multiple */ >> + nbytes_expected = roundup(nbytes_expected - nbytes_streamed, TS_PACKET_LEN); > > drivers/media/test-drivers/vidtv/vidtv_mux.o: In function `vidtv_mux_tick': > vidtv_mux.c:(.text+0x788): undefined reference to `__udivdi3' > > This is a 64-by-32 division, hence it should use a helper from > <linux/math64.h>. > > However, I'm wondering if "nbytes_expected - nbytes_streamed" is > guaranteed to be a "small" number, hence a 32-by-32 division would be > sufficient? I think so. I will send a patch to address the things you pointed out in this email. -- thanks -- Daniel
WARNING: multiple messages have this Message-ID (diff)
From: "Daniel W. S. Almeida" <dwlsalmeida@gmail.com> To: Geert Uytterhoeven <geert@linux-m68k.org> Cc: "mchehab+huawei@kernel.org" <mchehab+huawei@kernel.org>, "r.verdejo@samsung.com" <r.verdejo@samsung.com>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "nicolas@ndufresne.ca" <nicolas@ndufresne.ca>, "linux-kernel-mentees@lists.linuxfoundation.org" <linux-kernel-mentees@lists.linuxfoundation.org>, "linux-media@vger.kernel.org" <linux-media@vger.kernel.org> Subject: Re: [Linux-kernel-mentees] [v10 3/4] media: vidtv: add a bridge driver Date: Tue, 15 Sep 2020 10:26:17 -0300 [thread overview] Message-ID: <EA89ED5D-E477-4464-B523-D729796CE0D0@getmailspring.com> (raw) In-Reply-To: <alpine.DEB.2.21.2009151345001.31296@ramsan.of.borg> Hi Geert, Thanks for bringing this to my attention. >> + u32 nbytes = 0; /* the number of bytes written by this function */ >> + >> + u64 nbytes_expected; /* the number of bytes we should have written */ >> + u64 nbytes_streamed; /* the number of bytes we actually wrote */ >> + u32 num_null_pkts; /* number of null packets to bridge the gap */ >> + >> + u64 elapsed_time_msecs = jiffies_to_usecs(m->timing.current_jiffies - >> + m->timing.past_jiffies); >> + >> + elapsed_time_msecs = min(elapsed_time_msecs, >> (u64)VIDTV_MAX_SLEEP_USECS / 1000); >> + nbytes_expected = div64_u64(m->mux_rate_kbytes_sec * 1000, MSEC_PER_SEC); > > Seriously?!? > > You multiply by 1000 first, followed by a division by 1000 using an > expensive 64-by-64 division? This entire function is broken and needs a do-over :) > using an expensive 64-by-64 division? I am new to kernel development. I wasn't even aware that this was expensive, to be honest. >> + if (nbytes_streamed < nbytes_expected) { >> + /* can't write half a packet: roundup to a 188 multiple */ >> + nbytes_expected = roundup(nbytes_expected - nbytes_streamed, TS_PACKET_LEN); > > drivers/media/test-drivers/vidtv/vidtv_mux.o: In function `vidtv_mux_tick': > vidtv_mux.c:(.text+0x788): undefined reference to `__udivdi3' > > This is a 64-by-32 division, hence it should use a helper from > <linux/math64.h>. > > However, I'm wondering if "nbytes_expected - nbytes_streamed" is > guaranteed to be a "small" number, hence a 32-by-32 division would be > sufficient? I think so. I will send a patch to address the things you pointed out in this email. -- thanks -- Daniel _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
next prev parent reply other threads:[~2020-09-16 0:36 UTC|newest] Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-08-21 12:58 [v10 0/4] media: vidtv: Implement a virtual DVB driver Daniel W. S. Almeida 2020-08-21 12:58 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-08-21 12:58 ` [v10 1/4] media: vidtv: implement a tuner driver Daniel W. S. Almeida 2020-08-21 12:58 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-08-21 12:58 ` [v10 2/4] media: vidtv: implement a demodulator driver Daniel W. S. Almeida 2020-08-21 12:58 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-08-21 12:58 ` [v10 3/4] media: vidtv: add a bridge driver Daniel W. S. Almeida 2020-08-21 12:58 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-09-15 11:53 ` Geert Uytterhoeven 2020-09-15 11:53 ` [Linux-kernel-mentees] " Geert Uytterhoeven 2020-09-15 13:26 ` Daniel W. S. Almeida [this message] 2020-09-15 13:26 ` Daniel W. S. Almeida 2020-09-15 13:35 ` Geert Uytterhoeven 2020-09-15 13:35 ` [Linux-kernel-mentees] " Geert Uytterhoeven 2020-09-15 18:13 ` Daniel W. S. Almeida 2020-09-15 18:13 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-09-16 7:01 ` Mauro Carvalho Chehab 2020-09-16 7:09 ` Geert Uytterhoeven 2020-08-21 12:58 ` [v10 4/4] media: Documentation: vidtv: Add ReST documentation for vidtv Daniel W. S. Almeida 2020-08-21 12:58 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-09-11 8:02 ` [v10 0/4] media: vidtv: Implement a virtual DVB driver Mauro Carvalho Chehab 2020-09-11 8:02 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-09-11 12:18 ` Daniel W. S. Almeida 2020-09-11 12:18 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-09-11 13:10 ` Mauro Carvalho Chehab 2020-09-11 13:56 ` Mauro Carvalho Chehab 2020-09-12 2:54 ` Daniel W. S. Almeida 2020-09-12 7:38 ` Mauro Carvalho Chehab 2020-09-12 8:21 ` Hans Verkuil 2020-09-12 8:21 ` [Linux-kernel-mentees] " Hans Verkuil 2020-09-12 9:15 ` Mauro Carvalho Chehab 2020-09-12 9:15 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-09-12 14:49 ` Daniel W. S. Almeida 2020-09-12 14:49 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-09-12 17:57 ` Mauro Carvalho Chehab 2020-09-12 17:57 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-09-14 8:33 ` Hans Verkuil 2020-09-14 8:33 ` [Linux-kernel-mentees] " Hans Verkuil 2020-09-12 8:35 ` Hans Verkuil 2020-09-12 8:35 ` [Linux-kernel-mentees] " Hans Verkuil
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=EA89ED5D-E477-4464-B523-D729796CE0D0@getmailspring.com \ --to=dwlsalmeida@gmail.com \ --cc=geert@linux-m68k.org \ --cc=linux-kernel-mentees@lists.linuxfoundation.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-media@vger.kernel.org \ --cc=mchehab+huawei@kernel.org \ --cc=nicolas@ndufresne.ca \ --cc=r.verdejo@samsung.com \ --cc=skhan@linuxfoundation.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.