| Message ID | 20210106083318.39407-1-867314078@qq.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fix lock error when BT IRQ preempt BT timer | expand |
On 1/6/21 2:03 PM, lixg wrote: > BT IRQ may preempt BT timer if BMC response host when bt msg timeout. > When BT IRQ preempt BT timer, the infight_bt_msg did not protected by bt.lock very well. > > And we will see the following log: > [29006114.163785853,3] BT: seq 0x81 netfn 0x0a cmd 0x23: Timeout sending message > [29006114.288029290,3] BT: seq 0x81 netfn 0x0b cmd 0x23: Timeout sending message > [29006114.288917798,3] IPMI: Incorrect netfn 0x0b in response > > It may cause 'CPU Hardlock UP', 'memory refree', 'kernel crash' or something else... > > Signed-off-by: lixg <867314078@qq.com> > --- > hw/bt.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/hw/bt.c b/hw/bt.c > index cf967f89..24e6ef7f 100644 > --- a/hw/bt.c > +++ b/hw/bt.c > @@ -111,7 +111,7 @@ struct bt { > }; > > static struct bt bt; > -static struct bt_msg *inflight_bt_msg; /* Holds in flight message */ > +static struct bt_msg * volatile inflight_bt_msg; /* Holds in flight message */ Why do we need volatile here? > > static int ipmi_seq; > > @@ -211,6 +211,11 @@ static void bt_msg_del(struct bt_msg *bt_msg) > { > list_del(&bt_msg->link); > bt.queue_len--; > + > + /* once inflight_bt_msg out of list, it should be emptyed */ > + if (bt_msg == inflight_bt_msg) > + inflight_bt_msg = NULL; > + > unlock(&bt.lock); > ipmi_cmd_done(bt_msg->ipmi_msg.cmd, > IPMI_NETFN_RETURN_CODE(bt_msg->ipmi_msg.netfn), > @@ -394,7 +399,7 @@ static void bt_expire_old_msg(uint64_t tb) > bt_msg_del(bt_msg); > > /* Ready to send next message */ > - inflight_bt_msg = NULL; > + //inflight_bt_msg = NULL; We don't want to keep commented code. Please remove this. -Vasant
On 1/11/21 8:39 AM, DD Aric li wrote: > Vasant Hegde <hegdevasant@linux.vnet.ibm.com> 于2021年1月7日周四 下午7:36写道： > >> On 1/6/21 2:03 PM, lixg wrote: >>> BT IRQ may preempt BT timer if BMC response host when bt msg timeout. >>> When BT IRQ preempt BT timer, the infight_bt_msg did not protected by >> bt.lock very well. >>> >>> And we will see the following log: >>> [29006114.163785853,3] BT: seq 0x81 netfn 0x0a cmd 0x23: Timeout sending >> message >>> [29006114.288029290,3] BT: seq 0x81 netfn 0x0b cmd 0x23: Timeout sending >> message >>> [29006114.288917798,3] IPMI: Incorrect netfn 0x0b in response >>> >>> It may cause 'CPU Hardlock UP', 'memory refree', 'kernel crash' or >> something else... >>> >>> Signed-off-by: lixg <867314078@qq.com> >>> --- >>> hw/bt.c | 9 +++++++-- >>> 1 file changed, 7 insertions(+), 2 deletions(-) >>> >>> diff --git a/hw/bt.c b/hw/bt.c >>> index cf967f89..24e6ef7f 100644 >>> --- a/hw/bt.c >>> +++ b/hw/bt.c >>> @@ -111,7 +111,7 @@ struct bt { >>> }; >>> >>> static struct bt bt; >>> -static struct bt_msg *inflight_bt_msg; /* Holds in flight message */ >>> +static struct bt_msg * volatile inflight_bt_msg; /* Holds in flight >> message */ >> >> Why do we need volatile here? >> > > There will be multiple thread write " inflight_bt_msg ", so we need to > ensure that it is always the latest value. This is protected by locks. So we are good right? -Vasant
On 1/11/21 3:26 PM, DD Aric li wrote: > Hi Vasant： > I wonder if every core will buffer infight_bt_msg in it own cache. > If that true, can the lock ensure that the value of "infight_bt_msg" read > by every thread is the latest? I think lock will take care of it. We don't need volatile. -Vasant > > Vasant Hegde <hegdevasant@linux.vnet.ibm.com> 于2021年1月11日周一 下午2:12写道： > >> On 1/11/21 8:39 AM, DD Aric li wrote: >>> Vasant Hegde <hegdevasant@linux.vnet.ibm.com> 于2021年1月7日周四 下午7:36写道： >>> >>>> On 1/6/21 2:03 PM, lixg wrote: >>>>> BT IRQ may preempt BT timer if BMC response host when bt msg timeout. >>>>> When BT IRQ preempt BT timer, the infight_bt_msg did not protected by >>>> bt.lock very well. >>>>> >>>>> And we will see the following log: >>>>> [29006114.163785853,3] BT: seq 0x81 netfn 0x0a cmd 0x23: Timeout >> sending >>>> message >>>>> [29006114.288029290,3] BT: seq 0x81 netfn 0x0b cmd 0x23: Timeout >> sending >>>> message >>>>> [29006114.288917798,3] IPMI: Incorrect netfn 0x0b in response >>>>> >>>>> It may cause 'CPU Hardlock UP', 'memory refree', 'kernel crash' or >>>> something else... >>>>> >>>>> Signed-off-by: lixg <867314078@qq.com> >>>>> --- >>>>> hw/bt.c | 9 +++++++-- >>>>> 1 file changed, 7 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/hw/bt.c b/hw/bt.c >>>>> index cf967f89..24e6ef7f 100644 >>>>> --- a/hw/bt.c >>>>> +++ b/hw/bt.c >>>>> @@ -111,7 +111,7 @@ struct bt { >>>>> }; >>>>> >>>>> static struct bt bt; >>>>> -static struct bt_msg *inflight_bt_msg; /* Holds in flight message */ >>>>> +static struct bt_msg * volatile inflight_bt_msg; /* Holds in flight >>>> message */ >>>> >>>> Why do we need volatile here? >>>> >>> >>> There will be multiple thread write " inflight_bt_msg ", so we need to >>> ensure that it is always the latest value. >> >> This is protected by locks. So we are good right? >> >> -Vasant >> >
On Tue, Jan 12, 2021 at 4:24 PM Vasant Hegde <hegdevasant@linux.vnet.ibm.com> wrote: > > On 1/11/21 3:26 PM, DD Aric li wrote: > > Hi Vasant： > > I wonder if every core will buffer infight_bt_msg in it own cache. > > If that true, can the lock ensure that the value of "infight_bt_msg" read > > by every thread is the latest? > > I think lock will take care of it. We don't need volatile. Just FYI only one half of this conversation is going to the mailing list. Anyway, the kernel docs have a decent overview of why the locking primitives should be used rather than volatile: https://www.kernel.org/doc/Documentation/process/volatile-considered-harmful.rst The locks in skiboot were designed along similar lines to the spinlocks in the kernel so most of the advice there applies to skiboot. If you're seeing crashes which seem to be fixed by adding "volatile" then I'd say that some code which should be taking the lock isn't. There's also some potential pitfalls around the ordering of MMIO operations vs unlock() since the lwsync() we do on unlock doesn't enforce ordering between MMIO and regular memory ops, but I'd rule out the missing lock before going down that rabbit hole. Oliver
On 1/12/21 11:13 AM, Oliver O'Halloran wrote: > On Tue, Jan 12, 2021 at 4:24 PM Vasant Hegde > <hegdevasant@linux.vnet.ibm.com> wrote: >> >> On 1/11/21 3:26 PM, DD Aric li wrote: >>> Hi Vasant： >>> I wonder if every core will buffer infight_bt_msg in it own cache. >>> If that true, can the lock ensure that the value of "infight_bt_msg" read >>> by every thread is the latest? >> >> I think lock will take care of it. We don't need volatile. > > Just FYI only one half of this conversation is going to the mailing list. Thanks! Looks like accidentally I marked `lixgemail@gmail.com` ID to filter list. I have fixed it. Now we should get all emails to mailing list. > > Anyway, the kernel docs have a decent overview of why the locking > primitives should be used rather than volatile: > https://www.kernel.org/doc/Documentation/process/volatile-considered-harmful.rst > The locks in skiboot were designed along similar lines to the > spinlocks in the kernel so most of the advice there applies to > skiboot. If you're seeing crashes which seem to be fixed by adding > "volatile" then I'd say that some code which should be taking the lock > isn't. There's also some potential pitfalls around the ordering of > MMIO operations vs unlock() since the lwsync() we do on unlock doesn't > enforce ordering between MMIO and regular memory ops, but I'd rule out > the missing lock before going down that rabbit hole. Thanks for the details. -Vasant
diff --git a/hw/bt.c b/hw/bt.c index cf967f89..24e6ef7f 100644 --- a/hw/bt.c +++ b/hw/bt.c @@ -111,7 +111,7 @@ struct bt { }; static struct bt bt; -static struct bt_msg *inflight_bt_msg; /* Holds in flight message */ +static struct bt_msg * volatile inflight_bt_msg; /* Holds in flight message */ static int ipmi_seq; @@ -211,6 +211,11 @@ static void bt_msg_del(struct bt_msg *bt_msg) { list_del(&bt_msg->link); bt.queue_len--; + + /* once inflight_bt_msg out of list, it should be emptyed */ + if (bt_msg == inflight_bt_msg) + inflight_bt_msg = NULL; + unlock(&bt.lock); ipmi_cmd_done(bt_msg->ipmi_msg.cmd, IPMI_NETFN_RETURN_CODE(bt_msg->ipmi_msg.netfn), @@ -394,7 +399,7 @@ static void bt_expire_old_msg(uint64_t tb) bt_msg_del(bt_msg); /* Ready to send next message */ - inflight_bt_msg = NULL; + //inflight_bt_msg = NULL; /* * Timing out a message is inherently racy as the BMC
BT IRQ may preempt BT timer if BMC response host when bt msg timeout. When BT IRQ preempt BT timer, the infight_bt_msg did not protected by bt.lock very well. And we will see the following log: [29006114.163785853,3] BT: seq 0x81 netfn 0x0a cmd 0x23: Timeout sending message [29006114.288029290,3] BT: seq 0x81 netfn 0x0b cmd 0x23: Timeout sending message [29006114.288917798,3] IPMI: Incorrect netfn 0x0b in response It may cause 'CPU Hardlock UP', 'memory refree', 'kernel crash' or something else... Signed-off-by: lixg <867314078@qq.com> --- hw/bt.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)