Re: updates?

[Charles Cui]

Thanks for your feedbacks, I will try to fix these problems as soon as
possible.



Thanks, Charles.

2016-07-14 8:20 GMT-07:00 Christos Zoulas <chri...@zoulas.com>:

> On Jul 14, 5:09pm, charles...@gmail.com (Charles Cui) wrote:
> -- Subject: Re: updates?
>
> | Hi Christos,
> |
> | actually, I have a draft implementation of real time signals,
> | Let me know if you guys think it is enough (worth to spend time to
> complete
> | it or switch to cross process synchronization).
> | Here is the commit,
> |
> https://github.com/ycui1984/posixtestsuite/commit/cb9f0e1f5c2516a0ac38e457c26b8ed623e6f235
>
> There is also the issue of handling sigqueue properly, delivery order, etc:
> http://pubs.opengroup.org/onlinepubs/007908775/xsh/realtime.html
>
> christos
>

[Christos Zoulas]

On Jul 17, 2:02am, charles...@gmail.com (Charles Cui) wrote:
-- Subject: Re: updates?

| Hi Christos,
|

| I considered the questions that you asked.
| Here is another patch that improves the previous one in terms of error
| checking
| https://github.com/ycui1984/posixtestsuite/blob/master/patches/SIGNAL/0003-improve-error-checking.patch
|
| I have checked the queue size is the total number instead of per signal.
| my patch can pass the tests here,
| https://github.com/ycui1984/posixtestsuite/blob/master/conformance/interfaces/sigqueue/9-1.c
| which can work as an unit test.
| In terms of the sig queue delivery order, for real time signals, I need to
| familiar with the
| consuming logics in the netbsd code base. I guess maybe we need to sort
| real time signals when enqueuing,
| or selecting the correct real time signal when consuming.
|
| My action items,
| to familiar with signal consuming logic in netbsd, to make sure real time
| signals are always consumed based on its signal number (the smaller the
| real time signal number, the earlier to be consumed.)

Thanks. What does FreeBSD do? Can we have a separate queue for realtime
signals and deliver those first?

christos

[Charles Cui]

Hi Christos,

I considered the questions that you asked.
Here is another patch that improves the previous one in terms of error
checking
https://github.com/ycui1984/posixtestsuite/blob/master/patches/SIGNAL/0003-improve-error-checking.patch

I have checked the queue size is the total number instead of per signal.
my patch can pass the tests here,
https://github.com/ycui1984/posixtestsuite/blob/master/conformance/interfaces/sigqueue/9-1.c
which can work as an unit test.
In terms of the sig queue delivery order, for real time signals, I need to
familiar with the
consuming logics in the netbsd code base. I guess maybe we need to sort
real time signals when enqueuing,
or selecting the correct real time signal when consuming.

My action items,
to familiar with signal consuming logic in netbsd, to make sure real time
signals are always consumed based on its signal number (the smaller the
real time signal number, the earlier to be consumed.)

Thanks, Charles.

2016-07-15 21:15 GMT-07:00 Charles Cui <charles...@gmail.com>:

> Thanks for your feedbacks, I will try to fix these problems as soon as
> possible.
>
>
>
> Thanks, Charles.
>
> 2016-07-14 8:20 GMT-07:00 Christos Zoulas <chri...@zoulas.com>:
>

>> On Jul 14, 5:09pm, charles...@gmail.com (Charles Cui) wrote:
>> -- Subject: Re: updates?
>>
>> | Hi Christos,
>> |

[Charles Cui]

I will study FreeBSD logic first and share with you guys.

2016-07-17 6:39 GMT-07:00 Christos Zoulas <chri...@zoulas.com>:

> On Jul 17, 2:02am, charles...@gmail.com (Charles Cui) wrote:
> -- Subject: Re: updates?
>
> | Hi Christos,
> |

> | I considered the questions that you asked.
> | Here is another patch that improves the previous one in terms of error
> | checking
> |
> https://github.com/ycui1984/posixtestsuite/blob/master/patches/SIGNAL/0003-improve-error-checking.patch
> |
> | I have checked the queue size is the total number instead of per signal.
> | my patch can pass the tests here,
> |
> https://github.com/ycui1984/posixtestsuite/blob/master/conformance/interfaces/sigqueue/9-1.c
> | which can work as an unit test.
> | In terms of the sig queue delivery order, for real time signals, I need
> to
> | familiar with the
> | consuming logics in the netbsd code base. I guess maybe we need to sort
> | real time signals when enqueuing,
> | or selecting the correct real time signal when consuming.
> |
> | My action items,
> | to familiar with signal consuming logic in netbsd, to make sure real time
> | signals are always consumed based on its signal number (the smaller the
> | real time signal number, the earlier to be consumed.)
>

[Christos Zoulas]

In article <CA+SXE9t2KmC8rsdohdTtaXQLPV44L=Jbuco6X1cT...@mail.gmail.com>,
Charles Cui <charles...@gmail.com> wrote:
>-=-=-=-=-=-

>
>I will study FreeBSD logic first and share with you guys.
>
>2016-07-17 6:39 GMT-07:00 Christos Zoulas <chri...@zoulas.com>:
>
>> On Jul 17, 2:02am, charles...@gmail.com (Charles Cui) wrote:
>> -- Subject: Re: updates?
>>
>> | Hi Christos,
>> |
>> | I considered the questions that you asked.
>> | Here is another patch that improves the previous one in terms of error
>> | checking
>> |
>>
>https://github.com/ycui1984/posixtestsuite/blob/master/patches/SIGNAL/0003-improve-error-checking.patch

I think it is better instead of using ret = -1, to set error =
EAGAIN, from the beginning and return 0 for success and error (with
errno) for failure like other kernel functions do.

>-=-=-=-=-=-

What I would do now is write a synthetic test using
sigqueue and sigprocmask to queue a bunch of signals and then release
the mask and test for delivery order. Something like:

int siglist[] = {
SIGINT, SIGSEGV, SIGRTMIN + 2, SIGRTMIN + 1, SIGINT, SIGRTMIN + 1,
SIGRTMIN + 2, ...
};

void
handler(int sig, siginfo_t *info, void *aux) {
/* check signal order and sigval */
}


...
/* block signals you want delivered */
/* setup a handler for the signals you want to deliver */
for (size_t i = 0; i < __arraycount(siglist); i++)
/* queue signals you want to deliver */
/* unblock signals... */
/* pause until all signals have been delivered */
...


I would run this on different OS's to see what they do...

christos

[Charles Cui]

Please see my comments below.

2016-07-20 7:47 GMT-07:00 Christos Zoulas <chri...@astron.com>:

> In article <CA+SXE9t2KmC8rsdohdTtaXQLPV44L=

> Jbuco6X1cT...@mail.gmail.com>,
> Charles Cui <charles...@gmail.com> wrote:
> >-=-=-=-=-=-
> >
> >I will study FreeBSD logic first and share with you guys.
> >
> >2016-07-17 6:39 GMT-07:00 Christos Zoulas <chri...@zoulas.com>:
> >
> >> On Jul 17, 2:02am, charles...@gmail.com (Charles Cui) wrote:
> >> -- Subject: Re: updates?
> >>
> >> | Hi Christos,
> >> |
> >> | I considered the questions that you asked.
> >> | Here is another patch that improves the previous one in terms of error
> >> | checking
> >> |
> >>
> >
> https://github.com/ycui1984/posixtestsuite/blob/master/patches/SIGNAL/0003-improve-error-checking.patch
>
> I think it is better instead of using ret = -1, to set error =
> EAGAIN, from the beginning and return 0 for success and error (with
> errno) for failure like other kernel functions do.
>

I did not quite understand it.
https://github.com/ycui1984/posixtestsuite/blob/master/patches/REALTIME_SIGNAL/0004-improve-error-checking.patch
This patch removes all ret = -1 logics, and set error at kpsignal2,
did you want to set error code inside sigput?

>-=-=-=-=-=-
>
> What I would do now is write a synthetic test using
> sigqueue and sigprocmask to queue a bunch of signals and then release
> the mask and test for delivery order. Something like:
>
> int siglist[] = {
> SIGINT, SIGSEGV, SIGRTMIN + 2, SIGRTMIN + 1, SIGINT, SIGRTMIN + 1,
> SIGRTMIN + 2, ...
> };
>
> void
> handler(int sig, siginfo_t *info, void *aux) {
> /* check signal order and sigval */
> }
>
>
> ...
> /* block signals you want delivered */
> /* setup a handler for the signals you want to deliver */
> for (size_t i = 0; i < __arraycount(siglist); i++)
> /* queue signals you want to deliver */
> /* unblock signals... */
> /* pause until all signals have been delivered */
> ...
>
>
> I would run this on different OS's to see what they do...
>

The handling order of realtime signal and standard signals are not defined
by POSIX.
Some OSes handle realtime signal in FIFO order, but some select the
realtime signal with lowest number first.
It's still good to know the real results, though.


> christos
>
>

[Christos Zoulas]

In article <CA+SXE9uJ7LG40XyT0g1v-CzN...@mail.gmail.com>,

Charles Cui <charles...@gmail.com> wrote:

>I did not quite understand it.
>https://github.com/ycui1984/posixtestsuite/blob/master/patches/REALTIME_SIGNAL/0004-improve-error-checking.patch
>This patch removes all ret = -1 logics, and set error at kpsignal2,
>did you want to set error code inside sigput?

Yes sigput should return EAGAIN, the ret variable should be called error,
and all errors from sigput() should goto discard (unless there is some
cleanup that needs to be done).

>The handling order of realtime signal and standard signals are not defined
>by POSIX.
>Some OSes handle realtime signal in FIFO order, but some select the
>realtime signal with lowest number first.
>It's still good to know the real results, though.

Yes, and it is good to verify that the ordering for realtime signals is
correct too.

christos

[Christos Zoulas]

On Jul 21, 10:33am, charles...@gmail.com (Charles Cui) wrote:
-- Subject: Re: updates?

| Got it, I will fix it.

Thanks!

| OK, I can write a program as you designed. Right now I have netbsd and
| Linux environments, and
| can verify the ordering in these platforms.

Once you write it we can test more :-)

christos

[Charles Cui]

2016-07-21 10:16 GMT-07:00 Christos Zoulas <chri...@astron.com>:

> In article <
> CA+SXE9uJ7LG40XyT0g1v-CzN...@mail.gmail.com>,
> Charles Cui <charles...@gmail.com> wrote:
>
> >I did not quite understand it.
> >
> https://github.com/ycui1984/posixtestsuite/blob/master/patches/REALTIME_SIGNAL/0004-improve-error-checking.patch
> >This patch removes all ret = -1 logics, and set error at kpsignal2,
> >did you want to set error code inside sigput?
>
> Yes sigput should return EAGAIN, the ret variable should be called error,
> and all errors from sigput() should goto discard (unless there is some
> cleanup that needs to be done).
>

Got it, I will fix it.

>

> >The handling order of realtime signal and standard signals are not defined
> >by POSIX.
> >Some OSes handle realtime signal in FIFO order, but some select the
> >realtime signal with lowest number first.
> >It's still good to know the real results, though.
>
> Yes, and it is good to verify that the ordering for realtime signals is
> correct too.
>

OK, I can write a program as you designed. Right now I have netbsd and
Linux environments, and
can verify the ordering in these platforms.

>

> christos
>
>

[Charles Cui]

Hi Christos,
Here is a improved patch based on your code review,
https://github.com/ycui1984/posixtestsuite/blob/master/patches/REALTIME_SIGNAL/0005-improve-error-checking-improve-based-on-code-review.patch

I will send you the signal test later once I am ready.

2016-07-21 11:32 GMT-07:00 Christos Zoulas <chri...@zoulas.com>:

> On Jul 21, 10:33am, charles...@gmail.com (Charles Cui) wrote:
> -- Subject: Re: updates?
>

> | Got it, I will fix it.
>

> Thanks!

>
> | OK, I can write a program as you designed. Right now I have netbsd and
> | Linux environments, and
> | can verify the ordering in these platforms.
>

[Charles Cui]

FreeBSD used a similar method with us.
There is one queue for both realtime signals and standard signals.
For each real time signal, it is pushed back to the end of the queue.
Here is its code, http://bxr.su/FreeBSD/sys/kern/kern_sig.c#351
I need to research how signals are consumed though.

2016-07-17 7:11 GMT-07:00 Charles Cui <charles...@gmail.com>:

> I will study FreeBSD logic first and share with you guys.
>

> 2016-07-17 6:39 GMT-07:00 Christos Zoulas <chri...@zoulas.com>:

>
>> On Jul 17, 2:02am, charles...@gmail.com (Charles Cui) wrote:
>> -- Subject: Re: updates?
>>

>> | Hi Christos,
>> |
>> | I considered the questions that you asked.
>> | Here is another patch that improves the previous one in terms of error
>> | checking
>> |
>> https://github.com/ycui1984/posixtestsuite/blob/master/patches/SIGNAL/0003-improve-error-checking.patch
>> |

[Christos Zoulas]

In article <CA+SXE9u+2SNfNC7YoV3c_b52...@mail.gmail.com>,
Charles Cui <charles...@gmail.com> wrote:
>-=-=-=-=-=-

>
>Hi Christos,
>Here is a improved patch based on your code review,
>https://github.com/ycui1984/posixtestsuite/blob/master/patches/REALTIME_SIGNAL/0005-improve-error-checking-improve-based-on-code-review.patch
>
>I will send you the signal test later once I am ready.

That looks good. Did you mean to goto discard; instead of goto out;?

christos

[Christos Zoulas]

On Jul 23, 12:15pm, charles...@gmail.com (Charles Cui) wrote:
-- Subject: Re: updates?

| I considered about this, it seems that if the variable kp is successfully
| allocated,
| we need to goto out to free kp first (which prevents memory leak).
| But if we never allocate the variable kp, and encountered an error,
| we need to goto discard, which does not free kp.
| Make sense?

Yes, it does. How is the test program coming along?

Thanks,

christos

[Christos Zoulas]

On Jul 23, 8:03pm, charles...@gmail.com (Charles Cui) wrote:
-- Subject: Re: updates?

| Hi Christos,
|
| I have attached a program to test the delivery order of signals.
| It turns out that both Linux and our current implementation use a FIFO
| policy to deliver signal, (1. only realtime signals and 2. mix of realtime
| signals and standard signals)
| I do not know how FreeBSD handles it.
| Let me know if there are any problems.

Ok good start. Let's add more things in the queue (or split it into
separate tests) to show that:
- realtime signals are ordered properly
- you can queue more than one realtime signal
- don't use sighold/sigrelse. these are obsolete and release
only one signal at a time. use sigprocmask (plus since you are
releasing them one at a time, the release order is the order
you are going to see them delivered.

Then we can turn it into an ATF test.

Thanks,

christos

[Charles Cui]

I considered about this, it seems that if the variable kp is successfully
allocated,
we need to goto out to free kp first (which prevents memory leak).
But if we never allocate the variable kp, and encountered an error,
we need to goto discard, which does not free kp.
Make sense?

>

> christos
>
>

[Charles Cui]

Hi Christos,

I have attached a program to test the delivery order of signals.
It turns out that both Linux and our current implementation use a FIFO
policy to deliver signal, (1. only realtime signals and 2. mix of realtime
signals and standard signals)
I do not know how FreeBSD handles it.
Let me know if there are any problems.

[Charles Cui]

I have attached the new version.
But it seems both Linux and and netbsd will process signal with large signo
number first.

2016-07-23 23:44 GMT-07:00 Christos Zoulas <chri...@zoulas.com>:

> On Jul 23, 8:03pm, charles...@gmail.com (Charles Cui) wrote:
> -- Subject: Re: updates?
>

> | Hi Christos,
> |
> | I have attached a program to test the delivery order of signals.
> | It turns out that both Linux and our current implementation use a FIFO
> | policy to deliver signal, (1. only realtime signals and 2. mix of
> realtime
> | signals and standard signals)
> | I do not know how FreeBSD handles it.
> | Let me know if there are any problems.
>

[Christos Zoulas]

On Jul 24, 3:00pm, charles...@gmail.com (Charles Cui) wrote:
-- Subject: Re: updates?

| I have attached the new version.
| But it seems both Linux and and netbsd will process signal with large signo
| number first.

Great, that is correct. I've modified slightly your test_signal.c and put
it in http://www.netbsd.org/~christos/test_signal.c. I am building a new
kernel and userland with all your changes. While this is happening can
you fill in the sigorder() function in test_signal.c and test that it works?

The correct output should be:

Inside Handler = 36
Inside Handler = 35
Inside Handler = 35
Inside Handler = 34
Inside Handler = 3
Inside Handler = 2

Best,

christos

[Charles Cui]

Sure, I can implement the sigorder function and will get back to you soon.


Thanks, Charles.

[Christos Zoulas]

On Jul 24, 9:19pm, charles...@gmail.com (Charles Cui) wrote:
-- Subject: Re: updates?

| Sure, I can implement the sigorder function and will get back to you soon.

So I am running the kernel with all the RT changes, and it does not seem
to work properly (it only delivers each RT signal once, where it should
deliver RTMIN + 1 twice). FYI, FreeBSD queues all signals, not just the
realtime ones so it prints:

FreeBSD:
Inside Handler = 67
Inside Handler = 66
Inside Handler = 66
Inside Handler = 65

Inside Handler = 3
Inside Handler = 2

Inside Handler = 2

NetBSD with or withour RT patches:

Inside Handler = 36
Inside Handler = 35

Inside Handler = 34
Inside Handler = 3
Inside Handler = 2

Linux:

Inside Handler = 36
Inside Handler = 35
Inside Handler = 35
Inside Handler = 34
Inside Handler = 3
Inside Handler = 2

christos

[Christos Zoulas]

On Jul 25, 8:13pm, charles...@gmail.com (Charles Cui) wrote:
-- Subject: Re: updates?

| I have attached the test_signal v2 version, which fills the signal
| reorder function. After we fix the bug that you found,
| we can use this program to test.

Great; I think that the following works too and does not use a temp
array. I have not tested it, and perhaps yours is faster?

christos

/*
* given a array of signals to be delivered in tosend of size len
* place in ordered the signals to be delivered in delivery order
* and return the number of signals that should be delivered
*/
static size_t
sigorder(int *ordered, const int *tosend, size_t len)
{
if (len == 0)
return len;

memcpy(ordered, tosend, len * sizeof(*tosend));
qsort(ordered, len, sizeof(*ordered), compare);

for (size_t i = 0; i < len - 1; i++) {
if (ordered[i] >= SIGRTMIN || ordered[i] != ordered[i + 1])
continue;
for (size_t j = i + 1; j < len - 1; j++)
ordered[j] = ordered[j + 1];
len--;
i--;
}
return len;
}

[Charles Cui]

yeah, it seems correct,
my solution use another array, so space complicity is O(n), time complicity
is O(nlogn) for sorting and O(n) for copying to new array. Your solution
has space complicity O(1), time complicity is O(nlogn) + O(n^2).

[Charles Cui]

I have attached the test_signal v2 version, which fills the signal
reorder function. After we fix the bug that you found,
we can use this program to test.

2016-07-25 16:03 GMT-07:00 Charles Cui <charles...@gmail.com>:

> I have see how to debug this. Hopefully my understanding for signal
> delivery is enough now.
>
> 2016-07-25 13:13 GMT-07:00 Christos Zoulas <chri...@zoulas.com>:

>
>> On Jul 24, 9:19pm, charles...@gmail.com (Charles Cui) wrote:
>> -- Subject: Re: updates?
>>

[Charles Cui]

Hi Christos,
This patch fixed the bug you found
https://github.com/ycui1984/posixtestsuite/blob/master/patches/REALTIME_SIGNAL/0006-bug-fix-for-real-time-signals.patch

2016-07-25 21:33 GMT-07:00 Charles Cui <charles...@gmail.com>:

> yeah, it seems correct,
> my solution use another array, so space complicity is O(n), time
> complicity is O(nlogn) for sorting and O(n) for copying to new array. Your
> solution has space complicity O(1), time complicity is O(nlogn) + O(n^2).
>

> 2016-07-25 20:24 GMT-07:00 Christos Zoulas <chri...@zoulas.com>:

>
>> On Jul 25, 8:13pm, charles...@gmail.com (Charles Cui) wrote:
>> -- Subject: Re: updates?
>>

>> | I have attached the test_signal v2 version, which fills the signal
>> | reorder function. After we fix the bug that you found,
>> | we can use this program to test.
>>

[Christos Zoulas]

On Jul 26, 8:15pm, charles...@gmail.com (Charles Cui) wrote:
-- Subject: Re: updates?

| Hi Christos,
| This patch fixed the bug you found
| https://github.com/ycui1984/posixtestsuite/blob/master/patches/REALTIME_SIGNAL/0006-bug-fix-for-real-time-signals.patch
|

Ok, I think that:

1. sp can't be NULL; if it was the code would die.
2. ret can't be 0 so we should KASSERT?
3. Is there a worry about efficiency?

christos

[Christos Zoulas]

On Jul 27, 8:23am, charles...@gmail.com (Charles Cui) wrote:
-- Subject: Re: updates?

| 3. there will be a concern if we count the number of signals each time,
| but this sigcnts logic may can be merged with siggetinfo.

Can you try doing that?

| 2. I can add kassert around ret.
| 1. I know sp cannot be NULL, and if it is NULL, the sigcnt is zero, what do
| you mean of code would die (which piece of code)?

I mean the code that unconditionally deleted the signal from the set.

christos

[Charles Cui]

3. there will be a concern if we count the number of signals each time,
but this sigcnts logic may can be merged with siggetinfo.

2. I can add kassert around ret.
1. I know sp cannot be NULL, and if it is NULL, the sigcnt is zero, what do
you mean of code would die (which piece of code)?

2016-07-27 3:10 GMT-07:00 Christos Zoulas <chri...@zoulas.com>:

> On Jul 26, 8:15pm, charles...@gmail.com (Charles Cui) wrote:
> -- Subject: Re: updates?
>

[Charles Cui]

Yeah, I will try to improve the code efficiency.

2016-07-27 10:14 GMT-07:00 Christos Zoulas <chri...@zoulas.com>:

> On Jul 27, 8:23am, charles...@gmail.com (Charles Cui) wrote:
> -- Subject: Re: updates?
>

> | 3. there will be a concern if we count the number of signals each time,
> | but this sigcnts logic may can be merged with siggetinfo.
>

> Can you try doing that?
>

> | 2. I can add kassert around ret.
> | 1. I know sp cannot be NULL, and if it is NULL, the sigcnt is zero, what
> do
> | you mean of code would die (which piece of code)?
>

[Charles Cui]

This patch
https://github.com/ycui1984/posixtestsuite/blob/master/patches/REALTIME_SIGNAL/0007-improve-efficiency.patch

improves the code efficiency and removes duplicate search in the signal
queue.

[Christos Zoulas]

In article <CA+SXE9sEcy0w9CEzsQXPoc2w...@mail.gmail.com>,

Charles Cui <charles...@gmail.com> wrote:
>-=-=-=-=-=-
>

>This patch
>https://github.com/ycui1984/posixtestsuite/blob/master/patches/REALTIME_SIGNAL/0007-improve-efficiency.patch
>
>improves the code efficiency and removes duplicate search in the signal
>queue.

yes, but where does the ksiginfo get freed now since you removed:
- ksiginfo_free(ksi); /* XXXSMP */

christos

>
>2016-07-27 10:34 GMT-07:00 Charles Cui <charles...@gmail.com>:
>
>> Yeah, I will try to improve the code efficiency.
>>
>> 2016-07-27 10:14 GMT-07:00 Christos Zoulas <chri...@zoulas.com>:
>>
>>> On Jul 27, 8:23am, charles...@gmail.com (Charles Cui) wrote:
>>> -- Subject: Re: updates?
>>>
>>> | 3. there will be a concern if we count the number of signals each time,
>>> | but this sigcnts logic may can be merged with siggetinfo.
>>>
>>> Can you try doing that?
>>>
>>> | 2. I can add kassert around ret.
>>> | 1. I know sp cannot be NULL, and if it is NULL, the sigcnt is zero,
>>> what do
>>> | you mean of code would die (which piece of code)?
>>>
>>> I mean the code that unconditionally deleted the signal from the set.
>>>
>>> christos
>>>
>>
>>
>

>-=-=-=-=-=-

[Charles Cui]

2016-07-28 5:03 GMT-07:00 Christos Zoulas <chri...@astron.com>:

> In article <
> CA+SXE9sEcy0w9CEzsQXPoc2w...@mail.gmail.com>,
> Charles Cui <charles...@gmail.com> wrote:
> >-=-=-=-=-=-
> >
> >This patch
> >
> https://github.com/ycui1984/posixtestsuite/blob/master/patches/REALTIME_SIGNAL/0007-improve-efficiency.patch
> >
> >improves the code efficiency and removes duplicate search in the signal
> >queue.
>
> yes, but where does the ksiginfo get freed now since you removed:
> - ksiginfo_free(ksi); /* XXXSMP */
>

well, the original logic only finds one target signal and return true, at
that time ksi is pointing to some data,
in my case, I need to loop all signals to return the count, and at the end
of the loop, ksi is set to be NULL.

[Christos Zoulas]

In article <CA+SXE9tdKyLz-YsS8Gu=dP79jwarxj5Fx1E2KcqomM=rYE...@mail.gmail.com>,

Charles Cui <charles...@gmail.com> wrote:

>> yes, but where does the ksiginfo get freed now since you removed:
>> - ksiginfo_free(ksi); /* XXXSMP */
>>
>well, the original logic only finds one target signal and return true, at
>that time ksi is pointing to some data,
>in my case, I need to loop all signals to return the count, and at the end
>of the loop, ksi is set to be NULL.

Ok, there are multiple ksi entries for the same signal in the loop. Each
invocation returns the count of them and pops the first one and returns
the count. Setting it to NULL does not free the data, you need to free
the data for the ksiginfo you freed. If you keep running the code you
should see signal memory accumulate.

christos

[Charles Cui]

Hi Christos,

This patch fix the memory leak that you found in the RT patch.

https://github.com/ycui1984/posixtestsuite/blob/master/patches/REALTIME_SIGNAL/0008-bug-fix.patch

2016-07-29 9:07 GMT-07:00 Charles Cui <charles...@gmail.com>:

> I will see how to improve this part.
>
>
> Thanks Charles
>
> 2016-07-29 1:00 GMT-07:00 Christos Zoulas <chri...@astron.com>:
>
>> In article <CA+SXE9tdKyLz-YsS8Gu=dP79jwarxj5Fx1E2KcqomM=

[Christos Zoulas]

On Jul 29, 4:28pm, charles...@gmail.com (Charles Cui) wrote:
-- Subject: Re: updates?

| Hi Christos,
|
| This patch fix the memory leak that you found in the RT patch.
|
| https://github.com/ycui1984/posixtestsuite/blob/master/patches/REALTIME_SIGNAL/0008-bug-fix.patch
|

Thanks but how are you testing this stuff? I just applied all the patche and
now I can't login to my vm via ssh. Signal delivery seems completely broken.
If I ^C a kernel make, I get from the kernel "sendsig: bad version 0", and
then Illegal Instruction. Does this work for you?

christos

[Charles Cui]

I have see how to debug this. Hopefully my understanding for signal
delivery is enough now.

2016-07-25 13:13 GMT-07:00 Christos Zoulas <chri...@zoulas.com>:

> On Jul 24, 9:19pm, charles...@gmail.com (Charles Cui) wrote:
> -- Subject: Re: updates?
>

[Charles Cui]

[Christos Zoulas]

On Jul 30, 11:42am, charles...@gmail.com (Charles Cui) wrote:
-- Subject: Re: updates?

| Yes, it works for me, each time I send you a new patch, I will compile my
| kernel and userland,
| and testing using the program we have written.
|
| Here is a short video that I created from my environment which shows the
| real time signal works for me
| https://www.dropbox.com/s/amds3jmpfev8jz9/realtime_signal.mov?dl=0
|
| Also, I looked at all files needed in the real time signal patches,
| and attached all files of them, you can replace using these files and see
| whether there are any
| differences with the current settings.
|
| /usr/src/sys/sys/signal.h
|
| /usr/src/include/limits.h
|
| /usr/src/lib/libc/gen/sysconf.c
|
| /usr/src/sys/kern/kern_sig.c
|
| /usr/src/sys/kern/sys_sig.c
|
| /usr/src/sys/sys/signalvar.h
|
| /usr/src/sys/sys/unistd.h
|
| 2016-07-30 9:36 GMT-07:00 Christos Zoulas <chri...@zoulas.com>:

There are two issues with the code that I found and I have everything
working now. It is not enough to test with the test program (which
works); for example as I mentioned before, booting with a kernel
that has all your patches fails on sshd (you can't login remotely)
and an interrupted make does not work properly. If you want I can
show you what's wrong with the changes, but I think it is better
if you find the problems yourself. The idea here is to make sure
that the code works exactly like before in the absense of real-time
signals, which is not the case currently. Both bugs are in kern_sig.c.
One of them is the critical one that breaks signal delivery, the other
is more of a clarification of what happens when we want to maintain
the pending signal mask.

Best,

christos

[Charles Cui]

Hi Christos,

Thanks for providing these information!
I will revisit my current code base and think carefully
how those two bugs are generated (by keeping your hints in mind).
Hopefully I can find the correct place and realize my mistakes. :)

Thanks Charles

[Charles Cui]

Hi Christos,

I spend time in studying the current code base, and checkout to previous
commit to see where I introduced the problems (just like a binary search).
In this way, I hope to narrow down to the real problem.
One thing that I found was my sshd is still working even applying all those
patches.
I can login into the vm via ssh, and run tests.
By the way, how do you fix the problems which makes your sshd broken?

[Christos Zoulas]

In article <CA+SXE9v9C=SfjQxZRCXLBSpKg=2sN-jSJH2hW0...@mail.gmail.com>,
Charles Cui <charles...@gmail.com> wrote:
>-=-=-=-=-=-

>
>Hi Christos,
>
> I spend time in studying the current code base, and checkout to previous
>commit to see where I introduced the problems (just like a binary search).
>In this way, I hope to narrow down to the real problem.
>One thing that I found was my sshd is still working even applying all those
>patches.
>I can login into the vm via ssh, and run tests.
>By the way, how do you fix the problems which makes your sshd broken?

1. In siggetinfo(), if we exit the loop without finding a signal, count == 0
and we return right there without initializing "out". This is what breaks
things for me.
2. In sigget we only remove from the set if ret == 1 (but this is not a bug
really); I think it is better to do (more similar to the old code):

sigdelset(&sp->sp_set, signo);
out:
count = siggetinfo(sp, out, signo);
if (count > 1)
sigaddset(&sp->sp_set, signo);

christos

[Charles Cui]

Hi Christos,

Please see my comments below about the real time signal bugs.
By the way, do you have any plans to commit this part?
I can change the testing program to be a unit test.


2016-08-03 0:10 GMT-07:00 Christos Zoulas <chri...@astron.com>:

> In article <CA+SXE9v9C=SfjQxZRCXLBSpKg=

> 2sN-jSJH2hW0...@mail.gmail.com>,
> Charles Cui <charles...@gmail.com> wrote:
> >-=-=-=-=-=-
> >
> >Hi Christos,
> >
> > I spend time in studying the current code base, and checkout to
> previous
> >commit to see where I introduced the problems (just like a binary search).
> >In this way, I hope to narrow down to the real problem.
> >One thing that I found was my sshd is still working even applying all
> those
> >patches.
> >I can login into the vm via ssh, and run tests.
> >By the way, how do you fix the problems which makes your sshd broken?
>
> 1. In siggetinfo(), if we exit the loop without finding a signal, count ==
> 0
> and we return right there without initializing "out". This is what
> breaks
> things for me.
>

yeah, this is a bug. I looked at the code several times, but never noticed
this problem :)

> 2. In sigget we only remove from the set if ret == 1 (but this is not a bug
> really); I think it is better to do (more similar to the old code):
>
> sigdelset(&sp->sp_set, signo);
> out:
> count = siggetinfo(sp, out, signo);
> if (count > 1)
> sigaddset(&sp->sp_set, signo);
>

got it, this looks better.

>
> christos
>
>

[Christos Zoulas]

On Aug 3, 2:29pm, charles...@gmail.com (Charles Cui) wrote:
-- Subject: Re: updates?

| Hi Christos,
|
| Please see my comments below about the real time signal bugs.
| By the way, do you have any plans to commit this part?
| I can change the testing program to be a unit test.

I've done this already :-) Please stay tuned.

christos